Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modern aliases #2142

Merged
merged 6 commits into from Nov 14, 2015
Merged

Modern aliases #2142

merged 6 commits into from Nov 14, 2015

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Oct 28, 2015

Sorry for the name, you know that's a hard problem ;-).

This resurrects the switch to NSURL bookmarks I tried to do (#2075), since I wanted to fix the class of network-alias-stalls bugs we have and the main QS method we had actually doesn't handle all the bells and whistles of bookmark resolution. Maybe I should have added a comment on -[NSFileManager resolveAliasAtPath:] and friends but, well, I didn't. Consider yourselves warned 😏.

This one fixes the issue Rob had with the hundreds of warning messages had because symlinks. Since I wasn't keen on basing the new resolver because of shaky grounds, I preferred to keep the symlink-checking separate.

There's also a fix for -infoRec returning the symlink target info instead of the symlink itself, which makes symlink actually "invisible" (and non-working, it would seem — at least here, current master doesn't allow me to right-arrow into a symlink because the code passes a symlink path to some directory-parsing code). I wonder how you guys feel about that, especially in light of #1897, #1485, and the broken test ;-).

The icing on the cake is that "resolvables" show their target path instead of theirs.

Based on the deprecation cleanup branch for motivation so the diff is cleaner.

skurfer
Copy link
Member

@skurfer skurfer commented on abb92f5 Oct 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, should we add NSURLIsSymbolicLinkKey to the infoRec and create an isSymbolicLink method for this type of test?

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 29, 2015

Nice to see you, as always. So, let’s see. Some quick testing I did shows that this fixes

In other words, every known issue with symbolic links. I had them all assigned to me, but hadn’t gotten to them yet. Nice work!

Regarding my comment above about the UTI comparison, I found an old comment that suggests this test might not even work, but I’m seeing the target path in the details, so maybe it was fixed at the OS level?

Personally, I’d like the target path in the details string prefixed with things like “Link to” and “Alias of”. Not saying you need to bother with that right now, though. I can add it later.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 29, 2015

I wasn't too keen on adding the -isSymlink thing, but maybe for consistency that's better. I'll try to review how much manual symlink testing there is, and if it can consolidate some tests, I'll add it here.

#2063 ? I think you meant #2064. #2030 I'm not sure because of Path Finder being involved, but I think that's a red-herring. The others should be fixed, given the few tests I ran.

Speaking of tests, I'll write some. I don't think the ones we currently have cover all the cases so they behave themselves (aliased symlinks anyone ? 😆).

The comment you point out is the error I had, because of the "code passes a symlink path to some directory-parsing code". -fileUTI failing was the same reason, it returned the target's UTI.

I made my details changes while running Bezel, the goal was to make the "resolvable" nature apparent by displaying the target's path instead of the resolvable in that case, which IMHO looks nicer than (symlink) and less jarring that what you're proposing (still a path).

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 29, 2015

Actually, I meant #2036. I’ve updated my comment.

the goal was to make the "resolvable" nature apparent by displaying the target's path instead of the resolvable in that case, which IMHO looks nicer than (symlink) and less jarring that what you're proposing (still a path).

It’s apparent to me, but I’m worried it will be confusing to some people. Not something we need to sort out immediately.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 29, 2015

Oh, I'll check that one then. Haven't really looked at what happened to actions on resolvables with that change.

I was hoping that the "little alias arrow" that gets added was sufficient for conveying this was a symlink/alias. Granted, the GUI atm only says "this is a resolvable", not the actual type of it, but I don't think the difference is that meaningful. But yeah, this is something we can change.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 29, 2015

Note that after looking at #1977, I will try to make a few network aliases (I had none of those when I wrote that), and then check what's their behavior in both master and this. Sad thing that you can't use your own MBP's file sharing to test...

@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 29, 2015

#2036 is not fixed by that. It looks to me like a "feature request" to make process actions work on resolvables.

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 7, 2015

I looked through the list of actions for any that could be potentially problematic for symbolic links. All of these worked in a sane way:

  • Reveal
  • Always Open Type With…
  • Tagging
  • Move to Trash
  • Delete
  • Get Info
  • Move To…
  • Copy To…
  • Rename
  • Get File URL
  • Get Path

These had minor problems.

  • Get Location - show the HFS+ path to the target file
  • Compress - doesn’t seem to do anything (when run on a link to a folder)
  • Spotlight comment - puts comments on the original file
  • Open in Terminal - doesn’t appear in the list of actions

Only one of those (Get Location) is provided in Core Support and needs to be dealt with here.

Things to note:

  • The ones that aren’t working probably never were, but we’re so close to fixing everything, we might as well talk about them now.
  • I only tested symbolic links. Not aliases. We should probably check those, too.

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 13, 2015

Tried the same actions with aliases. I wouldn’t say they all worked as expected, but they all worked consistent with Finder, which I’d call success.

I think there’s only one place (in the core application) that we’d use -isSymlink, so it’s not a priority. The only reason I’m not merging this now is I don’t have time to write the massive release notes. 😃

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 14, 2015

I was going to fix “Get Location” for symbolic links, then merge, but I’m not going to bother.

  1. It’s using kCFURLHFSPathStyle from NSURL, so unless we want to write our own code to make HFS paths, it’s going to return the path of the target.
  2. kCFURLHFSPathStyle is deprecated.
  3. Who cares?

On that last one, I would be OK with just removing the action completely. One less thing we depend on the ND classes for. 😃

skurfer added a commit that referenced this issue Nov 14, 2015
@skurfer skurfer merged commit dc49835 into t/deprecation-cleanup Nov 14, 2015
2 checks passed
@skurfer skurfer deleted the t/modern-aliases branch Nov 14, 2015
@skurfer skurfer mentioned this pull request Nov 14, 2015
skurfer added a commit that referenced this issue Nov 14, 2015
@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 15, 2015

I may or may not have based that PR against the deprecation-cleanup branch ;-). Sorry about that, I'll resubmit.

@tiennou tiennou restored the t/modern-aliases branch Nov 15, 2015
@skurfer
Copy link
Member

@skurfer skurfer commented Nov 16, 2015

I may or may not have based that PR against the deprecation-cleanup branch ;-).

You did, but that’s already been merged, so it doesn’t hurt anything. This has been merged too, so there’s no need to resubmit. 😃

@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 16, 2015

Are you sure ? I just glanced at it after coming back from a lengthy weekend-of-music, and the graph made me thing those commits weren't in master at all (only your release notes did), so I opened #2157.

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 16, 2015

Oh, crap. You’re right. It went back to the branch it was based on, so it did hurt something. Now we have a mess. I’ll try to clean it up tonight and do another release.

skurfer added a commit that referenced this issue Nov 16, 2015
@skurfer skurfer deleted the t/modern-aliases branch Nov 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants