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

better proxy caching #2322

Merged
merged 3 commits into from Mar 15, 2017
Merged

better proxy caching #2322

merged 3 commits into from Mar 15, 2017

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Feb 23, 2017

I think I’ve finally figured out how to get rid of the timer.

There are generally two places we want to resolve a proxy object: to show it in the interface, and to use it in a command. Once resolved, we want to hold onto that value until one of two things happens: The command is finished using the object, or the interface was dismissed.

Common scenarios (and their effect on the proxy):

Select and do nothing.

  1. Select a proxy in the interface (resolve)
  2. Dismiss the interface without doing anything (release)

Run a trigger.

  1. Run a command that uses a proxy object (resolve)
  2. The trigger’s command finishes (release)

Those are fine on their own, but an even more common scenario combines them in a weird order.

  1. Select a proxy in the interface (resolve)
  2. Hit Return to run the command, which dismisses the interface (release)
  3. Run the command (resolve)
  4. The command finishes (release)

The problem with that was, between 1 and 3, the object could change so you could run the action on something you didn’t want.

The previous solution was to just hold onto the resolved object for 2 more seconds, but that broke workflows that used the same proxy in rapid succession.

Now, when the interface is dismissed, instead of blindly clearing the proxy cache, it will check to see why the interface was dismissed. If it’s because a command was run, it leaves the cache in place knowing that it will get cleared anyway when the command finishes. Otherwise, the cache is cleared immediately.

The only thing I’m not 100% comfortable with is the change to when the notifications are posted. It used to be more limited it seems, but I don’t know if that made a difference in practice.

skurfer added 3 commits Feb 23, 2017
in a more official way so they’re easier to discover
when posting QSInterfaceDeactivatedNotification
use notifications instead of a timer to determine when the resolved object cache should be cleared
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 24, 2017

Hmmm yeah so you've basically moved the notification to take place earlier on - I have no idea of the effects this will have on how things work. I'm guessing you're running with this build now, places I think to look out for are probably:

abbreviations, icon clearing/caches, proxies (duh!), triggers

@skurfer
Copy link
Member Author

@skurfer skurfer commented Feb 24, 2017

Hmmm yeah so you've basically moved the notification to take place earlier on

Actually, I think it’s later (since it used to be in the willHideMainWindow method) but not meaningfully so. I’m more concerned that it might fire at times when it didn’t previously since it’s no longer inside that if.

But the only place watching for that notification before this was the trigger center which re-scoped when focus changed. I’ve tested some of my scoped triggers and all appears to be well.

And I just touched a few files in ~/Downloads, then selected and trashed them via QS as fast as I could. Always seemed to get the right target.

@skurfer skurfer merged commit af6cb6d into master Mar 15, 2017
0 of 2 checks passed
@skurfer skurfer mentioned this pull request Apr 17, 2017
@tiennou tiennou deleted the proxytimer branch Nov 28, 2017
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