Page MenuHome
Differential D6816

UI: Windows File Attributes in File Browser
ClosedPublic
Actions

Authored by Harley Acheson (harley) on Feb 12 2020, 3:22 AM.

Details

Reviewers
Campbell Barton (campbellbarton)
William Reynish (billreynish)
Bastien Montagne (mont29)
Commits
rB1fb62d1272db: UI: Windows File Attributes and Hidden Items
Summary

This patch adds support for Windows file attributes in the File Browser, and does so in a way that we can also do so for Mac and Linux later too.

Currently the file browser hides all files with names that start with a period, even though these files are not hidden on Windows. With this patch such files are only hidden on Linux and Mac. But files that have the "hidden" attribute are now shown on Windows. But on all platforms if you choose to show hidden files they will be shown with lower opacity.

Further, folders that are marked as "system" are shown specially. And folders that are Mount Points, Hard Links, and soft links are shown specially as well.

The following capture of my own user folder matches that shown in the operating system:

Diff Detail

Repository
rB Blender

Event Timeline

Harley Acheson (harley) created this revision.Feb 12 2020, 3:22 AM
Harley Acheson (harley) added a reviewer: William Reynish (billreynish).Feb 12 2020, 3:35 AM
Duarte Farrajota Ramos (duarteframos) awarded a token.Feb 12 2020, 1:03 PM
Duarte Farrajota Ramos (duarteframos) added a subscriber: Duarte Farrajota Ramos (duarteframos).
Harley Acheson (harley) added a comment.Feb 12 2020, 4:58 PM

Some examples of bug reports this patch addresses:

Jacques Lucke (JacquesLucke) added a subscriber: Jacques Lucke (JacquesLucke).Feb 12 2020, 6:05 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/editors/space_file/file_draw.c
144

I find the name is_hidden a bit confusing. It can mean that the file is hidden inside of Blender or "in the OS". Usually it means something is hidden in Blender, so it does not make sense to draw it. However, I couldn't come up with a better name right now, so it's probably fine.

163

Can you add a comment that explains what these values mean? Does not seem very obvious to me.

source/blender/editors/space_file/filelist.c
618

!ELEM(filename[1], '.', '\0')

652

Variables should not have the exact same name as functions if it can be avoided.

Harley Acheson (harley) updated this revision to Diff 21664.Feb 12 2020, 7:03 PM

Updated to address concerns and suggestions by @Jacques Lucke (JacquesLucke):

I find the name is_hidden a bit confusing...

Yes, and at that point it is in the drawing code so really isn't even a file that is hidden, but an icon to be drawn with lowered opacity. So changed that to "dimmed".

Can you add a comment that explains what these values mean?

Yes, I added a comment explaining how that odd combination of values results in an icon with lowered opacity.

`!ELEM(filename[1], '.', '\0')`

Thanks!

Variables should not have the exact same name as functions if it can be avoided.

Good point. I made those two function much clearer by naming one "is_hidden_dot_filename" and the other "is_filtered_hidden".

Campbell Barton (campbellbarton) requested changes to this revision.Feb 13 2020, 3:08 AM
Campbell Barton (campbellbarton) added a reviewer: Bastien Montagne (mont29).

Overall patch seems fine.

Would keep the flag member of the struct, and not mix FILE_ENTRY_INVALID_PREVIEW with filesystem attributes, so we don't confuse these with internal run-time flags.

source/blender/blenlib/BLI_fileops.h
78

Mixing this with file attributes seems strange.

source/blender/editors/space_file/filelist.c
217

A typed enum could be used here.

This revision now requires changes to proceed.Feb 13 2020, 3:08 AM
Harley Acheson (harley) updated this revision to Diff 21676.Feb 13 2020, 5:04 AM

Updated based on review and suggestions from @Campbell Barton (campbellbarton):

Would keep the flag member of the struct, and not mix FILE_ENTRY_INVALID_PREVIEW with filesystem attributes, so we don't confuse these with internal run-time flags.

Yes done. I'm not sure why I was trying so hard to save a short. LOL.

Mixing this with file attributes seems strange.

No longer mixed.

A typed enum could be used here.

Not done, only because I couldn't find any other examples where we use a variable of type enum to hold bit flags. These values aren't exclusive in that a file item could be read-only, hidden, and system at once. Since the combined value wouldn't be a member of the enum, wouldn't we then have to use typecasting and treat it as an int anyway? Sorry if I am just not understanding your intent. "C" still holds some mystery to me.

Harley Acheson (harley) updated this revision to Diff 21703.Feb 14 2020, 2:33 AM

Updating as per @Campbell Barton (campbellbarton)'s suggestion to use enum vars for the file attributes.

Harley Acheson (harley) mentioned this in D6743: WIP: File Attributes and Linked Files.Feb 14 2020, 6:49 PM
Ankit Meel (ankitm) mentioned this in D6679: Fix T60682: adds macOS alias redirection for directories.Feb 17 2020, 12:21 PM
Ankit Meel (ankitm) added a subscriber: Ankit Meel (ankitm).Feb 18 2020, 10:43 AM
Campbell Barton (campbellbarton) requested changes to this revision.Feb 20 2020, 10:41 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenlib/intern/storage.c
202

This should take a filename, as other file functions.

If it's useful to do this from the file selector, there could be a wrapper function like file_extension_type.

OTOH, if multiple functions are making full paths, it could be better to assign this once and pass the path in.

This revision now requires changes to proceed.Feb 20 2020, 10:41 AM
Harley Acheson (harley) updated this revision to Diff 21932.Feb 20 2020, 6:56 PM

Updated to current state of master and to address suggestions by @Campbell Barton (campbellbarton).

BLI_file_attributes() changed to take a full path (like other similar functions there), rather than separate root and relpath.

Harley Acheson (harley) added a comment.Feb 20 2020, 6:59 PM

Hey @Bastien Montagne (mont29)! Do mind taking a quick look at this? Is good to go otherwise.

Bastien Montagne (mont29) accepted this revision.Feb 21 2020, 10:05 AM

From quick check LGTM. Hope we can get mac/linux attributes handling soon too. :)

source/blender/editors/space_file/file_draw.c
793

const bool ;)

Harley Acheson (harley) updated this revision to Diff 21962.Feb 21 2020, 5:12 PM

Updated to current state of master.
Fixed missing "const" qualifier, as suggested by @Bastien Montagne (mont29)

Hope we can get mac/linux attributes handling soon too. :)

Yes, I have a couple people in mind to conscript for that.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 21 2020, 5:21 PM
Closed by commit rB1fb62d1272db: UI: Windows File Attributes and Hidden Items (authored by Harley Acheson (harley)). · Explain Why
This revision was automatically updated to reflect the committed changes.
Harley Acheson (harley) added a commit: rB1fb62d1272db: UI: Windows File Attributes and Hidden Items.

Revision Contents
Changeset List

PathSize
source/
blender/
blenlib/
24 lines
intern/
63 lines
editors/
include/
2 lines
space_file/
30 lines
116 lines
makesdna/
2 lines

Diff 21963

View Options

source/blender/blenlib/BLI_fileops.h

Loading...
View Options

source/blender/blenlib/intern/storage.c

Loading...
View Options

source/blender/editors/include/UI_icons.h

Loading...
View Options

source/blender/editors/space_file/file_draw.c

Loading...
View Options

source/blender/editors/space_file/filelist.c

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...