Page MenuHome

UI: Windows File Attributes in File Browser
ClosedPublic

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

Details

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

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.

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.Thu, Feb 13, 3:08 AM

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.Thu, Feb 13, 3:08 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.

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

Campbell Barton (campbellbarton) requested changes to this revision.Thu, Feb 20, 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.Thu, Feb 20, 10:41 AM

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.

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

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 ;)

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.Fri, Feb 21, 5:21 PM
This revision was automatically updated to reflect the committed changes.