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 object fixes (Fixes #475, fixes #237) #793

Merged
merged 8 commits into from Apr 24, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 6, 2012

Improve method for getting package names (e.g. application or bundle names) and guessing the type of groups of files (e.g. 'current application' proxy)

pjrobertson added 3 commits Apr 6, 2012
Not an NSMutable dictionary as previously used. Defined in QSCatalogSource.h
… of files

* Fixes quicksilver#475 - see http://developer.apple.com/library/mac/#qa/qa1544/_index.html for the methods used
* Fixes quicksilver#237 - Finder has type "FNDR" so it was messing up application proxies
* Various other small improvements
@skurfer
Copy link
Member

@skurfer skurfer commented Apr 6, 2012

I had to clear caches to see the fix, but it looks good now.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 6, 2012

Yeah, that'll get done for a new version of QS anyway so no need to worry about it.

Nice to have proper names eh?!

@@ -728,7 +728,8 @@ - (id)initWithArray:(NSArray *)paths {
// return an already-created object if it exists
QSObject *existingObject = [QSObject objectWithIdentifier:thisIdentifier];
if (existingObject) {
return existingObject;
// autorelease since this method will have been called from [QSObject alloc...] increasing its retain count
return [existingObject autorelease];
Copy link
Contributor

@HenningJ HenningJ Apr 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? Why autorelease? existingObjectis created with [QSObject objectWithIdentifier:], which should already return an autoreleased object. Why would you autorelease it again, without retaining it first?

Copy link
Member Author

@pjrobertson pjrobertson Apr 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken... (yay memory management, I may well be wrong!)

This method is an init method so it'll get called in the following manner:

QSObject *myNewObject = [[QSObject alloc] initWithArray:myArray];

now the alloc in the above code increases myNewObject's retain count by one. If an existingObject is returned, then it already has a retain count of 1 (or more I guess) so it is autoreleased so as to not increase the retain count from the original alloc message.

I'm happy to go through and debug the [object retainCount] to see what's what. I've made myself think this might not actually be right now...

Copy link
Contributor

@HenningJ HenningJ Apr 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it some more, I'm not sure what's the correct thing here anymore. After calling

QSObject *myNewObject = [[QSObject alloc] initWithArray:myArray];

you should have myNewObject with retain count 1, which you need to release yourself (because it's an init... method, it should not return an autoreleased object).

Inside the initWithArray: method, an autoreleased object is created with the line:

QSObject *existingObject = [QSObject objectWithIdentifier:thisIdentifier];

(because that is using a convenience method, not starting with init...)

Now you want to autorelease existingObject object again, decreasing the retaincount once more?
I think you might actually need to retain existingObject at that point, taking ownership of that object.

But by now I'm confused, so I'm not sure about anything anymore. Maybe I'm just confused.

Copy link
Member

@skurfer skurfer Apr 9, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the rule about owning an object if you call a method with “init” in the name can really only be trusted if it’s a class from Apple. There’s nothing forcing the rest of the world to follow that convention.

In this case, it’s a method we control, so if it isn’t following the convention, we should probably fix it. But isn’t ARC just around the corner? :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 8, 2012

Minor issue (in that very few users will ever encounter it), but have you tried browsing ~/Library/Application Support/Quicksilver/PlugIns with this change? I’m not sure how or why, but it used to show the name you’d see in the plug-ins preferences instead of the filename.

I’ve also noticed that Xcode projects now look like Quicksilver.xcodeproj Xcode Project. Any idea why it shows both the extension and the kind? (Maybe I’m the only one seeing this. See #433. I haven’t tried another account.)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 8, 2012

Minor issue (in that very few users will ever encounter it), but have you tried browsing ~/Library/Application Support/Quicksilver/PlugIns with this change? I’m not sure how or why, but it used to show the name you’d see in the plug-ins preferences instead of the filename.

That's an inconsistency in the way different bundles are setting their name. In the Info.plist files, QSPlugins set CFBundleName for their name, whereas most bundles set a CFBundleDisplayName key which is what finder reads (and why Finder shows com.blacktree.pluginName.qsplugin)
Now if we reverted back to looking at CFBundleName as opposed to CFBundleDisplayName, printer packages have this set as PrinterProxy whereas the CFBundleDisplayName key isn't set (which, with my changes makes the actual file name be read much like for the QSPlugins)

The options we have:

  • Keep it as is and only display the CFBundleDisplayName. We're pretty much acting exactly like finder now
  • Read Both CFBundleName and CFBundleDisplayName (incase one doesn't exist) and hard code the method to avoid any names called PrinterProxy to fix the original bug. I've gone for this here, but it may be pretty dodgy. See what you think

I’ve also noticed that Xcode projects now look like Quicksilver.xcodeproj Xcode Project. Any idea why it shows both the extension and the kind? (Maybe I’m the only one seeing this. See #433. I haven’t tried another account.)

This has been a 'feature' of Quicksilver ever since day one apparently. It wasn't previously being set since no name was ever being found for the package. I've altered the method now to reflect this, so it won't show Xcode Project

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 9, 2012

Read Both CFBundleName and CFBundleDisplayName (incase one doesn't exist) and hard code the method to avoid any names called PrinterProxy to fix the original bug. I've gone for this here, but it may be pretty dodgy.

I’d hate to have something like that hard-coded just to avoid a cosmetic issue most people will never see. “PrinterProxy” is unlikely to be the only thing like that we’d want to avoid. I was just curious why it was happening, but you’ve explained it.

So should we start adding (or switching over to) display name in plug-ins? We’d have to update the code to check both values, but that should only be in one place (QSPlugIn -name).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 9, 2012

So should we start adding (or switching over to) display name in
plug-ins? Wed have to update the code to check both values, but that
should only be in one place (QSPlugIn -name).

Technically, yes, but it'll be a pain! I wasn't sure about hard coding, I
can remove it and keep it as it was (still fixing the 'Xcode Project' thing)

On 9 April 2012 19:52, Rob McBroom <
reply@reply.github.com

wrote:

Read Both CFBundleName and CFBundleDisplayName (incase one doesn't
exist) and hard code the method to avoid any names called PrinterProxy to
fix the original bug. I've gone for this here, but it may be pretty dodgy.

Id hate to have something like that hard-coded just to avoid a cosmetic
issue most people will never see. PrinterProxy is unlikely to be the only
thing like that wed want to avoid. I was just curious why it was
happening, but youve explained it.

So should we start adding (or switching over to) display name in plug-ins?
Wed have to update the code to check both values, but that should only be
in one place (QSPlugIn -name).


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

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

I've removed the hard-coding of the printer proxy name.

Now, QSPlugins won't have a nice name. Their "Bundle Display Name" keys should be set in the future

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 16, 2012

I think there’s a } missing around line 818 in the last commit. :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

Hmm... more haste, less speed Patrick!

Thanks Rob, pushed :)

On 16 April 2012 19:41, Rob McBroom <
reply@reply.github.com

wrote:

I think there’s a } missing around line 818 in the last commit. :-)


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

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 24, 2012

Hmm... more haste, less speed Patrick!

This just means the rule against merging our own pulls is doing its job. But let’s hope a future git bisect never puts us between these last two commits. :-)

FYI, I just noticed that this seems to fix my long-standing problem of most apps appearing in the first pane with the .app prefix.

Merging.

skurfer added a commit that referenced this issue Apr 24, 2012
@skurfer skurfer merged commit facc782 into quicksilver:master Apr 24, 2012
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 24, 2012

This just means the rule against merging our own pulls is doing its job. But let’s hope a future git bisect never puts us between these last two commits. :-)

Well. the compiler will never build the 'wrong' commit, so it should be ok :)

FYI, I just noticed that this seems to fix my long-standing problem of most apps appearing in the first pane with the .app prefix.

Good news :) When I released the mini and window interfaces a few days ago, I put in the 'bundle display name' strings so we can start moving over to using those whenever I guess

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 24, 2012

When I released the mini and window interfaces a few days ago, I put in the 'bundle display name' strings so we can start moving over to using those whenever I guess

I did the same with the iTunes version bump. I’ll just start adding them every time I touch a plug-in.

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.

3 participants