Details

Reviewers

Brecht Van Lommel (brecht)
Bastien Montagne (mont29)
Julian Eisel (Severin)

Maniphest Tasks

T60682: macOS alias folders not visible in file browser

Commits

rBafb1a64ccb81: Fix T60682: adds macOS alias redirection for directories

Summary

Fixes T60682

Currently in the File Browser, symlinks are supported but not aliases(which are GUI friendly). So this patch adds that support. Carbon API has been Deprecated, so Foundation is being used. All methods are supported for > macOS 10.10.

The folder icon for redirection was added by D6816

Currently if I open "fake", it goes to "original" folder. Clicking back arrow brings me back to Desktop; directory up arrow takes me to Downloads. There doesn't seem compelling need to allow opening blender files via aliases, or adding scripts etc. Only folders are functional.

notes:
GHOST_SystemPathsCocoa.mm has two functions : one to get all required attributes/ resourcekeys using NSURLResourceKey methods; other resolves file path and copies it in the provided buffer. All other ghost files are wrappers around it.

Diff Detail

Repository

rB Blender

Branch

aliasattributes (branched from master)

Build Status

Buildable 6728
Build 6728: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This revision now requires changes to proceed.Feb 3 2020, 1:24 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:

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

Multiple #defines of PATH_MAX 1024.

@Harley Acheson (harley) had comments about caching the file list and attributes etc for speed. https://blender.chat/channel/ui?msg=CeX7vw6W3xcoADwG9
To choose between usage of @autoreleasepool{} vs manual [pool drain] at every return event.

The underlying code works though.

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 Meel (ankitm) edited the summary of this revision. (Show Details)Feb 3 2020, 7:14 PM

In D6679#158060, @Brecht Van Lommel (brecht) wrote:

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 Meel (ankitm) mentioned this in T60682: macOS alias folders not visible in file browser.Feb 4 2020, 8:53 PM

Switched to arcanist and clang-format

Harbormaster completed remote builds in B6495: Diff 21406.Feb 4 2020, 11:26 PM

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.

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

Ankit Meel (ankitm) retitled this revision from Verifies if a file is an alias and resolves its actual path, if it is. to Fix T60682: adds macOS alias redirection for directories .Feb 23 2020, 4:21 PM

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

Ankit Meel (ankitm) added projects: BF Blender, Platform: macOS.

Ankit Meel (ankitm) added a task: T60682: macOS alias folders not visible in file browser.

blenlib/CMakeLists.txt (I guess) needs to be edited to fix the hardcoded path to intern/ghost/GHOST_Path-api.h.I tried with the same method as I did in space_file/CMakeLists.txt but couldn't fix build errors.

Harbormaster completed remote builds in B6728: Diff 21979.Feb 23 2020, 4:24 PM

cleanup

Harbormaster completed remote builds in B6729: Diff 21980.Feb 23 2020, 4:38 PM

Julian Eisel (Severin) resigned from this revision.Feb 24 2020, 12:02 PM

On Linux, I got multiple build errors, so I had to do some changes. With these changes I also didn't need to use an absolute patch for the Ghost includes:

P1272 Linux compile fixes for D6679

1	diff --git a/intern/ghost/GHOST_ISystemPaths.h b/intern/ghost/GHOST_ISystemPaths.h
2	index 9ba53954667..df572cf84bf 100644
3	--- a/intern/ghost/GHOST_ISystemPaths.h
4	+++ b/intern/ghost/GHOST_ISystemPaths.h
5	@@ -99,12 +99,14 @@ class GHOST_ISystemPaths {
6	bool *aliasIsDirectory,
7	int targetPathMaxLength) const = 0;
8
9	+#ifdef __APPLE__
10	/**
11	* Get file attributes like read-only, writeable, isAlias etc.
12	* struct values defined in storage.c.
13	*/
14	virtual struct NSURLkeys getFileAttributes(const char filePath,
15	struct NSURLkeys *values) const = 0;
16	+#endif
17
18	private:
19	/** The one and only system paths*/
20	diff --git a/intern/ghost/GHOST_Path-api.h b/intern/ghost/GHOST_Path-api.h
21	index 73e4a521ec8..26c915c3beb 100644
22	--- a/intern/ghost/GHOST_Path-api.h
23	+++ b/intern/ghost/GHOST_Path-api.h
24	@@ -87,11 +87,13 @@ extern bool GHOST_resolveFilePath(const char *shortcutPath,
25	bool *aliasIsDirectory,
26	int targetPathMaxLength);
27
28	+#ifdef __APPLE__
29	/**
30	* Get file attributes like read-only, writeable, isAlias etc.
31	* struct values defined in storage.c.
32	*/
33	extern struct NSURLkeys GHOST_getFileAttributes(const char filePath, struct NSURLkeys *values);
34	+#endif
35
36	#ifdef __cplusplus
37	}
38	diff --git a/intern/ghost/intern/GHOST_Path-api.cpp b/intern/ghost/intern/GHOST_Path-api.cpp
39	index f190b46837d..175c23ee8ef 100644
40	--- a/intern/ghost/intern/GHOST_Path-api.cpp
41	+++ b/intern/ghost/intern/GHOST_Path-api.cpp
42	@@ -77,6 +77,7 @@ bool GHOST_resolveFilePath(const char *shortcutPath,
43	return 0;
44	}
45
46	+#ifdef __APPLE__
47	NSURLkeys GHOST_getFileAttributes(const char filePath, NSURLkeys *values)
48	{
49	GHOST_ISystemPaths *systemPaths = GHOST_ISystemPaths::get();
50	@@ -85,3 +86,4 @@ NSURLkeys GHOST_getFileAttributes(const char filePath, NSURLkeys *values)
51	}
52	return values;
53	}
54	+#endif
55	diff --git a/intern/ghost/intern/GHOST_SystemPaths.h b/intern/ghost/intern/GHOST_SystemPaths.h
56	index bf84291d05a..094463d21fe 100644
57	--- a/intern/ghost/intern/GHOST_SystemPaths.h
58	+++ b/intern/ghost/intern/GHOST_SystemPaths.h
59	@@ -79,11 +79,13 @@ class GHOST_SystemPaths : public GHOST_ISystemPaths {
60	char *targetPath,
61	bool *aliasIsDirectory,
62	int targetPathMaxLength) const = 0;
63	+#ifdef __APPLE__
64	/**
65	* Get file attributes like read-only, writeable, isAlias etc.
66	* struct values defined in storage.c.
67	*/
68	virtual NSURLkeys getFileAttributes(const char filePath, NSURLkeys *values) const = 0;
69	+#endif
70	};
71
72	#endif
73	diff --git a/intern/ghost/intern/GHOST_SystemPathsUnix.cpp b/intern/ghost/intern/GHOST_SystemPathsUnix.cpp
74	index 9514edb7404..572a23aa7fa 100644
75	--- a/intern/ghost/intern/GHOST_SystemPathsUnix.cpp
76	+++ b/intern/ghost/intern/GHOST_SystemPathsUnix.cpp
77	@@ -117,3 +117,13 @@ void GHOST_SystemPathsUnix::addToSystemRecentFiles(const char * /filename/) co
78	{
79	/* TODO: implement for X11 */
80	}
81	+
82	+bool GHOST_SystemPathsUnix::resolveFilePath(const char * /shortcutPath/,
83	+ char * /targetPath/,
84	+ bool * /aliasIsDirectory/,
85	+ int /targetPathMaxLength/) const
86	+{
87	+
88	+ /* TODO: implement for X11 */
89	+ return false;
90	+}
91	diff --git a/intern/ghost/intern/GHOST_SystemPathsUnix.h b/intern/ghost/intern/GHOST_SystemPathsUnix.h
92	index dcd3ab34704..41207824c91 100644
93	--- a/intern/ghost/intern/GHOST_SystemPathsUnix.h
94	+++ b/intern/ghost/intern/GHOST_SystemPathsUnix.h
95	@@ -64,6 +64,11 @@ class GHOST_SystemPathsUnix : public GHOST_SystemPaths {
96	* Add the file to the operating system most recently used files
97	*/
98	void addToSystemRecentFiles(const char *filename) const;
99	+
100	+ bool resolveFilePath(const char *shortcutPath,
101	+ char *targetPath,
102	+ bool *aliasIsDirectory,
103	+ int targetPathMaxLength) const;
104	};
105
106	#endif /* __GHOST_SYSTEMPATHSUNIX_H__ */
107	diff --git a/source/blender/blenlib/CMakeLists.txt b/source/blender/blenlib/CMakeLists.txt
108	index 5bccc0d951a..3317762470f 100644
109	--- a/source/blender/blenlib/CMakeLists.txt
110	+++ b/source/blender/blenlib/CMakeLists.txt
111	@@ -24,6 +24,7 @@ set(INC
112	../makesdna
113	../../../intern/atomic
114	../../../intern/eigen
115	+ ../../../intern/ghost
116	../../../intern/guardedalloc
117	../../../intern/numaapi/include
118	../../../extern/wcwidth
119	diff --git a/source/blender/blenlib/intern/storage.c b/source/blender/blenlib/intern/storage.c
120	index 1118e3b4656..3d6659f5179 100644
121	--- a/source/blender/blenlib/intern/storage.c
122	+++ b/source/blender/blenlib/intern/storage.c
123	@@ -70,7 +70,8 @@
124	#include "BLI_string.h"
125	#include "BLI_fileops.h"
126	#include "BLI_path_util.h"
127	-#include "/Users/ankitkumar/blender-build/blender/intern/ghost/GHOST_Path-api.h"
128	+
129	+#include "GHOST_Path-api.h"
130
131	/**
132	* Copies the current working directory into *dir (max size maxncpy), and
133	diff --git a/source/blender/editors/space_file/filelist.c b/source/blender/editors/space_file/filelist.c
134	index aa005702015..fc4f95ca698 100644
135	--- a/source/blender/editors/space_file/filelist.c
136	+++ b/source/blender/editors/space_file/filelist.c
137	@@ -82,7 +82,7 @@
138
139	#include "filelist.h"
140
141	-#include "/Users/ankitkumar/blender-build/blender/intern/ghost/GHOST_Path-api.h"
142	+#include "GHOST_Path-api.h"
143
144	/* ----------------- FOLDERLIST (previous/next) -------------- */
145

I'm not a big fan of the design of this. For one, it seems wrong to me to access Ghost from BLI. We already do it in BKE (in appdir.c), which I find just as bad, but that's a different topic.
Also, the whole point of the Ghost APIs is to abstract away OS specific types and code. The NSURLkeys and the signature of GHOST_getFileAttributes() violate this idea.

I don't have the time right now to think about alternatives or to check details. This is just some initial design feedback.

This revision now requires changes to proceed.Feb 24 2020, 2:42 PM

The need for breaking the separation between Ghost and other files came from this change:
https://developer.blender.org/D6743#inline-55733

Harbormaster completed remote builds in B6747: Diff 22010.Feb 24 2020, 7:47 PM

I still maintain that almost all of this complexity would be removed if you follow the existing pattern for this.

BLI storage.c already contains platform-specific low-level file routines. So if you put your code for Mac attribute handling directly in the APPLE section of BLI_file_attributes() it would clear and simple (since also have direct access to the eFileAttributes flag values). And similarly place a BLI_file_alias_target() in that same source file. No need to change or include any ghost files. Having these routines available to BLI is good enough.

Here is a prototype of this patch with comments in the (only) two places where you need to make changes.

MacAttributesAlias.diff5 KBDownload

included Harley's changes. removed Ghost from from filelist.c

Harbormaster completed remote builds in B6749: Diff 22014.Feb 24 2020, 9:59 PM

minor fix

Harbormaster completed remote builds in B6750: Diff 22017.Feb 24 2020, 10:09 PM

removed remnants from past merges; updated for today's master

Hi @Brecht Van Lommel (brecht)! It seems ready now.

Harbormaster completed remote builds in B6775: Diff 22076.Feb 26 2020, 8:21 AM

Brecht Van Lommel (brecht) requested changes to this revision.Feb 26 2020, 8:47 AM

Brecht Van Lommel (brecht) added inline comments.

intern/ghost/GHOST_ISystemPaths.h
93	Don't add comments about `entry->redirection_path` here, GHOST works completely independent of such data structures.
intern/ghost/GHOST_Path-api.h
35	All GHOST types and enum values must be prefixed with `GHOST_`. Further, types are defined in `GHOST_Types.h`. Using `NSURL` in the name also makes this apple specific, while the point of GHOST is to provide an abstraction. So, please change this as follows, and move it to `GHOST_Types.h`. typedef struct { bool isAlias; bool isHidden; ... } GHOST_FileAttributes;
37	This is unnecessary, we already have other ways to detect if a file is a directory.
42	This one is unused so can be removed. Presumably you can just resolve the path and then find out if it's a directory or not.
81–82	Don't add comments about `entry->redirection_path` here, GHOST works completely independent of such data structures.
intern/ghost/intern/GHOST_SystemPathsUnix.cpp
127 ↗	(On Diff #22076)	Is there actually anything to do here? I would think on Linux there is no need for this.
source/blender/blenlib/intern/storage.c
288	The point of an abstraction like ghost is that you don't have to `#ifdef` function calls like this, please remove that. What you can do is add a comment that there is no Windows implementation yet.
291	Setting `target` to `NULL` here does nothing.
source/blender/editors/space_file/file_ops.c
1547	Don't use `strcpy`, it's unsafe, use `STRNCPY` or `BLI_strncpy` instead.
source/blender/editors/space_file/filelist.c
215	Add space after `.`
2496	Is there really anything to do in this place in the code, isn't the comment in `BLI_file_alias_target` enough?

This revision now requires changes to proceed.Feb 26 2020, 8:47 AM

Brecht Van Lommel (brecht) added inline comments.Feb 26 2020, 8:57 AM

source/blender/blenlib/intern/storage.c
286	Change `char *target` to `char target[FILE_MAXDIR]`.
289	Change `PATH_MAX` to `FILE_MAXDIR`.

Julian Eisel (Severin) resigned from this revision.Feb 26 2020, 11:56 AM

Julian Eisel (Severin) added a project: User Interface.

Ankit Meel (ankitm) marked 11 inline comments as done.Feb 26 2020, 8:10 PM

Ankit Meel (ankitm) added inline comments.

intern/ghost/intern/GHOST_SystemPathsUnix.cpp
127 ↗	(On Diff #22076)	I got it from Julian, to resolve build errors on linux. Removed it. P1272 Please see if other `ifdef __APPLE__` in Ghosts are to be kept or not.
source/blender/blenlib/BLI_fileops.h
98 ↗	(On Diff #22076)	Should I change `char *target` to `char target[FILE_MAXDIR]` here too ? FILE_MAXDIR 768 definition is in DNA_space_types.h , not included in BLI_fileops.h. `PATH_MAX 1024` is in limits.h, already included, to have just this `def` here.
source/blender/blenlib/intern/storage.c
291	It is checked at 2496 in filelist.c

git doesnt seem to be picking up changes in GHOST_SystemPathsUnix.cpp upto here. my local file shows that function removed, but here, https://developer.blender.org/D6679#change-W7fHqoDeBWLW we see it.

Harbormaster completed remote builds in B6790: Diff 22098.Feb 26 2020, 8:11 PM

Ankit Meel (ankitm) marked an inline comment as not done.Feb 26 2020, 8:32 PM