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

Tweaks #1922

Merged
merged 6 commits into from Sep 14, 2014
Merged

Tweaks #1922

merged 6 commits into from Sep 14, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 11, 2014

Various small things. I would like them to go into v1.2.0 before its release, but the only one that REALLY needs to be included (if the mergers are strapped for time) is the URLByReallyResolvingSymlinksInPath commit.

It fixes a bug meaning you can't → into /tmp anymore (since it's a symlink)

pjrobertson added 2 commits Sep 9, 2014
If urlRef was NULL, CFRelease(NULL) --> Crash
@skurfer
Copy link
Member

@skurfer skurfer commented Sep 14, 2014

To make this easier to merge, on my local copy of this branch, I did get rebase -i HEAD~6 and just removed the AppleScript commit and the commit that reverts it. Feel free to do that and force push.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 14, 2014

OK. I can right-arrow into /tmp, but not a symlink in my home directory. I think this is what I was trying to fix with #1897. The other commits here look fine.

pjrobertson added 4 commits Sep 14, 2014
This ensures symbols are not desymbolicated, and makes it easier to find your way through the code
(Note: correct solution would be to set up using .dsym - Issue #1610)
Apple's equivalent doesn't resolve all paths (e.g. /tmp, /etc and /var)
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 14, 2014

I've just looked into #1897, and it seems that bug is caused because the NSURL NSURLIsAliasFileKey (or NSURLIsSymbolicLinkKey for that matter) both return False for a symlink in the home directory. So I think you're right that we might have to blindly resolve paths.

Personally, I would just move the line path = [manager resolveAliasAtPath:path]; up 2 lines, so it is outside if ([object isAlias]) { (QSObject_FileHandling.m:L400), but then that brings into question the point of the stuff in that if statement at all. I could probably all be removed.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 14, 2014

(or NSURLIsSymbolicLinkKey for that matter) both return False for a symlink in the home directory.

Were you trying to get it from an existing QSObject? Because we never populate that data in infoRecord.

Personally, I would just move the line path = [manager resolveAliasAtPath:path]; up 2 lines, so it is outside if ([object isAlias]) { (QSObject_FileHandling.m:L400), but then that brings into question the point of the stuff in that if statement at all.

I think had that exact same thought process, but I can’t remember what I discovered when testing different things. It sounds like you’re saying 6400a7b works as intended, so that’s fine with me. We can revisit the rest later.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 14, 2014

Were you trying to get it from an existing QSObject? Because we never populate that data in infoRecord.

Yeah, I made some changes to actually populate that, but it was still False :(

I think had that exact same thought process, but I can’t remember what I discovered when testing different things. It sounds like you’re saying 6400a7b works as intended, so that’s fine with me. We can revisit the rest later.

Yeah, but that commit fixes what it's supposed to fix (resolving /tmp) but doesn't fix #1897, which can be considered later (but probably for v1.2...

skurfer added a commit that referenced this issue Sep 14, 2014
@skurfer skurfer merged commit dfeb3cc into master Sep 14, 2014
1 check failed
@skurfer skurfer deleted the tweaks branch Sep 14, 2014
skurfer added a commit that referenced this issue Sep 14, 2014
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