Details

Reviewers

William Reynish (billreynish)
Ray Molenkamp (LazyDodo)
Julian Eisel (Severin)

Group Reviewers

User Interface

Commits

rB7c2217cd126a: UI: File Browser Volumes and System Lists Icons

Summary

The File Browser has some list on the left side showing system volumes and other shortcuts. Although those items have icons, there is a limitation in that you can only have one icon PER LIST. Every item in each list has the same icon.

This patch makes it so that each item in the "System" and "Volumes" lists can be added with unique icons. Therefore volumes can have different icons per drive type. And each System icon can be different, if desired, to better differentiate them. Like having a special icon for "Desktop" or "My Documents", etc.

Note that the above is a capture from my Windows machine after applying this patch. It does not show changes to Volumes icons because we don't have those new icons properly added to the project. They will be added soon:

Diff Detail

Repository: rB Blender

Event Timeline

Harley Acheson (harley) created this revision.Sep 15 2019, 5:12 PM

noki paike (amonpaike) awarded a token.Sep 15 2019, 5:23 PM

The spirit of the patch is good, but in testing I couldn't get it to do what you claim it does:

This revision now requires changes to proceed.Sep 15 2019, 6:58 PM

Interesting. Did you rebuild target? There is a change to a python file included here.

Updated patch to use new "desktop", "removable drive", and "network drive" icons.

Note that this patch adds the ABILITY to use unique icons in the Volume and System lists, where we now they all have to be the same. And adds this ability for all platforms.

But this patch then uses that ability to display nicer icons when it can for Windows users, since drive list interrogation is platform-specific. Other platforms can do so as well. They just need to specify a desired icon as they add each entry item with fsmenu_insert_entry().

How it looks, on my Windows system, after this patch is applied:

The "recents" section is still there?

@ThinkingPolygons (ThinkingPolygons) - The "recents" section is still there?

It sure is. I just make my interface scale large so it is scrolled below in that capture.

👍

@Harley Acheson (harley) Ok, it was not clear to me that this was initially Windows-specific. I am fine with supporting Windows directories first and adding more later. But I'm then not able to review this. Adding @Ray Molenkamp (LazyDodo) and @Julian Eisel (Severin) instead to test on Windows.

The spirit of the patch I have no issue with, but it's better if someone actually on Windows can test.

William Reynish (billreynish) added reviewers: Julian Eisel (Severin), User Interface.Sep 15 2019, 10:10 PM

William Reynish (billreynish) added a subscriber: William Reynish (billreynish).

Linux "Desktop" System item can use ICON_DESKTOP

Martin Capitanio (capnm) awarded a token.Sep 16 2019, 8:50 PM

Henrik Dick (weasel) awarded a token.Sep 17 2019, 4:09 PM

updating for current state of master

Updated for current state of master.

Replacing icon used for Fonts directory with ICON_FONTPREVIEW.

Wouldn't it be easier to scan with the eyes if the drive letter comes first, then the name?

@Julian Eisel (Severin) - Wouldn't it be easier to scan with the eyes if the drive letter comes first, then the name?

The names in the drive list? Possibly. What we see in that capture matches exactly what we see in Windows. It used to be that we took any label and appended the " (X:)" to it, but now that text is coming directly from a Windows API call.

It is probably because the drive letters are a bit secondary, while the device is longer-lived. Some device that is now mapped to "H" could be mapped to something different later.

In D5802#135807, @Julian Eisel (Severin) wrote:

Wouldn't it be easier to scan with the eyes if the drive letter comes first, then the name?

I agree, drive letters first would be easier to read.

In D5802#135809, @Harley Acheson (harley) wrote:

It is probably because the drive letters are a bit secondary, while the device is longer-lived. Some device that is now mapped to "H" could be mapped to something different later.

Then again you probably save most often to "fixed volumes" like local drives, or network mapped locations, that are generally attributed the same drive on boot.
Also drive letters are usually the same character length, something along the lines of (D:) while volume labels can vary, which creates a nice visual alignment that is easy to follow like

(C:) Windows
(D:) Local Disk
(E:) Blu Ray
(F:) Home
(G:) Backup

as opposed to the more visually disconnected

Windows (C:)
Local Disk (D:)
Blu Ray (E:)
Home (F:)
Backup (G:)

@Duarte Farrajota Ramos (duarteframos) - I agree, drive letters first would be easier to read.

We could swap that around, but...

That entire string - in that particular order and including the drive letter - is provided by the operating system as the definitive display name for each volume. It isn't something I've put together. It should match exactly what you see in the operating system when it shows you your list of drives.

Platform wise, not a huge fan of sprinkling #ifdef win32's all over the place, but there currently is no place to segregate platform specific stuff like this, so what you have now will do just fine for now.

Took a quick peek, UI wise it would be nice if CD/DVD drives have their own icon, they now show up as an external drive which people will probably nitpick over.

I have to agree with @Harley Acheson (harley) here that it be best to follow windows explorer here and stick to the Description (Letter:) formatting.

From a technical perspective i have nothing to hold up this diff up and i'll leave it up to the UI guys to have the final say here.

This revision is now accepted and ready to land.Oct 6 2019, 7:59 PM

Thanks @Ray Molenkamp (LazyDodo)!

Although, for the record, should say that Julien’s (and others) comments on the drive description string are off topic as this patch doesn’t change that at all, only the icons shown beside the text. The change to volume labels - for Windows only - was done in an earlier patch committed quite a while ago.

@Ray Molenkamp (LazyDodo) - it would be nice if CD/DVD drives have their own icon, they now show up as an external drive which people will probably nitpick over.

I'm torn on this one. It would be pretty easy for jendrych to add a little CD/DVD icon. But then we'd also need a large version of it too. Although we are not currently using them we have large "volume" icons to go along with the large document and folder icons we see when in thumbnail view. The idea is to (eventually) have a "computer root" display.

@Harley Acheson (harley) If we need a root view, we'll need a large disk icon probably anyway. We could add this, and add support for it after the icon is added.

@William Reynish (billreynish) - yes, you are right. And is all a bit outside of this patch anyway. In the end this patch doesn't really do much more than add the ability to have distinct icons for each item in the "Volumes" and "System" lists. What items are on those lists and what icons are shown can easily be added, corrected, changed, later.

William Reynish (billreynish) accepted this revision as: William Reynish (billreynish).Oct 6 2019, 9:58 PM

Updated for current state of master.

Using new "CD" icon for DVD drives. New "Documents" and "Temp" folder icons.

Mine looks like this now...

William Reynish (billreynish) accepted this revision.Oct 30 2019, 2:04 PM

Harley Acheson (harley) edited reviewers, added: Campbell Barton (campbellbarton); removed: Brecht Van Lommel (brecht).Oct 31 2019, 5:30 PM

Updating for current state of master (change of identifier for an icon used here).

Also set Linux gvfs shares to use ICON_NETWORK_DRIVE

Pablo Vazquez (pablovazquez) awarded a token.Nov 11 2019, 2:36 PM

Please note that reviewers might not see "cdrom" icon while testing. That identifier was changed but the associated icon DAT files have not (yet) been renamed in the repository. Am hoping to do that when committing this patch.

Updated to current state of master.

Also moved where ED_file_init() is called in wm_init_exit.c so that it is called after profile loading since some FSMenu items might contain profile paths. Needed since we are not refreshing constantly any more. So start with initial list after preferences.

And also added ED_file_init() to the space_file listener so that the FSMenu lists are refreshed (just once!) every time the window is opened. Only needed with this patch since I want some System list items to be based off some preference file paths.

@Julian Eisel (Severin) could you check if this is ok for inclusion in 2.82?

Found one issue, but besides that this seems fine to me.

source/blender/editors/space_file/fsmenu.c
345	Can just call `ED_fsmenu_entry_set_icon()` here.
source/blender/editors/space_file/space_file.c
356 ↗	(On Diff #19956)	Is this really needed? It makes Blender re-read all file icons from disk, which is not totally cheap, but should be entirely unnecessary (since it's already been done on startup). Worse, it causes to re-read the icons without freeing the old ones first. Causes a memory leak of 95MB here :)

This revision now requires changes to proceed.Dec 6 2019, 11:52 AM

Updating to current state of master, and...

Addressing @Julian Eisel (Severin)'s comments:

calling ED_fsmenu_entry_set_icon() rather than setting icon directly
not calling ED_file_init() on browser show. Was only there in case some user preferences were changed, and that is not needed now and can be done in a different way later when it is needed.

Also removed the extra System list items, like temp and font folders. These were mostly added to the patch in order to show off the utility of having per-item icons. I will instead propose separate patches later as those deserve separate discussion. For example, I like all of these on Windows, but there might be some overlap with the Mac list.

Julian Eisel (Severin) accepted this revision.Dec 6 2019, 7:23 PM

This revision is now accepted and ready to land.Dec 6 2019, 7:23 PM

Harley Acheson (harley) removed a reviewer: Campbell Barton (campbellbarton).Dec 6 2019, 7:36 PM

Brandon (Brandon777) awarded a token.Dec 6 2019, 10:20 PM

Closed by commit rB7c2217cd126a: UI: File Browser Volumes and System Lists Icons (authored by Harley Acheson (harley)). · Explain WhyDec 6 2019, 10:22 PM

This revision was automatically updated to reflect the committed changes.

Harley Acheson (harley) added a commit: rB7c2217cd126a: UI: File Browser Volumes and System Lists Icons.

The Thumbnail folders will now show special inset icons if the path is in the System list.

For example, the "Desktop" folder is one of the special folders in the "System" list so when viewing that folder (in Thumbnail view) it will be shown with a special inset icon. For examples:

I accidentally updated this (diff) with a different patch.

UI: File Browser Volumes and System Lists Icons
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 20140

release/scripts/startup/bl_ui/space_filebrowser.py

source/blender/editors/include/ED_fileselect.h

source/blender/editors/interface/interface_icons.c

source/blender/editors/space_file/file_ops.c

source/blender/editors/space_file/fsmenu.h

source/blender/editors/space_file/fsmenu.c

source/blender/makesrna/intern/rna_space.c

source/blender/windowmanager/intern/wm_init_exit.c

UI: File Browser Volumes and System Lists IconsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 20140

release/scripts/startup/bl_ui/space_filebrowser.py

source/blender/editors/include/ED_fileselect.h

source/blender/editors/interface/interface_icons.c

source/blender/editors/space_file/file_ops.c

source/blender/editors/space_file/fsmenu.h

source/blender/editors/space_file/fsmenu.c

source/blender/makesrna/intern/rna_space.c

source/blender/windowmanager/intern/wm_init_exit.c

UI: File Browser Volumes and System Lists Icons
ClosedPublic
Actions

Revision Contents
Changeset List