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

Recent documents changes #819

Merged
merged 3 commits into from Apr 24, 2012
Merged

Conversation

HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 14, 2012

Updated code for showing recent documents when right arrowing into applications:

  • Removed pre-10.6 code
  • Moved special code for QuickTime Player to its own plugin. See quicksilver/QuickTimePlayer-qsplugin#1
  • Added synchronization of preferences, so latest values will be available. Note: The application still has to synchronized its recent documents for them to show up in QS. That's unavoidable. But at least the additional level of delay that was caused by the caching on QS's side is removed with this.

HenningJ added 2 commits Apr 14, 2012
…plications:

* Removed pre-10.6 code
* Added synchronization of preferences, so latest values will be available
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 14, 2012

So you finally got round to making the QT plugin? ;-)

I've actually already made a pull request that removes the 10.5 code, but it doesn't add the 3rd thing you have. See #756
It also removes the catalog presets.

Would you be able to take the extra stuff from that pull request and put it in here?

@HenningJ
Copy link
Contributor Author

@HenningJ HenningJ commented Apr 14, 2012

So you finally got round to making the QT plugin? ;-)

What I actually set out to do was the third point. I was annoyed that my recent documents didn't show up until a reboot (or restart of QS maybe?). The QT plugin was just a by-product.

I've actually already made a pull request that removes the 10.5 code, but it doesn't add the 3rd thing you have. See #756
It also removes the catalog presets.

Woups...I forgot about that one. But now that I look at it, it doesn't seem to contain any code changes. Just .plist and localization stuff. If that's correct, there shouldnt be any conflicts.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 14, 2012

Oh yeah, you're right - no changes to the code there. Maybe I did it, but
didn't commit it. OK I'll look through this when I get some time :)

On 14 April 2012 20:17, Henning Jungkurth <
reply@reply.github.com

wrote:

So you finally got round to making the QT plugin? ;-)

What I actually set out to do was the third point. I was annoyed that my
recent documents didn't show up until a reboot (or restart of QS maybe?).
The QT plugin was just a by-product.

I've actually already made a pull request that removes the 10.5 code,
but it doesn't add the 3rd thing you have. See #756
It also removes the catalog presets.

Woups...I forgot about that one. But now that I look at it, it doesn't
seem to contain any code changes. Just .plist and localization stuff. If
that's correct, there shouldnt be any conflicts.


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

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 16, 2012

I'm not sure if it was previously like this, but it seems to be a bit slow when right arrowing into some applications (as well as ones where it doesn't work, e.g. Activity Monitor)

Perhaps you could put the CFPreferencesSynchronize call in an if() statement, and if it returns false (there's no .plist I'm guessing) then you could just return.

It'd probably also be worth doing the same for if (recentDocuments106 = nil)

Other than that, it's good to have this finally fixed!

@HenningJ
Copy link
Contributor Author

@HenningJ HenningJ commented Apr 17, 2012

I'm not sure if it was previously like this, but it seems to be a bit slow when right arrowing into some applications (as well as ones where it doesn't work, e.g. Activity Monitor)

I didn't notice, but I don't doubt it. But the caching stuff is in place for a reason. Every time you do caching, there's a trade of between having more recent data and speed. In this case I went for having more recent data (because it always annoyed me not to have the recent data).

Perhaps you could put the CFPreferencesSynchronize call in an if() statement, and if it returns false (there's no .plist I'm guessing) then you could just return.

Nope, CFPreferencesSynchronize Always return YES, even if the particular .plist doesn't exist.

It'd probably also be worth doing the same for if (recentDocuments106 = nil)

Nope, isn't worth it. If there are no recent documents, it doesn't do much anyways. It skips the whole for-loop.
I ran some test, run both the version with early return and the current version 100,000 times and the difference was still barely measurable. And most times the one without early return was even a little bit faster.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 24, 2012

Are you sure 100,000 times is enough? :-)

Merged. Now we need to get the QT plugin out :)

pjrobertson added a commit that referenced this issue Apr 24, 2012
@pjrobertson pjrobertson merged commit 34d1c10 into quicksilver:master Apr 24, 2012
@HenningJ
Copy link
Contributor Author

@HenningJ HenningJ commented Apr 25, 2012

Are you sure 100,000 times is enough? :-)

Well...the number had to be large enough so the execution takes some time. You can't notice much of a difference on fractions of milliseconds ;-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 25, 2012

Sorry, my Aussie sarcasm didn't come through much on that post ;-)
I'll keep looking at the lag to see if I can think of anything to fix it

On 25 April 2012 08:32, Henning Jungkurth <
reply@reply.github.com

wrote:

Are you sure 100,000 times is enough? :-)

Well...the number had to be large enough so the execution takes some time.
You can't notice much of a difference on fractions of milliseconds ;-)


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

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