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

Reset the search string when clearing the object view #1772

Merged
merged 3 commits into from Feb 11, 2014
Merged

Reset the search string when clearing the object view #1772

merged 3 commits into from Feb 11, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 8, 2014

Fixes #1760

Not much to it really.

pjrobertson added 2 commits Feb 8, 2014
CGSPrivate (and hence fancy transitions) don't play nicely with ARC. Fixes #1731
@pjrobertson
Copy link
Member Author

pjrobertson commented Feb 8, 2014

Meh, this pull was so small I added a commit that fixes #1731. I mentioned just removing the transitions all together, nobody seemed bothered, so I went ahead and did it :)

@skurfer
Copy link
Member

skurfer commented Feb 8, 2014

I still get the EXC_BAD_ACCESS in flare:. I didn’t see it unless I removed ~/Library/Application Support/Quicksilver as well as the prefs. I mentioned a couple of ways to fix it in #1731, but I’m not sure which is best. Assigning windowNumber to a variable first thing seems less expensive than @synchronized, but perhaps less reliable.

@pjrobertson
Copy link
Member Author

pjrobertson commented Feb 9, 2014

I can't reproduce that crash. But if it's another crash with CGSPrivate, can't we just remove it?

I don't understand how the crash can actually come about, because how can self be released?!
You could try something like this:

NSWindow __strong *strongSelf = self;

And then using strongSelf in all the calls. I'm not sure how that would work.

@pjrobertson
Copy link
Member Author

pjrobertson commented Feb 9, 2014

P.S. How the hell do you put your preferences back? Replacing com.blacktree.quicksilver.plist doesn't work :(

@skurfer
Copy link
Member

skurfer commented Feb 9, 2014

I can't reproduce that crash. But if it's another crash with CGSPrivate, can't we just remove it?

It’s in QSWindow. Well, technically, NSWindow (Effects).

I don't understand how the crash can actually come about, because how can self be released?!

I can see it. If the retain count goes to 0, does the system care if the apparently unused object is in the middle of running some internal method?

You could try something like this:

NSWindow __strong *strongSelf = self;

Yeah, that would probably work. Pretty similar to something I saw in Reachability. To be safe, I suppose we should do that for every method in NSWindow (Effects). I’ll handle it since I can reproduce the crash.

How the hell do you put your preferences back?

Yeah, 10.9 caches user defaults pretty aggressively. I had to add this to my script that backs up and restores settings. After you put the file back in place…

defaults read "$HOME/Library/Preferences/com.blacktree.Quicksilver.plist" > /dev/null

I’ll send you the whole script if you want it.

@pjrobertson
Copy link
Member Author

pjrobertson commented Feb 10, 2014

It’s in QSWindow. Well, technically, NSWindow (Effects).

Yeah I can see where you mean. The crash is still technically with the call to CGSGetWindowTransform() right?
What I don’t get is how assigning an NSInteger ([self windowNumber]) to an iVar can fix things - an NSInteger isn’t an object, so can’t be retained/released.

Also, I only see flare: getting called in two places in the code, and neither refer to the setup assistant, so I’m confused as to how you’re getting the call (one is the splash screen, the other trigger display windows - I can’t get a crash on either).

The fact that we’re having to do anything at all in flare: to keep self around is pretty dodgy anyway I think. If you can find out what/where is calling flare: then we can see if that method isn’t retaining theWindow correctly.

I’ll send you the whole script if you want it.

Yes please. Your extra line worked though, thanks.

On 9 Chwef 2014, at 22:41, Rob McBroom notifications@github.com wrote:

I can't reproduce that crash. But if it's another crash with CGSPrivate, can't we just remove it?

It’s in QSWindow. Well, technically, NSWindow (Effects).

I don't understand how the crash can actually come about, because how can self be released?!

I can see it. If the retain count goes to 0, does the system care if the apparently unused object is in the middle of running some internal method?

You could try something like this:

NSWindow __strong *strongSelf = self;

Yeah, that would probably work. Pretty similar to something I saw in Reachability. To be safe, I suppose we should do that for every method in NSWindow (Effects). I’ll handle it since I can reproduce the crash.

How the hell do you put your preferences back?

Yeah, 10.9 caches user defaults pretty aggressively. I had to add this to my script that backs up and restores settings. After you put the file back in place…

defaults read "$HOME/Library/Preferences/com.blacktree.Quicksilver.plist" > /dev/null
I’ll send you the whole script if you want it.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

skurfer commented Feb 10, 2014

The crash is still technically with the call to CGSGetWindowTransform() right?

Don’t think so.

What I don’t get is how assigning an NSInteger ([self windowNumber]) to an iVar can fix things - an NSInteger isn’t an object, so can’t be retained/released.

It’s the window (self) that gets overreleased.

You don’t need an iVar for the number. Just a local variable in that method. (You’ll see that [self windowNumber] gets called 3 times, but only the third is ever a problem. So if you capture the value early enough, it seems to work. But I doubt that either of us thinks we can rely on that working in the long term.

Also, I only see flare: getting called in two places in the code, and neither refer to the setup assistant, so I’m confused as to how you’re getting the call (one is the splash screen, the other trigger display windows - I can’t get a crash on either).

If you set a breakpoint, you can see it getting called twice on launch. I know it’s being called on the same object twice and I suspect those calls are simultaneous. Some NSLog() statements support that belief, but they also prevent the crash. (I suspect the timing has to be just right and thee logs introduce enough of a delay to hide the problem.) To save you the time…

startQuicksilver: → setupSplash → hideSplash: → flare:
startQuicksilver: → hideSplash: → flare:

I’m trying to figure out of those are both necessary, but what I’m more interested in is the intentional backgrounding at the end of setupSplash. Shouldn’t it be on the main thread since it does UI stuff?

@skurfer
Copy link
Member

skurfer commented Feb 10, 2014

Just changed setupSplash to use dispatch_get_main_queue() and the crash goes away, but I’ll wait for confirmation from you before declaring that as “the fix” since you wrote it and probably understand better what it was meant to do.

@skurfer
Copy link
Member

skurfer commented Feb 10, 2014

After reviewing Apple’s docs, I’ve added another commit here. You don’t always have to deal with NSWindow on the main thread, but it should always be the same thread. We were accessing it simultaneously from two. I’ve confirmed several times that the change prevents the crash on my end.

I’m OK will all the other changes here, so if you’re happy with the last commit, merge away.

@pjrobertson
Copy link
Member Author

pjrobertson commented Feb 11, 2014

That looks better. I'm not sure why I did what I did. It was perhaps a typo. Should everything in setupSplash be run on the main thread (using QSGCDMainSync() say)

Anyways, your change looks find (if current_queue() == main thread)
Feel free t merge/release a dev pre

@skurfer
Copy link
Member

skurfer commented Feb 11, 2014

Should everything in setupSplash be run on the main thread (using QSGCDMainSync() say)

We know it’s fine with the current code. Maybe we could do some things to make it harder for future changes to put things on different threads, but not an immediate priority IMO.

skurfer added a commit that referenced this pull request Feb 11, 2014
Reset the search string when clearing the object view
@skurfer skurfer merged commit 51ef4bb into master Feb 11, 2014
@skurfer skurfer deleted the 1760 branch Feb 11, 2014
skurfer added a commit that referenced this pull request Feb 11, 2014
skurfer added a commit that referenced this pull request Mar 19, 2014
skurfer added a commit that referenced this pull request Apr 14, 2014
skurfer added a commit that referenced this pull request May 13, 2014
skurfer added a commit that referenced this pull request May 30, 2014
skurfer added a commit that referenced this pull request Aug 7, 2014
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.

Switching search scope doesn’t clear previous search
2 participants