Page MenuHome

Verifies if a file is an alias and resolves its actual path, if it is.
Needs ReviewPublic

Authored by Ankit (ankitm) on Mon, Jan 27, 2:47 AM.

Details

Summary

T60682
Currently in the File Browser, symlinks are supported but not aliases(which are GUI friendly). So this patch adds that support.

GHOST already has Objective-C, so the path resolve function is added there and are accessible by cpp-API. Carbon API has been Deprecated, so Foundation is being used. All methods are supported for > macOS 10.10.

It checks whether a file is alias or not, if yes, resolves it and copies its target location to the buffer provided in the second argument.

Diff Detail

Repository
rB Blender
Branch
a (branched from master)
Build Status
Buildable 6495
Build 6495: arc lint + arc unit

Event Timeline

Ankit (ankitm) created this revision.Mon, Jan 27, 2:47 AM
Ankit (ankitm) edited the summary of this revision. (Show Details)Mon, Jan 27, 3:50 PM
Ankit (ankitm) edited the summary of this revision. (Show Details)Mon, Jan 27, 5:21 PM
Ankit (ankitm) edited the summary of this revision. (Show Details)Mon, Jan 27, 7:57 PM

Proper point of changing the target location needs to be determined. In filelist.c: these two functions filelist_readjob_list_dir and filelist_readjob_do handle the directory update. They talk in terms of rel_subdir, dir, root and rel_name which is not useful for aliases where a full path needs to be updated, not just the relative one. GHOST_SystemPathsCocoa::resolveAliasFromURL takes alias path and true path (which strcpy updates).

This indeed requires some refactoring. I guess FileListInternEntry can get an optional abs_resolved_path member that contains the resolved path. And then you can create a utility function to get the absolute path from an entry, that will return either the resolved path or root + relpath.

I believe we have similar issues with junction points on Windows, that could be resolved the same way. So it's worth making the code generic here.

intern/ghost/GHOST_ISystemPaths.h
92

Maybe call this resolveFilePath, since Alias and URL are rather macOS specific here.

92–94

I'm not sure we need a isAliasType function, just a single one to resolve a file path seems enough?

intern/ghost/intern/GHOST_SystemPathsCocoa.mm
120–137

This implementation is incomplete and won't work, which I guess you are aware of.

You'll need to allocate a char array, copy the resolved path into it and return it. Then let the caller free it. Just assigning pointers will not work.

Ankit (ankitm) marked 2 inline comments as done.Tue, Jan 28, 3:20 PM

I unfortunately don't have a windows to test, I can set up a Parallels desktop though.

intern/ghost/GHOST_ISystemPaths.h
92–94

there is this method https://developer.apple.com/documentation/foundation/nsurlisaliasfilekey which I think would need to be run over all the extensionless files in a particular directory to determine if there's any alias there. After it returns YES, that alias/ file would be shown with directory icon and made clickable and its true path will be obtained by the resolve function and updated wherever necessary.
so is it feasible to check all extension less files with that method NSURLIsAliasFileKey?

This was what I was thinking. Passing every file in a directory to resolve path and then trying to understand by errors if it is alias or not.. seems not good to me.

https://developer.apple.com/documentation/foundation/nsurl/1416404-urlbyresolvingaliasfileaturl This is the resolving function and it returns three things: resolved URL, nil, and original url in case the

url argument does not refer to an alias file as defined by the NSURLIsAliasFileKey property,

So should all files (which can get to a huge number in some folders) be passed to resolve?

intern/ghost/intern/GHOST_SystemPathsCocoa.mm
120–137

The case of the "same" needs to be updated, I'll put a getFileSystemRepresentation there too. Though this case is very unlikely to happen if only aliases are passed to this function. If all files are passed, this would be required.

getFileSystemRepresentation:maxLength:
Fills the provided buffer with a C string representing a local file system path.
... Returns YES if the URL could be converted into a file system representation, otherwise NO.

I have checked the code for an alias in an older diff: https://developer.blender.org/differential/changeset/?ref=237070%2F-1

finalpath was getting updated to a resolved one, getting to the MAXPATHLEN 1024 limit isn't easy.

https://developer.apple.com/documentation/foundation/nsurl/1415117-getfilesystemrepresentation?language=objc

Ankit (ankitm) marked an inline comment as done.Tue, Jan 28, 3:24 PM
intern/ghost/GHOST_ISystemPaths.h
92–94

I think it's fine to test if a file is an alias before trying to resolve it, but that can all be done in a single GHOST API function.

intern/ghost/intern/GHOST_SystemPathsCocoa.mm
120–137

I see. Instead of dynamically allocating it is also fine to use char finalPath[PATH_MAX], but then write it that way in the function declaration and definition so that it's clear what is expected.

I have understood the comments, and would make the necessary changes. But that would be Friday, got some quizzes in the way. Will mark the comments done later only.
Probably resolve function would be int to indicate -1: not alias, 0/null: error, 1: success and proceed. But I could be wrong, will check. (-:

Brecht Van Lommel (brecht) requested changes to this revision.Mon, Feb 3, 1:24 PM
This revision now requires changes to proceed.Mon, Feb 3, 1:24 PM
Ankit (ankitm) marked 3 inline comments as done.Mon, Feb 3, 7:11 PM

Icon design has been done by jendrzych devtalk :

So I tried to clear up the mentioned comments, new things came up which need review:

  1. Main: How to actually go into target directory once short-cut is clicked upon (yeah I renamed them) (no I couldn't figure it out).
  2. messages in console:
found bundled python: /Users/ankitkumar/blender-build/build/bin/Debug/Blender.app/Contents/Resources/2.83/python #normal
<time etc>SecTaskLoadEntitlements failed error=22 cs_flags=20, pid=15716  #new
<time etc>SecTaskCopyDebugDescription: Blender[15716]/0#-1 LF=0.           #new
  1. Multiple #defines of PATH_MAX 1024.
  1. @Harley Acheson (harley) had comments about caching the file list and attributes etc for speed. https://blender.chat/channel/ui?msg=CeX7vw6W3xcoADwG9
  2. To choose between usage of @autoreleasepool{} vs manual [pool drain] at every return event.

The underlying code works though.

Ankit (ankitm) updated this revision to Diff 21362.EditedMon, Feb 3, 7:12 PM

Another way to combine the two functions was to add a third argument as a flag to indicate which of the two parts is be used. Instead, I used -1 0 +1 and kept two filepath arguments.

Ankit (ankitm) edited the summary of this revision. (Show Details)Mon, Feb 3, 7:14 PM

I believe we have similar issues with junction points on Windows, that could be resolved the same way. So it's worth making the code generic here.

We currently do support Junction points (hard links to folders), soft links to folders, and mount points. But we don't mark any of them specially. Something with similar behavior is Shell Links (.lnk files).

I'm currently working on a patch that could add support for Windows file attributes and files with reparse points. With the thought that we could then do "hidden" properly on the varying platforms and would give us place to "plug in" these Mac Alias functions. Should have something in a day or two to look at anyway.

A sample so far, while experimenting. Obviously would not use the "closed eye" for folder with hidden attribute (would be dimmed instead).

Ankit (ankitm) edited the summary of this revision. (Show Details)

Switched to arcanist and clang-format

Merged all my changes in D6743 and thanks to Harley, the alias redirection occurs if target is a directory. When using left arrow, it comes back to source directory; When using up arrow, it goes to parent of target directory.

Ok, if development continues in D6743: WIP: File Attributes and Linked Files I'll abandon this one.

Ankit (ankitm) reclaimed this revision.EditedMon, Feb 17, 12:20 PM

D6816 <- this needs to go in first, then I'll request review here. Sorry for the confusion.