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

Save the catalog.plist when catalog entries are removed. Fixes #569 #1108

Merged
merged 2 commits into from Sep 17, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 12, 2012

Ensures that the Catalog.plist file is written and the changes subsequently saved

We really need to work on the amount the catalog is written to disk. It's not OTT (like the triggers is/used to be), but when you create a new catalog item, writeCatalog: in QSLibrarian.m is called about 5 or 6 times.

Go ahead and check it!

I've looked into removing some of the notifs, but the unfortunate thing is - that could break any other area of the code.
I'll leave it for now… :)

Ensures that the Catalog.plist file is written and the changes subsequently saved
@tiennou
Copy link
Member

@tiennou tiennou commented Sep 12, 2012

Heh, I actually have a Sticky note filed on my computer about that (created 11/11/11) that says :

Try putting breakpoints in -writeCatalog, then navigate the catalog pref pane. I got almost a call per click in the window doing this (that was while checking why -writeCatalog wouldn't run). Click, write :-S.
IIRC the same happens for the trigger stuff (except the trigger stuff coalesces its writes).

My take on fixing that would be to rename -writeCatalog: to -writeCatalogNow: and rewrite a -writeCatalog: that's less agressive. See QSTriggerCenter.m for an example. Only issue with that is if you crash while the delay is still running, then no write for ya :-(.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 12, 2012

I was about to implement the exact same thing as you did with triggers, but
thought… "timers and things are messy, there must be a better way", so I
left it for now.

FWIW, the reason this happens (I think) is that there's a binding for the
NSTreeControllers to 'selection' in both the Triggers and Catalog prefs
pane. Whenever the selection changes, then the binding is triggered.
writeCatalog: isn't as bad as writeTriggers:, which is why I left it.
It's still bad though, and needs sorting… one day :)

On 12 September 2012 18:34, Etienne Samson notifications@github.com wrote:

Heh, I actually have a Sticky note filed on my computer about that
(created 11/11/11) that says :

Try putting breakpoints in -writeCatalog, then navigate the catalog pref
pane. I got almost a call per click in the window doing this (that was
while checking why -writeCatalog wouldn't run). Click, write :-S.
IIRC the same happens for the trigger stuff (except the trigger stuff
coalesces its writes).

My take on fixing that would be to rename -writeCatalog: to
-writeCatalogNow: and rewrite a -writeCatalog: that's less agressive. See
QSTriggerCenter.mhttps://github.com/quicksilver/Quicksilver/blob/master/Quicksilver/Code-QuickStepCore/QSTriggerCenter.m#L180for an example. Only issue with that is if you crash while the delay is
still running, then no write for ya :-(.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1108#issuecomment-8503082.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 13, 2012

I could be missing something, but when will the new method get called? I've tried this and it doesn't seem to make a difference in behavior.

The - button in the catalog prefs appears to call remove: on the tree controller. Should it be changed to call the new removeItem: instead (which calls remove: on the tree controller and writes the changes to disk)?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 13, 2012

I should have changed the .xib to call removeItem: in QSCatalogPrefs, but
perhaps that commit didn't push. I'll update it asap (busy evening though)

Yes - the 'new' method gets called when you click the - button.
See previous comments with Etienne about my initial idea to call
writeCatalog whenever the selection changed. Wasteful :)

On 13 September 2012 20:32, Rob McBroom notifications@github.com wrote:

I could be missing something, but when will the new method get called?
I've tried this and it doesn't seem to make a difference in behavior.

The - button in the catalog prefs appears to call remove: on the tree
controller. Should it be changed to call the new removeItem: instead
(which calls remove: on the tree controller and writes the changes to
disk)?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1108#issuecomment-8540196.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 13, 2012

Yes - the 'new' method gets called when you click the - button.

OK, good. That makes more sense.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 16, 2012

Sorry for the delay, a new commit has been added

(oooh, it just popped up with AJAX :) )

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 17, 2012

Yes, that's better. Merging.

skurfer added a commit that referenced this issue Sep 17, 2012
Save the catalog.plist when catalog entries are removed. Fixes #569
@skurfer skurfer merged commit 87e961f into quicksilver:master Sep 17, 2012
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 31, 2012

Working on a change log for B71 and ran across this. It's definitely not fixed, as we discussed the other day. I guess I'll say we attempted to fix it. :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 31, 2012

Well, maybe. I think what doesn't work is removing things from a Web Search list (and possibly other changes to an entry). Removing an entry entirely does seem to trigger saving.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Nov 1, 2012

Well, maybe. I think what doesn't work is removing things from a Web
Search list (and possibly other changes to an entry). Removing an entry
entirely does seem to trigger saving

Good point, I hadn't thought about that case. Maybe some more KVO is in
order :)

On 31 October 2012 19:30, Rob McBroom notifications@github.com wrote:

Well, maybe. I think what doesn't work is removing things from a Web
Search list (and possibly other changes to an entry). Removing an entry
entirely does seem to trigger saving.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1108#issuecomment-9958732.

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 1, 2012

Something that definitely needs to be observed in addition to the above: Checking and unchecking entries.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Nov 1, 2012

Checking and unchecking objects (in the 'contents' tab) doesn't need to be observed. writeCatalog is already being called in QSLibrarian: - (void)setItem:(QSBasicObject *)item isOmitted:(BOOL)omit

If you are just referring to adding/removing entries from the web search plugin, then this is a bug there. It need to call [QSLib writeCatalog:sender]; when the + or - buttons are pressed, or any of the values are changed.

@skurfer
Copy link
Member

@skurfer skurfer commented Nov 1, 2012

I meant checking/unchecking an entire entry or preset, such as Catalog → Plugins → Finder.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Nov 1, 2012

OK, gotcha. Will look into it. Currently downloading Xcode. 6 hours 48
minutes remaining :)

On 1 November 2012 17:21, Rob McBroom notifications@github.com wrote:

I meant checking/unchecking an entire entry or preset, such as Catalog →
Plugins → Finder.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1108#issuecomment-9988140.

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