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

Don't mount volumes when resolving aliases #2178

Merged
merged 2 commits into from Jan 27, 2016
Merged

Don't mount volumes when resolving aliases #2178

merged 2 commits into from Jan 27, 2016

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Jan 26, 2016

I think this is going to be controversial. This makes QS never try to mount network volumes when resolving aliases, as right now we have no way to discern whether this is a "user-initiated" resolution, or some background scan.

Thinking about it, I should have split that... The second part makes the resolution fail completely instead of returning itself. Say you're browsing some folder containing one of those remote aliases, if you right-arrowed (and had the patience to wait for it to fail), you'd end up with a new result set with only that alias, instead of the expected bump.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 26, 2016

we already have the withUI flag so to me what you've done seems fair enough. I'd like to know if right arrowing does or doesn't set withUI = True though. Hopefully it would.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Jan 26, 2016

Depends. withUI: is only used by the openFile: handler, so no right arrow. I guess we should just weight the damage that a lone remote alias can do vs. the amount of reports we've got because of stuck Quicksilvers. As a corollary, if I had 10 aliases to 10 different, accessible remote locations, I would get all 10 of them mounted on each scan...

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 27, 2016

I think this is going to be controversial.

I don’t. 😃

skurfer added a commit that referenced this issue Jan 27, 2016
Don't mount volumes when resolving aliases
@skurfer skurfer merged commit f119f96 into master Jan 27, 2016
2 checks passed
@skurfer skurfer deleted the t/fix-alias-hang branch Jan 27, 2016
@skurfer
Copy link
Member

@skurfer skurfer commented Jan 27, 2016

So is this the real fix for #2020 or just revisiting #1977? Both?

@tiennou
Copy link
Member Author

@tiennou tiennou commented Jan 28, 2016

It's a real "fix" for the deadlock in #2020, which is triggered by resolving an alias to a recognizable clippingTypes UTI. It's related to #1977 because also aliases, but this one was scanning-related right ? #2020 happens when using the interface (actually, when you move over an indirect-allowed action with an alias to a clippingTypes-compatible object).

@tiennou
Copy link
Member Author

@tiennou tiennou commented Jan 28, 2016

Err, you lost me sir 😆. That comment above belongs to #2177 ;-). This one is just some more fixes over #1977 because I took a bunch of aliases to some remote workplace stuff, and I actually checked how QS handled those (the response is not nicely).

skurfer added a commit that referenced this issue Jan 28, 2016
pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on fc6a0bd Jan 29, 2016

Choose a reason for hiding this comment

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

NSURL just got a whole lot more interesting...!

https://michaellynn.github.io/2015/10/24/apples-bookmarkdata-exposed/

tiennou
Copy link
Member Author

@tiennou tiennou commented on fc6a0bd Jan 30, 2016

Choose a reason for hiding this comment

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

the OS is going to attempt to resolve the resource, even if it doesn't pop up anything to the user when it can't find it.

You just said interesting, amirite ? 😉 I do note that the linked article is about security-scoped bookmarks, so I don't think we're concerned, but now I have doubts...

pjrobertson pushed a commit that referenced this issue Jan 30, 2016
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

3 participants