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

Respect the 'run tasks in background' setting for triggers as well #1501

Merged
merged 1 commit into from Jun 6, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 21, 2013

From #1477 I realised QS doesn't currently run triggers in the background, ever. That's not so good :/

I've fixed this now. It would have been nicer to tie it into the existing code that checks for this setting, but that's related to the interface, and this is related to triggers (which don't use the interface, so they'll have to be independent

@skurfer
Copy link
Member

@skurfer skurfer commented May 24, 2013

Weird problem I ran into. My trigger for Current Web Page (Safari) ⇥ Paste as Plain Text started pasting stale values. (The URL wasn't updating as I changed tabs.) Using the proxy manually shows the correct URL.

What's especially weird is that I can't reproduce it on this branch alone, but if I merge this with several other pending branches, it starts showing the incorrect behavior. Haven't had time to investigate further.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 25, 2013

Strange, I guess running on a background thread could theoretically cause problems resolving the proxy, but I don't see how… unless ScriptingBridge has to be run on the main thread?

I'll let you figure it out, since it might be a problem somewhere else

On 25 Mai 2013, at 02:12, Rob McBroom notifications@github.com wrote:

Weird problem I ran into. My trigger for Current Web Page (Safari) ⇥ Paste as Plain Text started pasting stale values. (The URL wasn't updating as I changed tabs.) Using the proxy manually shows the correct URL.

What's especially weird is that I can't reproduce it on this branch alone, but if I merge this with several other pending branches, it starts showing the incorrect behavior. Haven't had time to investigate further.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented May 28, 2013

I'll let you figure it out, since it might be a problem somewhere else

Yeah, like I said, this alone won't do it, but git bisect leads here. I might have to merge this with other branches one at a time to figure it out.

@skurfer
Copy link
Member

@skurfer skurfer commented May 28, 2013

It happens when you mix this with tempMeta (from #1218). I guess the “perform selector after delay” stuff doesn’t work when called in the background, because expireCache: never runs.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 2, 2013

OK, nice detective work.

That would make sense, since (I'm assuming) this line is called on the main thread, and as such it works, but by doing stuff in the background it messes it up.

From the docs about that method:

This method sets up a timer to perform the aSelector message on the current thread’s run loop

Have you tried running it on the main thread using performSelectorOnMainThread:withObject:waitUntilDone:?

Personally, I still think the tempMeta branch has a few problems... I've gone ahead and tried to get the ball rolling there, but I think this problem you mention should be addressed there rather than here, as the only way to fix the problem here is to not run triggers on a background thread - which defeats the purpose of the pull :P

2 similar comments
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 2, 2013

OK, nice detective work.

That would make sense, since (I'm assuming) this line is called on the main thread, and as such it works, but by doing stuff in the background it messes it up.

From the docs about that method:

This method sets up a timer to perform the aSelector message on the current thread’s run loop

Have you tried running it on the main thread using performSelectorOnMainThread:withObject:waitUntilDone:?

Personally, I still think the tempMeta branch has a few problems... I've gone ahead and tried to get the ball rolling there, but I think this problem you mention should be addressed there rather than here, as the only way to fix the problem here is to not run triggers on a background thread - which defeats the purpose of the pull :P

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 2, 2013

OK, nice detective work.

That would make sense, since (I'm assuming) this line is called on the main thread, and as such it works, but by doing stuff in the background it messes it up.

From the docs about that method:

This method sets up a timer to perform the aSelector message on the current thread’s run loop

Have you tried running it on the main thread using performSelectorOnMainThread:withObject:waitUntilDone:?

Personally, I still think the tempMeta branch has a few problems... I've gone ahead and tried to get the ball rolling there, but I think this problem you mention should be addressed there rather than here, as the only way to fix the problem here is to not run triggers on a background thread - which defeats the purpose of the pull :P

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 6, 2013

Yeah, I agree that what you've done here is correct and doesn't need to be changed to work-around something that isn't merged (or even agreed upon) anyway. Merging, even though it breaks my precious cache. 😭

skurfer added a commit that referenced this issue Jun 6, 2013
Respect the 'run tasks in background' setting for triggers as well
@skurfer skurfer merged commit b70e887 into quicksilver:master Jun 6, 2013
@pjrobertson pjrobertson deleted the backgroundTriggers branch Jul 19, 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

2 participants