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

removed an annoyance where the results view is changed upon updating #325

Merged
merged 4 commits into from Mar 29, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 20, 2011

This is something that's annoyed me for a while:

If you type fast in the 1st pane then tab to the 2nd pane before the icons have loaded in the 1st pane, the selection is moved from the 2nd pane back to the 1st.

Easy way to reproduce: Type a letter then tab instantly after it.

Even easier:

  1. Set the 'wait before searching' time in QS Prefs to the longest (0.5s)
  2. type in the 1st pane then tab.
  3. when the icon in the 1st pane loads the focus goes back to the 1st pane. Really annoying, and this pull hopefully fixes it...

I've commented it out for now just so you can see what it is that I've removed. If we're all happy then I can remove it in another commit

@skurfer
Copy link
Member

@skurfer skurfer commented May 20, 2011

I can’t reproduce the behavior you’re complaining about. The focus stays in the 2nd pane, even as the action is changed. Which interface? What type of object are you calling up in the first pane?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 20, 2011

Happens to me on every interface.

Try changing the 'wait before searching' time in the prefs to the longest it
can be, then tabbing as quickly as possible to the 2nd pane after typing in
the 1st pane and waiting.

Still nothing?

On 21 May 2011 02:29, skurfer <
reply@reply.github.com>wrote:

I cant reproduce the behavior youre complaining about. The focus stays in
the 2nd pane, even as the action is changed. Which interface? What type of
object are you calling up in the first pane?

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

@skurfer
Copy link
Member

@skurfer skurfer commented May 20, 2011

Ah, I got it. I went to a clean account to record a video of it not happening and I ended up reproducing it. The focus goes back when the results list appears under the first pane (not when the object appears in it). In my main account, the results list never appears so the focus was staying in the 2nd pane.

I would argue that the right way to fix this is to prevent showResultView: from being called at all (or make it not do anything) unless the focus is still in the first pane. What do you think?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 20, 2011

Aaah yeah, same here if I disabled the results list. Good catch. I agree
that that's the right way around it, and looking more closely at the change
I've made - it's actually what I've done!

showResultView is called when the results list wants showing. The check at
the start to check the first responder and set it to the 1st pane is
unnecessary IMO.

On 21 May 2011 09:41, skurfer <
reply@reply.github.com>wrote:

Ah, I got it. I went to a clean account to record a video of it not
happening and I ended up reproducing it. The focus goes back when the
results list appears under the first pane (not when the object appears in
it). In my main account, the results list never appears so the focus was
staying in the 2nd pane.

I would argue that the right way to fix this is to prevent
showResultView: from being called at all (or make it not do anything)
unless the focus is still in the first pane. What do you think?

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

@skurfer
Copy link
Member

@skurfer skurfer commented May 21, 2011

Yeah, I think you’re right that you accidentally fixed it correctly. :)

For some reason when I looked at that line, I thought it was checking to see if QS was the frontmost app, but it’s actually checking which pane is active. I’ll merge it in and test it out.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 21, 2011

Hey hey, accidentally? I knew what I was doing all along! :P
As opposed to removing the line completely, I may add a comment above it
saying what the commented out line does.

On 21 May 2011 13:20, skurfer <
reply@reply.github.com>wrote:

Yeah, I think youre right that you accidentally fixed it correctly. :)

For some reason when I looked at that line, I thought it was checking to
see if QS was the frontmost app, but its actually checking which pane is
active. Ill merge it in and test it out.

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

@skurfer
Copy link
Member

@skurfer skurfer commented May 21, 2011

Hmmm. I don’t think it’s quite working the way it’s intended.

http://dl.dropbox.com/u/2214419/Quicksilver/annoyance.mov

Focus stays where you put it, but the results list is still appearing for the first pane when you’re in the second, at which point you can call up its results list too. I can’t reproduce it every time, but pretty consistently.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 26, 2011

I've taken note of this, and will try and fix it when I get some time -- as always, it's never just a simple fix :)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Jul 1, 2011

So, patrick, now that you're back in the UK and have summer break (I think), do you have some time...? :-)
(As you may have noticed, I'm looking through pull requests right now, doing a little bit of house keeping)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 1, 2011

Been back in Wales for 72 hours. Certainly enough time! I'll look into this next.
(Then I'll deal with the calculator pull, then I'll deal with getting triggers to work...)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 5, 2011

OK 61e2d2e should hopefully fix this.
My logic is that if the pane in question isn't the currently selected (first responder) then don't do anything.

There's a million other commits in here since I just merged MASTER into this branch.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 5, 2011

P.S. it looks like loads of line comments from those commits have also been added. They can just be ignored

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 24, 2011

Hmmm...is this finished? Or is there still something left to do? Still remember this, Patrick?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Nov 24, 2011

I think I can just about remember... :)
I don't think it was quite finished -there was something wrong. I'll look
into it (2nd on my list after the FF plugin)

On 24 November 2011 21:18, Henning Jungkurth <
reply@reply.github.com

wrote:

Hmmm...is this finished? Or is there still something left to do? Still
remember this, Patrick?


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

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 24, 2011

I'm just kind of in clean-up mode, looking through some of the older pull request.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Nov 24, 2011

Yeah I gathered that. Doing a good job as well :)
I'd completely forgotten about the calculator request!

On 24 November 2011 23:15, Henning Jungkurth <
reply@reply.github.com

wrote:

I'm just kind of in clean-up mode, looking through some of the older pull
request.


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

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 6, 2011

So what did we decide? This good to go? (I’ve been using it for a long time. It certainly doesn’t add any problems that I’ve found.)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Dec 6, 2011

Good question. Patrick said he'd look into it, but with him going on vacation, he seems to be swamped.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 6, 2011

Rob, if you've been playing with this for ages and it's fine (have you tried with 'show results > immediately' turned on?)

Then I guess it's good to go.

To be honest, if we remove the wait before searching thingy all together (see my timers pull request) this will probably be much less of a problem.

Still, I think the behaviour is right in that we shouldn't change focus when things get updated. If the user changed the focus, then they probably wanted it that way.

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 6, 2011

Rob, if you've been playing with this for ages and it's fine (have you tried with 'show results > immediately' turned on?)

Well, I never use that setting except when testing stuff, so maybe someone else should say. I was just able to get the “wrong” action to run by intentionally going quickly, but that could have nothing to do with this change.

I actually see a delay with that setting that I don’t normally see. It seems to take longer for keystrokes to register. The response time is just as fast once they do, but it sometimes takes close to a second before what I type shows in the interface. But again, this happens to some extent with or without these changes. (The timers branch doesn’t seem to make a difference either.)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 29, 2012

So she's good to go?!

Just tested it again after what seems like an age. Sorry for being so slow on this!

P.S. if you're going to test it again, make sure you merge master into it first. I just ran it without doing that and got a crash and lost all my triggers :(

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 29, 2012

I habitually merge this in every time I make a new “mine” branch, so I’ve been using it pretty much constantly since it was submitted. I no longer see any issues if I enable “Immediately” for results. That could be the result of my removal of moreComing? Not sure, but we can merge this.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Mar 29, 2012

OK, I'll merge my own pull...
10 months of work!

pjrobertson added a commit that referenced this issue Mar 29, 2012
removed an annoyance where the results view is changed upon updating
@pjrobertson pjrobertson merged commit 7f154f8 into quicksilver:master Mar 29, 2012
@skurfer
Copy link
Member

@skurfer skurfer commented Mar 29, 2012

I merged it too. Maybe you got to it first, but it looked to me like it went through. As long as it’s done, I guess.

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