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

avoid storing useless mnemonics #446

Merged
merged 4 commits into from Jul 29, 2011
Merged

avoid storing useless mnemonics #446

merged 4 commits into from Jul 29, 2011

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jul 25, 2011

This causes the saveMnemonic method to do nothing unless it’s operating on a single object. Previously, typing something like “xc,ab” to launch Xcode and Address Book (convoluted example) would store "/Developer/Applications/Xcode.app /Applications/Address Book.app” for the abbreviation “ab”.

  1. Obviously, “ab” is not an abbreviation for both apps together.
  2. There’s nothing in the catalog that represents both applications together, so storing them here will never match anything. Over time, these entries will build up and theoretically slow down the matching process.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 25, 2011

Did you look at the possibility of assigning ab,xc to Address book, Xcode... etc. Is it possible? I've added a line comment to the commit :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 25, 2011

The fact that it stores it under the abbreviation “ab” and not “xc,ab” leads me to believe that matchedString only contains the most recent abbreviation. We don’t have access to anything before the most recent ‘,’. I wouldn’t want to store them combined anyway. Better to keep them separate, so if it learns that “ab” is Address Book, I can use the abbreviation in combination with anything, not just Xcode.

You would think that if a user hits ‘,’ they’re satisfied that the current thing is what they want (same as if they had hit return). So I think we could achieve what you’re talking about if we can get saveMnemonic to trigger when ‘,’ is pressed. It would also have to be modified to only grab [[self objectValue] lastObject] to account for multiple comma hits.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 25, 2011

OK I see (the business of matchedString)

These changes make sense then.
Just a couple of small things: curly brackets and if statements... :/ and
also - the if(verbose) can probably be uncommented then put inside #ifdef
DEBUG. I'd say it's something worth having. It always amazes me how good the
verbose stuff is in QS (hold ⌥ when launching QS after building in Debug)

So just one last thing... if I go with xc,ab now I don't get any
mnemonics added? Whereas previously I'd get 'ab' added for Xcode and Address
Book. Would another solution not be to:

NSArray *splitObjects = [[self objectValue] splitObjects];
[[QSMnemonics sharedInstance] addObjectMnemonic:mnemonicKey forID:[splitObjects
objectAtIndex:[splitObjects count]]] identifier]];

That way we'd at least get ab added for address book (the xc would be
ignored)

On 25 July 2011 20:36, skurfer <
reply@reply.github.com>wrote:

The fact that it stores it under the abbreviation “ab” and not “xc,ab”
leads me to believe that matchedString only contains the most recent
abbreviation. We don’t have access to anything before the most recent ‘,’. I
wouldn’t want to store them combined anyway. Better to keep them separate,
so if it learns that “ab” is Address Book, I can use the abbreviation in
combination with anything, not just Xcode.

You would think that if a user hits ‘,’ they’re satisfied that the current
thing is what they want (same as if they had hit return). So I think we
could achieve what you’re talking about if we can get saveMnemonic to
trigger when ‘,’ is pressed. It would also have to be modified to only grab
[[self objectValue] lastObject] to account for multiple comma hits.

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

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 25, 2011

I think I’ve got it.

Now I can do “ab,cons” to run Address Book and Console and individual mnemonics will be added for both.

I’ll test it out for while.

So just one last thing... if I go with xc,ab now I don't get any mnemonics added? Whereas previously I'd get 'ab' added for Xcode and Address Book.

It was added, but useless. :)

[splitObjects objectAtIndex:[splitObjects count]]

That should probably be count - 1, but I scanned the docs for a better way and NSArray has a lastObject method. Much easier. :)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 25, 2011

That should probably be count - 1, but I scanned the docs for a better way and NSArray has a lastObject method. Much easier. :)

Duh lastObject - stupid me :P

I think if you go your way of performing the mnemonic method when you hit the ',' you won't need any of the fancy stuff I suggested - so it sounds like you're going the right way :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 26, 2011

Alright, give this a whirl. Here’s a little python script that’ll help you check the results. Finding new unused ones might be hard, so I’ve just been shutting down QS and removing some. Add the abbreviations your testing to the lookfor list.

import os
import plistlib

plist_path = os.getenv('HOME', '') + '/Library/Application Support/Quicksilver/Mnemonics.plist'

mnemonics = plistlib.readPlist(plist_path)

lookfor = ['cons', 'ab']

for qso in lookfor:
    try:
        matched_objects = mnemonics['abbreviation'][qso]
        print qso
        for mo in matched_objects:
            print '   ', mo, '(used %d times)' % mnemonics['implied'][mo][qso]
    except KeyError:
        print 'no match for "%s"' % qso

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 26, 2011

Oh, and I’m working on a Python script to clean the noise out of Mnemonics.plist. Maybe we can put it up on QSApp.com after the next release (if it includes this change).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

Oh, and Im working on a Python script to clean the noise out of
Mnemonics.plist. Maybe we can put it up on QSApp.com after the next
release (if it includes this change).

Cool, I was going to suggest something like that :)
Maybe even something we could include in QS that would run on startup -
would probably increase startup time by a couple of seconds, but would it be
worth it?

I actually trashed my mnemonics.plist as I had a quick look through it and
there IS so much noise!

On 26 July 2011 04:04, skurfer <
reply@reply.github.com>wrote:

Oh, and Im working on a Python script to clean the noise out of
Mnemonics.plist. Maybe we can put it up on QSApp.com after the next
release (if it includes this change).

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

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Jul 26, 2011

Maybe even something we could include in QS that would run on startup - would
probably increase startup time by a couple of seconds, but would it be worth it?

It would have to run only once, right? Not on every startup?
So actually it should run when updating to ß61.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

It would have to run only once, right? Not on every startup?
So actually it should run when updating to 61.

Add it to the update process for the next couple of release cycles then
remove it - is that what you're thinking? Doesn't sound too bad to me...
We could have a popup saying "We're just doing a little house keeping..." or
something. Maybe add a button for 'make backup' (of mnemonics.plist) then
'OK' for 'run the script'

On 26 July 2011 09:10, HenningJ <
reply@reply.github.com>wrote:

Maybe even something we could include in QS that would run on startup -
would
probably increase startup time by a couple of seconds, but would it be
worth it?

It would have to run only once, right? Not on every startup?
So actually it should run when updating to 61.

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

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Jul 26, 2011

Add it to the update process for the next couple of release cycles then
remove it - is that what you're thinking?

Exactly.

We could have a popup saying "We're just doing a little house keeping..."
or something. Maybe add a button for 'make backup' (of mnemonics.plist)
then 'OK' for 'run the script'

I'd rather not tell the user at all, because he shouldn't need to care. Just make a backup, run the script and leave the user in blissful ignorance. :-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

blissful ignorance.

Good. I like bliss :) So just "cp mnemonics.plist mnemoncis.plist.bak" type
thing (but with Cocoa of course)
All sounds like the right way to go. Think how we'll be tidying up thousands
of Macs around the world :D

I'll check this out and have a play now.

On 26 July 2011 09:45, HenningJ <
reply@reply.github.com>wrote:

Add it to the update process for the next couple of release cycles then
remove it - is that what you're thinking?

Exactly.

We could have a popup saying "We're just doing a little house keeping..."
or something. Maybe add a button for 'make backup' (of mnemonics.plist)
then 'OK' for 'run the script'

I'd rather not tell the user at all, because he shouldn't need to care.
Just make a backup, run the script and leave the user in blissful ignorance.
:-)

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

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

Code looks good. Testing now.
One small thing: I still think it's nice to keep the if (verbose) even in the #ifdef DEBUG code. When we're running QS in debug (most of the time for me) we still don't really want to clog up the console. At the moment, if we want to print loads of crap to the console we can hold ⌥ on startup ;)

Just my 2¢ though.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

Also, it'd probably be more useful to have the NSLog as:

NSLog(@"Added Mnemonic: %@ for object: %@", [self matchedString], [mnemonicValue identifier]);

or something similar.

One other thing I've just checked: When I deleted my mnemonics.plist the other day, I saw I had loads of matchedStrings as @"" (i.e. nothing)
Should we do a check on the string to see if it's 'nothing'. This is most notable when you pick the default action for an object. QSInterfaceController.m:597 saves the aSelector mnemonic, which is @""" if you just hit � straight away.

...although looking at QSMnemonics.m:114 it looks kinda funny there. If no object is set, it sets it to @""
Would it be worth adding in that method:

if (!key || [key isEqualToString:@""]) ...

^^ Just found another place where this is silly. Just look in your .plist for how many <key></key> bits there are. Every time you right arrow into a folder (or out for that matter) the mnemonic @"" gets saved for that folder! Surely we shouldn't have this?! :o

Finally finally: If you type ab, then hit the mnemonic ab gets saved twice. I don't think that's a problem, but thought I'd bring it up incase you have any ideas of any implications.

Otherwise, works well for me :)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Jul 26, 2011

Every time you right arrow into a folder (or out for that matter) the mnemonic @""
gets saved for that folder! Surely we shouldn't have this?!

Does this happen only happen when you right arrow into a folder you browsed (right-arrowed) to? Or also when you right arrow into a folder you got to by searching for it? In the later case it should store the mnemonic you used to find the folder. In the first case you're right, it shouldn't store anything at all. Right? :-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

In the later case it should store the mnemonic you used to find the
folder. In the first case you're right, it shouldn't store anything at all.
Right? :-)

Yep, what you said. It's QSMnemonics.m:115 that seems to be the silly line:

if (!mnem) mnem = @"";

Why?!

On 26 July 2011 10:18, HenningJ <
reply@reply.github.com>wrote:

Every time you right arrow into a folder (or out for that matter) the
mnemonic @""
gets saved for that folder! Surely we shouldn't have this?!

Does this happen only happen when you right arrow into a folder you browsed
(right-arrowed) to? Or also when you right arrow into a folder you got to by
searching for it? In the later case it should store the mnemonic you used to
find the folder. In the first case you're right, it shouldn't store anything
at all. Right? :-)

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

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 26, 2011

Ugh. I never should have turned over this rock. :)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

Ugh. I never should have turned over this rock. :)

Hehe. Didn't ya know Quicksilver wasn't that simple! :P
This kinda stuff is actually really great IMO. We've kinda got to the point where we're not just fixing bugs and crashes, but we're actually improving things for users and making Quicksilver better (in a way other than just 'stopping crashes')
If this speeds QS up, and improves its already awesome matching algorithm, then who can complain?!

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 26, 2011

One other thing I've just checked: When I deleted my mnemonics.plist the other day, I saw I had loads of matchedStrings as @"" (i.e. nothing)

Yeah, I noticed the empty entry too. I’m not sure how they get created, but it doesn’t seem like they could possibly do anything useful. I’ll probably need to catch them in both the saveMnemonic and addObjectMnemonic methods, since the latter is called directly from one other place (in QSObjectActions.m).

Every time you right arrow into a folder (or out for that matter) the mnemonic @"" gets saved for that folder!

I think this from saveMnemonic addresses that case

if (![self sourceArray]) { // don't add abbreviation if in a subsearch
    [[QSMnemonics sharedInstance] addAbbrevMnemonic:mnemonicKey forID:[mnemonicValue identifier] relativeToID:nil immediately:NO];
}

I think maybe we need to expand that to make some other statements conditional, or perhaps explicitly checking for “” will take care of it.

If you type ab, then hit the mnemonic ab gets saved twice. I don't think that's a problem, but thought I'd bring it up incase you have any ideas of any implications.

What do you mean by that? It’s counted as if it’s used twice? I don’t see that. The count only goes up by one each time I use it.

If I hit , then it gets counted twice because it stays in the first pane. Which leads me to wonder: Was it always like that? Seems like the main part of the pane should be empty after hitting comma and sending the object down to the “tray”. Should I change that too? I’ll see what B54 does (if it’ll still run on Lion).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 26, 2011

I agree with most of the stuff / it looks fine.

If I hit , then it gets counted twice because it stays in the first
pane. Which leads me to wonder: Was it always like that?

That's what I meant. I'm pretty sure it's always been like that. (Just
checked with ß54 - yep. Ugh the icon's ugly, and Bezel is so damn slow :P)
Is it worth it clearing the dSelector when , is pressed? If so, it's have to
be done right so it didn't wipe out the results list (incase you wanted to
go , ↓ , ↓ etc.)
Do you think it's worth the extra work/change? I'm not so sure myself
(should never have brought it up! :P)

On 26 July 2011 19:48, skurfer <
reply@reply.github.com>wrote:

One other thing I've just checked: When I deleted my mnemonics.plist the
other day, I saw I had loads of matchedStrings as @"" (i.e. nothing)

Yeah, I noticed the empty entry too. I’m not sure how they get created, but
it doesn’t seem like they could possibly do anything useful. I’ll probably
need to catch them in both the saveMnemonic and addObjectMnemonic
methods, since the latter is called directly from one other place (in
QSObjectActions.m).

Every time you right arrow into a folder (or out for that matter) the
mnemonic @"" gets saved for that folder!

I think this from saveMnemonic addresses that case

if (![self sourceArray]) { // don't add abbreviation if in a subsearch
[[QSMnemonics sharedInstance] addAbbrevMnemonic:mnemonicKey
forID:[mnemonicValue identifier] relativeToID:nil immediately:NO];
}

I think maybe we need to expand that to make some other statements
conditional, or perhaps explicitly checking for “” will take care of it.

If you type ab, then hit the mnemonic ab gets saved twice. I don't
think that's a problem, but thought I'd bring it up incase you have any
ideas of any implications.

What do you mean by that? It’s counted as if it’s used twice? I don’t see
that. The count only goes up by one each time I use it.

If I hit , then it gets counted twice because it stays in the first pane.
Which leads me to wonder: Was it always like that? Seems like the main part
of the pane should be empty after hitting comma and sending the object down
to the “tray”. Should I change that too? I’ll see what B54 does (if it’ll
still run on Lion).

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

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 26, 2011

I think I have it preventing the storing of empty abbreviations now, though I’m finding it hard to reliably add empties (using a version without my changes). Here’s how I’m checking for new ones:

import os
import plistlib
plist_path = os.getenv('HOME', '') + '/Library/Application Support/Quicksilver/Mnemonics.plist'
mnemonics = plistlib.readPlist(plist_path)
empty_count = 0
for (qso, abbs) in mnemonics['implied'].items():
    if '' in abbs:
        empty_count += abbs['']
print "empties:", empty_count

If so, it's have to be done right so it didn't wipe out the results list (incase you wanted to go , ↓ , ↓ etc.)
Do you think it's worth the extra work/change?

That’s true. It should probably stay as it is because often, you aren’t starting a new search after each comma, but selecting multiple items from the current results.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 29, 2011

All looks good to me. I've just done the manual thing of opening TextMate and searching for

Can't see that any more are being added :)

The only thing I can think of... are the @"" strings useful for ranking the contents of folders by score? Do the ones you right arrow into more often show up first?

... nope, just tested with a version without these changes and I don't think you can actually sort by score when in 'browsing' mode anyway.

Merged!

pjrobertson added a commit that referenced this issue Jul 29, 2011
avoid storing useless mnemonics
@pjrobertson pjrobertson merged commit e8484fd into quicksilver:master Jul 29, 2011
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