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

File label #1263

Merged
merged 5 commits into from Dec 9, 2012
Merged

File label #1263

merged 5 commits into from Dec 9, 2012

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Dec 4, 2012

As promised: This should address some of the problems we've found with the way files were being labeled. Mostly by removing the heavy reliance on Spotlight. The problems with that were:

  1. A user could have Spotlight disabled.
  2. There have been complaints about performance when right-arrowing into some folders for the first time. (To test the difference, try right arrowing into /usr/bin with this branch vs. master.)
  3. Files with multiple hard links might have different names, but since they're the same file, they have the same kMDItemDisplayName. (e.g. /usr/bin/2to3 would appear as smtpd.py)

The getNameFromFiles method largely resembles a previous attempt. It was only changed to use Spotlight to address a problem with localized paths. But the more I think about it, the less sense the fix makes. If the path was actually being passed in as /Applications/計算機.app, it would be treated as a non-existant path and the whole method would fall apart. To test the new changes, I've cleared caches and set my system to Japanese and I can locate applications using their filesystem names.

I've also held my nose and added some special cases for Preference Panes and Quicksilver Plug-ins. I hate things like that, but I don't see a way around it in this case.

Finally, I've changed setName: and setLabel: in QSObject to be aware of their relationship to each other. That allows developers to freely call these methods in any order, without doing any comparisons, and without even having to really understand the relationship themselves.

Side note: There's a comment in setName: that says "this should take first line only?". I'd like to either make the suggested change, or remove the comment. I don't have strong feelings either way. Multi-line names don't seem to be hurting anything.

skurfer added 5 commits Dec 4, 2012
Check to see if they're equal any time one of them changes and get rid
of label if it's redundant. This allows us to blindly set either one
without doing comparisons every time.

This also prevents label from being set to an empty string.
For name:
  * trust `lastPathComponent` for the filesystem name instead of asking
Spotlight
For label:
  * use NSFileManager's `displayNameAtPath` for almost everything
  * use non-standard techniques only for types where it's been found to
be an improvement over NSFileManager
  * not getting MDItemRef for every single file really speeds things up
4641413 makes the correct label appear immediately
Passing `nil` to`setLabel:` is safe.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 4, 2012

This will conflict with release won't it (Because of the change I made).

Yes it will. I meant to point that out. (As you can see, I've included your fix here, but at a lower level.) It's easy enough to resolve, and since I'm probably going to be the one doing it, I'm fine with it.

Perhaps this branch should go into release, thoughts?

Honestly, I think all of the current master should go into the next pre-release at this point. Those are my thoughts. :-)

Note the two commits I just added to remove code that's unnecessary now.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 5, 2012

Hey @heavenshell, since you originally reported problems with the old way (which we're sort of returning to), I'd appreciate it if you could try this out. As I said above, I tried switching to Japanese – at least I think it was Japanese ;-) – and everything seemed to work as intended.

Be sure to clear caches before running it. I just do this:

rm -r ~/Library/Caches/*Quicksilver

@heavenshell
Copy link

@heavenshell heavenshell commented Dec 7, 2012

Hey @skurfer
What should I do? and what should I check?
checkout skurfer:fileLabel branch, build it, and then type "calculator.app" to list up "計算機.app"?

I'll look into it in this weekend, please wait.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 7, 2012

What should I do? and what should I check?
checkout skurfer:fileLabel branch, build it, and then type "calculator.app" to list up "計算機.app"?

Yes, just be sure to clear caches before you run the new build to ensure that all the information for files gets rescanned. I would never expect you to see "計算機.app", but when you type "calc", it should match and show "計算機 Calculator.app" with the characters you typed underlined.

Thanks for testing!

@heavenshell
Copy link

@heavenshell heavenshell commented Dec 9, 2012

Hi, @skurfer

I've tried fileLabel branch.
I typed "calc" and showed "計算機.app Calculator.app" which you excepted.
Here is screenshot

Very nice! I like it!

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 9, 2012

Thanks for testing that!

Phew, nice work Rob :)
I'll just check it with a European language and then we're good to go.
Maybe in release? ;-)

On 9 December 2012 10:08, heavenshell notifications@github.com wrote:

Hi, @skurfer https://github.com/skurfer

I've tried fileLabel branch.
I typed "calc" and showed "計算機.app Calculator.app" which you excepted.
Here is screenshot http://gyazo.com/28b5a6f808096037fd2322d79f93ac03

Very nice! I like it!


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

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 9, 2012

Good, I can search perfectly in the localised language and for the name of the file:

Calculette / Calculator (French)

And no more mditemref! :D

pjrobertson added a commit that referenced this issue Dec 9, 2012
@pjrobertson pjrobertson merged commit 2d9639d into quicksilver:master Dec 9, 2012
@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 10, 2012

Maybe in release? ;-)

I sort feel like everything on master should be merged into release before the next preview. :-)

@skurfer skurfer mentioned this pull request Aug 14, 2013
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