resolve proxy objects before sending them to actions #652

Merged
merged 1 commit into from Feb 3, 2012

Projects

None yet

2 participants

@skurfer
Member
skurfer commented Jan 22, 2012

This should ensure that actions are always given a resolved object instead of a proxy (so every single action doesn’t have to test and resolve them). See #621 for more discussion.

Changing this method should catch all of the scenarios (interface controller, triggers, etc.).

One example of something that wouldn’t work before this change: Random Track → Add to Playlist… → Some Playlist

@pjrobertson
Member

... so every single action doesn’t have to test and resolve them

Could you elaborate? Don't quite understand this.

This method definitely seems more robust than the last one :)

@skurfer
Member
skurfer commented Jan 23, 2012

... so every single action doesn’t have to test and resolve them

Could you elaborate? Don't quite understand this.

This is sort of covered in #621, but I can (hopefully) clarify. Without this change, proxy objects are sent to actions as proxy objects. So getting certain things out of the object won’t work. For instance, objectForMeta: won’t give you what you expect because the metadata is in the object the proxy points to, not the proxy itself. The same is true for more fundamental things like identifier. An action can get around that by testing for proxies and resolving them if needed, but a plug-in developer would have to 1) know that and 2) add the extra code to every action. (Or they could just add the check to actions where it could be a problem, but they’d have to know a lot about the way the core app works to figure that out.)

If you and I didn’t even know this was a potential problem, what chance do others have? :-)

So with this change, there’s less to know and less to code for plug-in devs, and going along with that, I feel like it makes everything more reliable. Actions can just be written with the assumption that they’ll always be passed a “real” object.

@pjrobertson pjrobertson merged commit 133e116 into quicksilver:master Feb 3, 2012
@pjrobertson
Member

P.S. can I just say...

If you and I didn’t even know this was a potential problem, what chance do others have? :-)

YES! There was one thing that's been annoying me about the 'Paste as Plain Text' action, in that it never worked with the 'Clipboard Contents' proxy (because it would try and paste some unresolved object). I spent quite a few days trying to figure it out. Guess what, I didn't even know this was a potential problem, but now it works :D

@skurfer
Member
skurfer commented Feb 4, 2012

Cool! I expect there’ll be a lot of things that should have always worked, but didn’t for unexplained reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment