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

Refresh the third pane when using triggers #1587

Merged
merged 1 commit into from Sep 5, 2013
Merged

Refresh the third pane when using triggers #1587

merged 1 commit into from Sep 5, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Sep 3, 2013

This fixes the same bug as #1582, but I think it's a little cleaner than claiming the search object changed (when it didn't) just to trigger a known side-effect. I'm not convinced I've tested every use case, but it seems to fix the bug without adding any.

* fixes a bug introduced by 4919bcd
* partially addresses #1553
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 3, 2013

Cool, I'll test it later. Looks like it's better at making trigger interface activation more like normal activation

On 4 Medi 2013, at 04:14, Rob McBroom notifications@github.com wrote:

This fixes the same bug as #1582, but I think it's a little cleaner than claiming the search object changed when it didn't just to trigger a known side-effect. I'm not I've tested every use case, but it seems to fix the bug without adding any.

You can merge this Pull Request by running

git pull https://github.com/quicksilver/Quicksilver actionRefresh
Or view, comment on, or merge it at:

#1587

Commit Summary

show third pane in the correct mode when using a trigger
File Changes

M Quicksilver/Code-QuickStepInterface/QSInterfaceController.m (2)
Patch Links:

https://github.com/quicksilver/Quicksilver/pull/1587.patch
https://github.com/quicksilver/Quicksilver/pull/1587.diff

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 4, 2013

Since you're obviously awake, if you have a few minutes to look at this (and assuming it's all good), we can get the next pre-release out. I'm thinking that will be the final one as well, but we can wait until the T-shirt thing is sorted out before we throw the switch. I can also then merge release into master.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 4, 2013

I went to sleep just before you sent this :P

I'll look this morning, I have a bit of downtime.

On 4 Medi 2013, at 22:43, Rob McBroom notifications@github.com wrote:

Since you're obviously awake, if you have a few minutes to look at this (and assuming it's all good), we can get the next pre-release out. I'm thinking that will be the final one as well, but we can wait until the T-shirt thing is sorted out before we throw the switch. I can also then merge release into master.


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 4, 2013

I'm still a little worried we might be missing something. If you look at the only other place that calls updateIndirectObjects (QSSearchObjectView.m:L453) you'll see that there's also a call to [self updateViewLocations] - should we do that as well?

(That was my reasoning behind just saying the action object had changed - I was worried we would never be able to replicate everything that call does. Perhaps replacing your added line with a notification post for searchObjectChanged would be cleaner?)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 4, 2013

But seems to work fine of course.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 5, 2013

you'll see that there's also a call to [self updateViewLocations] - should we do that as well?

It looks like all that does is show or hide the third pane. Since we’re addressing the case where no selections have changed, it should already be in the right state (and it seems to work in practice). You think?

Perhaps replacing your added line with a notification post for searchObjectChanged would be cleaner?

If you check, there’s only one place observing that notification and it’s QSInterfaceController, so we could just call searchObjectChanged directly. Looking at that method is how I ran across updateIndirectObjects. I think that’s the only part of the method that matters.

pjrobertson added a commit that referenced this issue Sep 5, 2013
Refresh the third pane when using triggers
@pjrobertson pjrobertson merged commit 7ce1e08 into release Sep 5, 2013
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 5, 2013

I think that pretty much answers that :)

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