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

Update the search object and the actions in a more user friendly way, fixes #856 #857

Merged
merged 4 commits into from May 7, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 30, 2012

  • I've tidied up the code changes I made at 910accd (removed the tempNewObject and tempCurrentObject objects)
  • Added the changes I made in 910accd to the setObjectValue method (to reduce the number of 'searchObjectChanged' messages sent)
  • Added the previous and current objects to the QSSearchObjectChanged userInfo dictionary (so that methods receiving the notif can see the different)
  • Made the actions reload only if the previous and new object's types are different (if the types are the same then the actions will be the same)
  • Altered the updateActions and updateActionsNow methods so that the latter clears the action pane. This means that you won't get a 'blinking' effect for 0.1s (the time between updateActions and updateActionsNow being called) while the 2nd pane is empty

pjrobertson added 2 commits Apr 30, 2012
* I've tidied up the code changes I made at 910accd (removed the tempNewObject and tempCurrentObject objects)
* Added the changes I made in 910accd to the `setObjectValue` method (to reduce the number of 'searchObjectChanged' messages sent)
* Added the previous and current objects to the QSSearchObjectChanged `userInfo` dictionary (so that methods receiving the notif can see the different)
* Made the actions reload only if the previous and new object's types are different (if the types are the same then the actions will be the same)
* Altered the `updateActions` and `updateActionsNow` methods so that the latter clears the action pane. This means that you won't get a 'blinking' effect for 0.1s (the time between `updateActions` and `updateActionsNow` being called) while the 2nd pane is empty
@skurfer
Copy link
Member

@skurfer skurfer commented Apr 30, 2012

I love this. Seems like the kind of thing that should have been done long ago. Slight problem though (that I didn’t think of myself until just now)…

if the types are the same then the actions will be the same

If that were true, validActionsForDirectObject:indirectObject: wouldn’t exist. :-)

I can confirm that this breaks some things. For instance, the SSH action doesn’t appear for Windows remote hosts. If I select a Windows host, then immediately call up a Linux host, the SSH action isn’t available. I’m not sure of the best way to handle this while still preserving the efficiency this change brings. I’ll look over your changes and give it some thought tonight.


Thought of an easier-to-reproduce example: Bring up a regular URL in the first pane, then call up a Web Search URL. It should switch from “Open URL” to “Search For…”, but it doesn’t, and you can’t even get to “Search For…” manually because it’s not on the list.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 30, 2012

Hmm... you're right, as per usual - good catch.

I’m not sure of the best way to handle this while still preserving the
efficiency this change brings

The 'perceived' performance boost will still be there since I moved the
clearObjectValue call from from the updateActions to the
updateActionsNow method (stops the flickering). But of course, it doesn't
give any actual performance boots.
Short of calculating all the actions for the new object and comparing
(which defeats the purpose), I can't think of anything.

Bring up a regular URL in the first pane, then call up a Web Search URL

Or we just make a new type for web search URLs... ;-)

On 30 April 2012 21:36, Rob McBroom <
reply@reply.github.com

wrote:

I love this. Seems like the kind of thing that should have been done long
ago. Slight problem though (that I didn’t think of myself until just now)…

if the types are the same then the actions will be the same

If that were true, validActionsForDirectObject:indirectObject: wouldn’t
exist. :-)

I can confirm that this breaks some things. For instance, the SSH action
doesn’t appear for Windows remote hosts. If I select a Windows host, then
immediately call up a Linux host, the SSH action isn’t available. I’m not
sure of the best way to handle this while still preserving the efficiency
this change brings. I’ll look over your changes and give it some thought
tonight.


Thought of an easier-to-reproduce example: Bring up a regular URL in the
first pane, then call up a Web Search URL. It should switch from “Open URL”
to “Search For…”, but it doesn’t, and you can’t even get to “Search For…”
manually because it’s not on the list.


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

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 30, 2012

OK I've removed the test. I've looked through it all a lot, and have even
discovered that - if the new object is exactly the same - a new NSArray of
actions is made.

The next best thing I can come up with is to compare this new array with
the current actions array. If they're both the same, then don't bother
updating the actions pane.
As you said, refreshing the 2nd pane is probably quite costly, so this will
stop it from happening. Now the best optimised case we'd hoped for, but
still better

On 30 April 2012 21:45, Patrick Robertson robertson.patrick@gmail.comwrote:

Hmm... you're right, as per usual - good catch.

I’m not sure of the best way to handle this while still preserving the
efficiency this change brings

The 'perceived' performance boost will still be there since I moved the
clearObjectValue call from from the updateActions to the
updateActionsNow method (stops the flickering). But of course, it doesn't
give any actual performance boots.
Short of calculating all the actions for the new object and comparing
(which defeats the purpose), I can't think of anything.

Bring up a regular URL in the first pane, then call up a Web Search URL

Or we just make a new type for web search URLs... ;-)

On 30 April 2012 21:36, Rob McBroom <
reply@reply.github.com

wrote:

I love this. Seems like the kind of thing that should have been done long
ago. Slight problem though (that I didn’t think of myself until just now)…

if the types are the same then the actions will be the same

If that were true, validActionsForDirectObject:indirectObject: wouldn’t
exist. :-)

I can confirm that this breaks some things. For instance, the SSH action
doesn’t appear for Windows remote hosts. If I select a Windows host, then
immediately call up a Linux host, the SSH action isn’t available. I’m not
sure of the best way to handle this while still preserving the efficiency
this change brings. I’ll look over your changes and give it some thought
tonight.


Thought of an easier-to-reproduce example: Bring up a regular URL in the
first pane, then call up a Web Search URL. It should switch from “Open URL”
to “Search For…”, but it doesn’t, and you can’t even get to “Search For…”
manually because it’s not on the list.


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

@skurfer
Copy link
Member

@skurfer skurfer commented May 1, 2012

Yeah, I’ve thought about it and the list of actions is really an object-specific thing, so if the object changes, the list should be reexamined. The latest changes seem reasonable. I’ll test them out.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 4, 2012

P.S. Rob don't forget this one for ß68 :)

skurfer added a commit that referenced this issue May 7, 2012
Update the search object and the actions in a more user friendly way, fixes #856
@skurfer skurfer merged commit 8ad9997 into quicksilver:master May 7, 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