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

Reload catalog tables on the main thread. Fixes #893 #1313

Merged
merged 2 commits into from Jan 17, 2013

Conversation

pjrobertson
Copy link
Member

See #893 for the details of the bug.

To reproduce the bug (before this is merged), open the 'Custom' catalog prefs pane, invoke QS and press ⌘R repeatedly 'til everything disappears.

@skurfer
Copy link
Member

skurfer commented Jan 7, 2013

Do you want to wait for #1276 so you can use [self reloadData]?

@pjrobertson pjrobertson closed this Jan 7, 2013
@pjrobertson
Copy link
Member Author

Yeah, I'll add the commits on to that pull and close this one

On 7 January 2013 19:05, Rob McBroom notifications@github.com wrote:

Do you want to wait for #1276https://github.com/quicksilver/Quicksilver/issues/1276so you can use [self
reloadData]?


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

@pjrobertson pjrobertson reopened this Jan 7, 2013
@pjrobertson
Copy link
Member Author

Actually, the two can't be combined exactly, since they do slightly different things. I'll leave this open, and it can be merged independently of #1276 (unless you see a better way round it)

@skurfer
Copy link
Member

skurfer commented Jan 8, 2013

Tried to reproduce the bug after this was merged (to make sure it was fixed) and hitting ⌘R repeatedly seems to beach ball QS indefinitely. I also found another problem through pure dumb luck. If you're in Custom and try to drag an entry to reorder them, it beach balls then too.

@pjrobertson
Copy link
Member Author

I think I came across this problem once, but wasn't sure how reproducible
it was. I can't reproduce it now though. Try a clean build.

My thoughts were that when calling a block on the main thread from the
main thread, it'd cause problems. The solutions I can think of are:

  • To dispatch the block to a background thread, which then dispatches to the main thread
  • To use the [NSThread isMainThread] method.

Personally, I'd be inclined to go with 1, but what are your thoughts? If
you can give me a reliable way to reproduce I'll implement one of the above
(probably 1)

On 8 January 2013 14:33, Rob McBroom notifications@github.com wrote:

Tried to reproduce the bug after this was merged (to make sure it was
fixed) and hitting ⌘R repeatedly seems to beach ball QS indefinitely. I
also found another problem through pure dumb luck. If you're in Custom and
try to drag an entry to reorder them, it beach balls then too.

@skurfer
Copy link
Member

skurfer commented Jan 8, 2013

Dragging to reorder catalog entries does it for me every time. Closed Xcode, switched branches, opened Xcode, did a clean, then ran it. The drag appears to succeed (based on the position of things after a relaunch) but the UI becomes unresponsive.

I thought it might have been the Synonyms I had in my catalog (since this branch has no support for them), but removing them made no difference.

@skurfer
Copy link
Member

skurfer commented Jan 8, 2013

Oh, and as for your questions, I'm not sure. If you're on the main thread and want something else to run on the main thread, wouldn't you just run it? Or do we not necessarily know which thread this will be called from?

@pjrobertson
Copy link
Member Author

Hmm... I still can't reproduce.
Can you give any further hints as to how you're opening the catalog prefs,
sidebar open etc.?

Or do we not necessarily know which thread this will be called from?

Seems like it, hence the 2nd option to do if([NSThread isMainThread]){}. I
think the GCD way is better as it's asynchronous so shouldn't lock up the
main thread (as much)

On 8 January 2013 15:01, Rob McBroom notifications@github.com wrote:

Oh, and as for your questions, I'm not sure. If you're on the main thread
and want something else to run on the main thread, wouldn't you just run
it? Or do we not necessarily know which thread this will be called from?


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

@skurfer
Copy link
Member

skurfer commented Jan 8, 2013

Cleaned and tried again. Now simply opening the Catalog prefs seems to be enough to lock things up. When I quit via Activity Monitor, the debugger pauses here:

Catalog Hang

Based on this, and some NSLogging I did, the method is called in response to the notification, but the block is never executed.

@pjrobertson
Copy link
Member Author

Yesh, that's what I thought. Try the extra commit I've just pushed. It's what I said in the first bullet point above

@skurfer
Copy link
Member

skurfer commented Jan 8, 2013

Looks like a similar goal here: http://stackoverflow.com/a/12850706/491598

My completely uneducated opinion is that we should test isMainThread as he does (and as you suggested) instead. Going to another thread only to come back seems like a lot of overhead, and I know we're dealing with modern systems, but can we always be guaranteed that more than one thread can exist?

@pjrobertson
Copy link
Member Author

Reminder: use this function once this is merged, and release is merged into master

@pjrobertson
Copy link
Member Author

If I try and rebase this and #1276 against master, I get a merge conflict. Can we just merge these and I'll do the change afterwards?

skurfer added a commit that referenced this pull request Jan 17, 2013
Reload catalog tables on the main thread. Fixes #893
@skurfer skurfer merged commit 270734b into quicksilver:master Jan 17, 2013
@pjrobertson pjrobertson deleted the catalogScan branch January 17, 2013 21:04
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

2 participants