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

SIX small things #817

Merged
merged 14 commits into from Apr 24, 2012
Merged

SIX small things #817

merged 14 commits into from Apr 24, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 14, 2012

The first will improve debugging when plugins aren't loading (can be very frustrating!)

The second might not do anything, when the code is compiled, does the compiler optimise when variables are created? If so this, does nothing, but I like to think it'll make QS a tiny bit faster :)

pjrobertson added 4 commits Apr 14, 2012
Very handy for when you're developing and you can't for the life of you figure out why an old QS plugin won't load
There's a small chance that moving these means they won't be created for *every* object that's ranked, but I think the compiler does that anyway, so probably not...
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 15, 2012

I have added two more things to this commit.

The first turns off the 'Running from a new location' dialogue for debug builds
The second fixes something that's recently started annoying me more and more. How to reproduce:

  • Go to the catalog prefs and select a file & folder scanner entry (easiest to reproduce with this)
  • refresh the item by clicking the refresh icon
  • go to the actual folder on the disk and delete/move an item so that the folder's contents reduces
  • click the refresh icon again. Note that the catalog entry sidebar updates, but the little number next to the tick doesn't

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 15, 2012

Two more things...!

The first stops the exception of #687, and fixes the issue, which is nice.

The second improves the efficiency of the purgeImagesAndChildrenOlderThan method. Basically it was wasteful creating the tempArray, and I've added some autoreleasepools. This reduces the time it takes to perform the method from about 20µs to about 10µs. Not much difference, but for over 100,000 users, invoking this method say... 60 times a day (every time the QS interface closes) that's 60 seconds of computing power a day, across the world - saved! :)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 16, 2012

The first will improve debugging when plugins aren't loading (can be very frustrating!)

When did this happen?

The first turns off the 'Running from a new location' dialogue for debug builds

Nice. That was really annoying.

Update the catalog entry item 'count' when the refresh button is pressed

Cool...if it actually would work. :-p
To test this, I changed the scanning depth of a File&Folder scanner entry. And it only worked some of the time. And I think I know why. In your change, you update the display immediately after the scanning (in a new thread) was started. Now, if the scan finishes quickly enough, the new number is available when your update command is reached. But if it's a larger folder, it isn't guaranteed to be finished by then. Therefore, the old number is still around, so it stays the same.
Instead, you should update the display when the scan in the new thread is finished. The QSCatalogPrefPane.m is already listening to the QSCatalogEntryIndexed with the catalogIndexed: method. So maybe you should check why it's no correctly updated there.

The second improves the efficiency of the purgeImagesAndChildrenOlderThan method. Basically it was wasteful creating the tempArray, and I've added some autoreleasepools.

I agree with the tempArray thing, but I'm not so sure about the autoreleasepools. What exactly do they release? Just the immuatable copy of [iconLoadedArray allObjects], right? How big are those arrays? Creating an autoreleasepool just for that seems to be overkill. And if they actually help, wouldn't it be enough to have one autoreleasepool, surrounding both loops? Should be almost as good, and creating extra autoreleasepools takes time as well, right?
I don't really know, but maybe you do.

Another thing: When testing this, I got an "This is an old version of Quicksilver - You have previously used a newer version..." popup. Maybe you can deactivate that for debug builds as well. QS also started to download the latest version once it started. Another thing that doesn't make sense for debug builds. :-/

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

The first will improve debugging when plugins aren't loading (can be very frustrating!)

When did this happen?

When I was building the Email Support plugin for example. There are hundreds of reasons why the bundle won't necessarily load (almost always for us developers). In that case, it was because the executable in the MacOS dir had a different name to that defined in the Info.plist. This just made it easy for me to find the problem.
In the past, I've had problems because I've forgotten to copy a framework into the 'framework' folder of the plugin, meaning the bundle couldn't load etc. etc.

What exactly do they release? Just the immuatable copy of [iconLoadedArray allObjects], right? ...

Those arrays are pretty big, if you've had QS running for quite a while they can have 1000+ QSObjects in them.
The main thing I think the autorelease pools release (but you've made me think otherwise now!) is the thisObject objects. I'm assuming that in fast enumeration this takes place behind the scenes: thisObject = [anArray objectAtIndex:i], so you have lots of autoreleased objects. If it's not the case, then I'm happy to remove the autoreleasepools. I think wrapping both loops in 1 autoreleasepool would be better though (using 2 was a hang up from when I was trying an alternative method of doing this)

I'll make a few changes now and see what you think :)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 16, 2012

I'm assuming that in fast enumeration this takes place behind the scenes: thisObject = [anArray objectAtIndex:i], so you have lots of autoreleased objects.

I always assumed fast enumeration would take care of releasing those objects for you. But I don't really know.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

I always assumed fast enumeration would take care of releasing those
objects for you. But I don't really know.

I've trawled the web, but can't find anything. Seems like neither of us
really know... I'll ask on the cocoa dev list.

On 16 April 2012 10:34, Henning Jungkurth <
reply@reply.github.com

wrote:

I'm assuming that in fast enumeration this takes place behind the
scenes: thisObject = [anArray objectAtIndex:i], so you have lots of
autoreleased objects.

I always assumed fast enumeration would take care of releasing those
objects for you. But I don't really know.


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

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

@HenningJ - you were right about the fast enumeration. Here's what someone said on the list:

There are no temporary objects created during fast enumeration, you just get a reference to one of the objects in the thing you are enumerating.

I'll remove the autoreleasepools :)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 16, 2012

you were right about the fast enumeration

Yay. I like being right. :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

OK Henning, I've made the changes.

I've made it so that QS doesn't download updates in the background for debug builds, but you can still update (to test that things work properly) if you click the 'now' button in the prefs.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 16, 2012

Nice. Looks pretty good.

One more thing: Could you add comments to the #ifndef DEBUG sections, why they are deactivated. Something like "This is an old version" popup deactived for DEBUG builds. Otherwise they might be overlooked because of all the #ifdef DEBUG sections.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

sure thing :)

On 16 April 2012 11:57, Henning Jungkurth <
reply@reply.github.com

wrote:

Nice. Looks pretty good.

One more thing: Could you add comments to the #ifndef DEBUG sections,
why they are deactivated. Something like "This is an old version" popup deactived for DEBUG builds. Otherwise they might be overlooked because of
all the #ifdef DEBUG sections.


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

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

Done @HenningJ :)

On 16 April 2012 11:58, Patrick Robertson robertson.patrick@gmail.comwrote:

sure thing :)

On 16 April 2012 11:57, Henning Jungkurth <
reply@reply.github.com

wrote:

Nice. Looks pretty good.

One more thing: Could you add comments to the #ifndef DEBUG sections,
why they are deactivated. Something like "This is an old version" popup deactived for DEBUG builds. Otherwise they might be overlooked because of
all the #ifdef DEBUG sections.


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

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 16, 2012

Nice. :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 16, 2012

Note that the catalog entry sidebar updates, but the little number next to the tick doesn’t

I think it was only about 3 minutes after I merged your preferences clean-up that I thought about that stupid number not updating. :-) Glad it’s getting addressed here.

@skurfer
Copy link
Member

@skurfer skurfer commented Apr 16, 2012

Have you tried to do a Release build with this? For me, it chokes on the stuff in QSController.m.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 16, 2012

Have you tried to do a Release build with this? For me, it chokes on the stuff in QSController.m.

Right you are. Apparently variable declaration can't be the first thing inside a case block. Unless you wrap that block in curly braces like this:

case QSApplicationUpgradedLaunch: {
...
    break;
}

Then it works. Weird.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Apr 16, 2012

Sorry guys, I should really check over my pull requests a bit more
thoroughly... :-)

I've added another commit that fixes this

On 16 April 2012 20:42, Henning Jungkurth <
reply@reply.github.com

wrote:

Have you tried to do a Release build with this? For me, it chokes on the
stuff in QSController.m.

Right you are. Apparently variable declaration can't be the first thing
inside a case block. Unless you wrap that block in curly braces like this:

case QSApplicationUpgradedLaunch: {
...
break;
}

Then it works. Weird.


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

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

@skurfer skurfer commented Apr 24, 2012

Merged.

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