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

Fixes issue #753 - move to/copy to with items grabbed from Finder #755

Merged
merged 2 commits into from Mar 22, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 14, 2012

The recent change that made the 3rd pane show the items' folder, along with splitObjects not being fully robust caused the crash.

This pull request fixes both these problems.

pjrobertson added 2 commits Mar 14, 2012
…dren' if nothing else is found

Some combined objects, such as those created when grabbing multiple items from Finder with ⌘⎋ do not set the QSObjectComponents, but do set children
@skurfer
Copy link
Member

skurfer commented Mar 14, 2012

Interesting. Allowing splitObjects to return children is a somewhat big change. My gut tells me it’s a change for the better and will give us more flexibility, etc. I’m just trying to think of any potential problems.

What if the parent and children are different types? You could run the action on the parent, but if the action calls splitObjects on it (as many do) and ends up with children instead of components, it could cause problems. That is, actions are defined to work with certain types and typically assume that’s all they’ll be passed. This is only an issue if the components are missing, and I don’t think that’s very common, but the potential is there, no?

From your commit message:

Some combined objects, such as those created when grabbing multiple items from Finder with ⌘⎋ do not set the QSObjectComponents, but do set children

OK, but shouldn’t it use objectByMergingObjects:? That would make some of the changes here unnecessary, but like I said, they may be good things on their own.

@pjrobertson
Copy link
Member Author

pjrobertson commented Mar 14, 2012

OK, but shouldn’t it use objectByMergingObjects:? That would make some of the changes here unnecessary, but like I said, they may be good things on their own.

That was my original plan, but this object is actually created using:

- (id)initWithPasteboard:(NSPasteboard *)pasteboard types:(NSArray *)types {

in QSObject_Pasteboard.m. If you look at it, it's pretty funny how it gets items from the pasteboard, because the pasteboard already defines the '3 objects' as an array. In all honesty, I don't 100% understand how it works... :)

Interesting. Allowing splitObjects to return children is a somewhat big change.

The change I've made isn't that big a change, because looking at the object's children is the last resort. If there's only 1 parent object, then the parent object is returned, as it used to be. If there are object components, then they are returned. If neither of the 2 above are true, then the children are returned (if they exist)

@skurfer
Copy link
Member

skurfer commented Mar 22, 2012

OK, I can’t really think of (or manufacture) a scenario where this causes a problem. Merging.

skurfer added a commit that referenced this pull request Mar 22, 2012
Fixes issue #753 - move to/copy to with items grabbed from Finder
@skurfer skurfer merged commit c0e881e into quicksilver:master Mar 22, 2012
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