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

Reduce the finder proxy cache time to 0.5s. Fixes #1719 #1748

Merged
merged 10 commits into from Mar 12, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 16, 2014

This just does what Rob suggested in #1719 - that is, only change the proxy cache time for the Finder proxy object, not all objects

Aside: I still do think there are ground for reducing it from 3s for all proxies though - maybe to 2s? Computers are much faster since the time was set to 3s (you guessed it, it was 'code cleanup' just over 6 years ago!)

  • update: the one that was most annoying me was the Latest Download proxy, when I was wanting to move things to the trash in quick succession. I have updated that proxy to 0.5s cache time

@skurfer
Copy link
Member

skurfer commented Jan 16, 2014

A default of 2.0s for other proxies seems safe to me. Especially as the number of proxies that don’t provide a specific value is dropping all the time.

@pjrobertson
Copy link
Member Author

pjrobertson commented Jan 16, 2014

OK, default time reduced to 2.0s

About:

we should probably generalize a lot of that code instead of repeating it a third time in one file.) You want to add that to #1748? If you don’t have time, I can.

I'm not 100% sure as to what you're saying there, and it sounds like you know better. Are you OK with adding here - this branch is on the QS repo so writable by you :)

@skurfer
Copy link
Member

skurfer commented Jan 16, 2014

Are you OK with adding here - this branch is on the QS repo so writable by you

Oh, yeah. We started doing it that way for just such an occasion. I’ll add a commit in a bit.

@skurfer
Copy link
Member

skurfer commented Jan 16, 2014

OK, default time reduced to 2.0s

Could you push the commit, so I’ll have it before I start making changes? 😃

skurfer added 5 commits Jan 16, 2014
If there isn’t one, use the default.
* It only gets used in a class method, so it doesn’t need to be an
iVar. It’s declared locally.
* Made it static
* Sync the timeout for failure with the default proxy cache time.
They’re not the same thing, but they should both be updated as hardware
gets faster.
* only get information for the current application once
* alter the order when getting types: types from plist, fallback to
string, types from provider → types from provider, types from plist,
fallback to string
* eliminate the poorly named passthroughProxyInfo class method
@skurfer
Copy link
Member

skurfer commented Jan 17, 2014

OK, see what you think of this approach. We have to keep the currentSelection class method because ⌘G calls it. Not a big deal. I just created some static local variables to hang onto information for the current application. “Resolve” is always the first thing called, so their values should always be correct. (I wasn’t able to break it.)

One thing that I think needs scrutiny is the way I changed type behavior. Before, it was checking for types in this order:

  1. The kQSProxyTypes entry in the plug-in’s plist
  2. If there was no plist, fall back to the string type
  3. Ask the provider for a list of types

I believe that the provider (if it exists) is the most reliable source of type information, so now it’s:

  1. Ask the provider for a list of types
  2. The kQSProxyTypes entry in the plug-in’s plist
  3. If there was no plist, fall back to the string type, which I changed to QSTextType so it’ll automatically become a UTI when you get up the courage to change that. (Do it! 💊)

Now that I look, this doesn’t quite match the precedence in [QSProxyObject proxyTypes], but maybe that’s wrong, too. (If there’s a method defined to check the types in real-time, wouldn’t that always be preferable to a static list?) If you agree, I’ll change it.

…instead of returning the path to the source file(s)

fixes #1729
@skurfer
Copy link
Member

skurfer commented Feb 16, 2014

Added a semi-related commit.

allows us to use instance variables to keep track of things instead of
static locals
@skurfer
Copy link
Member

skurfer commented Feb 28, 2014

I appreciate the vote of confidence on IRC, but I’ll let someone else have a look at the last commit before merging.

@tiennou
Copy link
Member

tiennou commented Feb 28, 2014

The singleton's good ;-), but I don't like all that state (currentAppSelectionProxyInfo & currentAppSelectionProvider).

Here's what I would change :

  • Make a -currentSelectionBundleIdentifier that does the fetch-current-application-identifier step (lines 123-129).
  • Don't store the proxy info in an ivar : if you need it, fetch the identifier before and ask QSReg.
  • Make a new -(id <QSProxyObjectProvider>)currentSelectionProvider that resolves the actual instance or return nil if there's no specific provider.
  • Use that provider in -currentSelection, and switch to an early return — the else is unneeded. Also check the returns in that method : the last @"No selection" return can never execute.

Also, I think I just discovered one difference between getting the selection through the service and the older, removed method : I don't remember the application order changing in the application switcher. Right now if I create a trigger akin to Current Selection > Find With... > Google, QS becomes my "last used" application. Which means I can't easily select text in the first, invoke trigger (changes to browser), and come back with a single ⌘⇥. Aaaanyways ;-).

@skurfer
Copy link
Member

skurfer commented Feb 28, 2014

but I don't like all that state

It’s just there to avoid repeating the work over and over. Repeating -[QSReg tableNamed:] probably isn’t a big deal, but -[QSReg getClassInstance:] seems a little on the expensive side.

Also, I think I just discovered one difference between getting the selection through the service and the older, removed method

Lots of people have noticed that (see #1351), but I think you’re the first to pinpoint what change caused it. 😃

@tiennou
Copy link
Member

tiennou commented Feb 28, 2014

getClassInstance: caches its things, so it's not that expensive ;-). I'm just concerned that that "state" could cause inconsistencies between the call to typesForProxy: and resolveProxyObject:. Add threading to the mix and it doesn't look fun to debug.

@skurfer
Copy link
Member

skurfer commented Mar 3, 2014

OK, I think you’re right about getClassInstance:. It’ll have to do the more expensive stuff once no matter what, but should only be a dictionary look up from then on. But I still need some convincing.

I'm just concerned that that "state" could cause inconsistencies between the call to typesForProxy: and resolveProxyObject:.

In the hypothetical scenario where the application changes in the middle of these calls, if typesForProxy: ever got called first, I can see why that would be a problem, but

  1. In my testing, currentSelection was always called before the other two methods, so state should be set correctly.
  2. Looking up the current application in real-time from all three methods would cause the exact same inconsistencies. It would just take longer to get there. 😉

In other words, if the active application changed between calls, don’t we want it to “lie” and give us information for the same app we were just looking at?

Add threading to the mix and it doesn't look fun to debug.

I don’t have a good answer for that one.

@skurfer
Copy link
Member

skurfer commented Mar 7, 2014

Silence = merge? 😉

@pjrobertson
Copy link
Member Author

pjrobertson commented Mar 11, 2014

Hmmm.... reading up on Singletons, I still don't think we're doing much wrong. We're thread safe, and we are using iVars (and not just making a nice place to store methods).
Having said that, is there any reason why it needs to be a singleton. Why couldn't we just do:

[[QSGlobalSelectionProvider alloc] init] currentSelection] every time we need it?

@skurfer
Copy link
Member

skurfer commented Mar 11, 2014

Having said that, is there any reason why it needs to be a singleton.

Good question. As long as all the proxy object calls go to the same object, it should be fine. The proxy object will always refer to getClassInstance:, so for all practical purposes, it is a singleton unless you hit ⌘G. But that only needs the return from currentSelection and not any of the other proxy stuff.

Is it worth changing though? There’s still going to be one global selection object in memory at all times once you use it. Possibly two (for a millisecond) upon ⌘G if we change it as you suggest.

I’ll do whatever to get this merged. 😃

@pjrobertson
Copy link
Member Author

pjrobertson commented Mar 12, 2014

Meh, OK then. I hope you're happy ;-)

pjrobertson added a commit that referenced this pull request Mar 12, 2014
Reduce the finder proxy cache time to 0.5s. Fixes #1719
@pjrobertson pjrobertson merged commit ab10772 into master Mar 12, 2014
@pjrobertson pjrobertson deleted the finderproxytime branch Mar 12, 2014
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

3 participants