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

simplify and improve splitObjects #422

Merged
merged 1 commit into from Jul 19, 2011
Merged

simplify and improve splitObjects #422

merged 1 commit into from Jul 19, 2011

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jul 18, 2011

The splitObjects method was doing a lot of work to get some details of QSObjects and return them in an array of new QSObjects.

  1. It was only preserving the name and types for the object. (No metadata, details, label. etc.) Metadata in particular could be important to validate actions, etc.
  2. This work is unnecessary. All of the details of the component objects have already been stored in their entirety by the objectByMergingObjects method. All we need to do is return it instead of creating a bunch of new (incomplete) objects.
  3. This change will make it much easier for plug-ins to get objects passed in via the comma trick.

I only found three calls to splitObjects (in the application and all known plug-ins) and this should be compatible with all of them.

@pjrobertson
Copy link
Member

pjrobertson commented Jul 18, 2011

I only found three calls to splitObjects (in the application and all known plug-ins) and this should be compatible with all of them.

I'm assuming you've tested the changes of course :) but have you stuck break points in the 3 places and explicitly run the corresponding code? :)

Also, I stuck the if([self count] == 1) bit in, but I was never quite sure how reliable count was. It looks pretty robust to me (line 499) but it's worth just making sure.

All looks good otherwise :)

@skurfer
Copy link
Member Author

skurfer commented Jul 19, 2011

The actions were easy enough to test, but the 3rd call was a bit fuzzier. I’ll set a breakpoint as you suggest just to make sure that code is getting called and not running into problems.

@pjrobertson
Copy link
Member

pjrobertson commented Jul 19, 2011

Sounds good enough to me :)

I'll pull it and have a play :)

@skurfer
Copy link
Member Author

skurfer commented Jul 19, 2011

I set a breakpoint and I don’t think the 3rd call will ever get triggered, so its hard to test. :)

The check for [[dict objectForKey:kActionSplitPluralArguments] boolValue] will never be TRUE because the kActionSplitPluralArguments constant isn’t set by anything ever (from what I can see). So lines 301 - 308 of QSAction.m are never executed. This goes back to the initial commit, so who knows. Maybe Alcor was working on a way to run an action over and over instead of handing the combined objects to the action and letting it do the splitting?

So, I don’t know what you want to do with that information. :)

@pjrobertson
Copy link
Member

pjrobertson commented Jul 19, 2011

Interesting. Who know what Alcor was working on...!

I'd say it's fine as is then. I've still yet to test it...

On 19 July 2011 14:49, skurfer <
reply@reply.github.com>wrote:

I set a breakpoint and I dont think the 3rd call will ever get triggered,
so its hard to test. :)

The check for [[dict objectForKey:kActionSplitPluralArguments] boolValue]
will never be TRUE because the kActionSplitPluralArguments constant isnt
set by anything ever (from what I can see). So lines 301 - 308 of
QSAction.m are never executed. This goes back to the initial commit, so
who knows. Maybe Alcor was working on a way to run an action over and over
instead of handing the combined objects to the action and letting it do the
splitting?

So, I dont know what you want to do with that information. :)

Reply to this email directly or view it on GitHub:
#422 (comment)

@pjrobertson
Copy link
Member

pjrobertson commented Jul 19, 2011

No problems. Works fine with my 1Password plugin after updating it :)

pjrobertson added a commit that referenced this pull request Jul 19, 2011
simplify and improve splitObjects
@pjrobertson pjrobertson merged commit af10f76 into quicksilver:master Jul 19, 2011
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