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

Add an option to remove the last character only when pressing ⌫. Fixes #177 #2249

Merged
merged 8 commits into from Aug 5, 2016

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 29, 2016

This is a rework of #1395 and fixes #177

There's also one more commit which disables ATS globally for QS. Now everybody says this is a bad idea, but I don't see any way around it for us if we're to allow → into HTTP domains (right now, if you try and right arrow into a HTTP domain you'll get a warning in the console saying it's insecure)

@@ -1074,9 +1075,10 @@ - (void)keyDown:(NSEvent *)theEvent {
return;
}

if ([theEvent isARepeat] && !([theEvent modifierFlags] &NSFunctionKeyMask) )
if ([theEvent isARepeat] && !([theEvent modifierFlags] &NSFunctionKeyMask) && [eventCharactersIgnoringModifiers characterAtIndex:0] != NSDeleteCharacter) {
Copy link
Member Author

@pjrobertson pjrobertson Jul 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fixes a small bug whereby if you held the delete key, it would automatically execute the current object/action combo.
This is useful when you hold another key (e.g. "S" to open safari) but not for the delete key

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 29, 2016

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 29, 2016

Aren’t we using something old for HTTP which ATS doesn’t affect? In any case, I was also worried about the ability to right-arrow into URLs, but didn’t want to turn off ATS.

Since we’re using Python to parse the data anyway, my plan was to just use Python (with Requests) to fetch the data in the first place. I’ll read that article before I make up my mind, though.

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 29, 2016

If there were a way to enable ATS for just qsapp.com and turn it off for everyone else, I’d be OK with that, but that doesn’t seem possible.

So what do you think of keeping it enabled and letting Python handle the HTTP connection?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 29, 2016

If there were a way to enable ATS for just qsapp.com http://qsapp.com/ and turn it off for everyone else, I’d be OK with that, but that doesn’t seem possible.

Yes, that’s possible, I was thinking about that but then though: what if people want to right arrow into http://qsapp.com (in the end it’ll get redirected to HTTPS, but ATS wouldn’t even allow that)

So what do you think of keeping it enabled and letting Python handle the HTTP connection?

I don’t see the benefit of letting Python do it over just carrying on the way we are? Is it that “it’s in a separate process” means it’s more secure or something? Sounds like we’d kind of just be jumping through hoops to keep Apple happy, but in the end the difference for the user is non-existent: we’re still (potentially) passing user data over the net unencrypted - that’s what ATS was designed to stop.

On 29 Jul 2016, at 21:05, Rob McBroom notifications@github.com wrote:

If there were a way to enable ATS for just qsapp.com and turn it off for everyone else, I’d be OK with that, but that doesn’t seem possible.

So what do you think of keeping it enabled and letting Python handle the HTTP connection?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #2249 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAJLn4lmprz3Fx3eInooO5_bIRxnBKV6ks5qafqkgaJpZM4JXytc.

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 29, 2016

I don’t see the benefit of letting Python do it over just carrying on the way we are?

Speaking only about right-arrowing into URLs: There are no security benefits from switching to Python vs. completely turning off ATS. Both would make the feature work, and both would allow insecure traffic. Switching to Python would be more work.

But keeping ATS enabled and using Python would mean everything else QS does would be more secure, like talking to Remember the Milk, Pinboard, Dropbox, etc. and most importantly checking for plug-in and application updates from our server.

In any case, what does this have to do with the ⌫ key? I forgot to even look at those changes. 😛

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 30, 2016

OK, so I've removed the ATS commit. I didn't want to hold this up ;-)

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 30, 2016

OK, so I've removed the ATS commit. I didn't want to hold this up ;-)

After I commented on the ATS issue, I was about to come over here and ask you to do that. 😄 I’ll take a look later. Waiting for the first PyOhio talk!

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 30, 2016

Waiting for the first PyOhio talk!

Enjoy! I still have the Twilio t-shirt I got at last year's event! ;-)

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 30, 2016

OK, just tried this briefly for possible inclusion in the upcoming dev preview. At first, it didn’t seem like it was working (the key seemed to do nothing). Then I realized it was the “reset search after” preference.

Two problems:

  1. Not new, and we should have caught this long ago, but the visual indication of what matched should be removed whether the search was reset manually (with ⌫) or automatically by a timer. (Right now on master, if the search is reset automatically, hitting ⌫ still clears it visually, requiring a second ⌫ to clear the selection. Changing this might take some getting used to, but I think it will mean fewer surprises in the end.)
  2. On this branch, if you wait for the search to be reset automatically, ⌫ does nothing no matter how many times you hit it or what your ⌫ pref is. It’s probably trying to remove characters from a string that’s been wiped out.

I can look into the first problem. No promises on the second. I’ll push any changes I make to this branch (but won’t be looking for 14+ hours if you feel an urge to tackle it before then).

if (![searchTimer isValid]) {
searchTimer = [NSTimer scheduledTimerWithTimeInterval:searchDelay target:self selector:@selector(performSearch:) userInfo:nil repeats:NO];
}
[searchTimer setFireDate:[NSDate dateWithTimeIntervalSinceNow:searchDelay]];
}
Copy link
Member Author

@pjrobertson pjrobertson Jul 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just a tidy up/rewrite of existing code.

in the case of searchMode == SearchFilterAll the old code was creating a searchTimer, then immediately firing it. It's much cleaner to just not create the searchTimer in the first place

@tiennou tiennou mentioned this pull request Jul 31, 2016
@skurfer
Copy link
Member

@skurfer skurfer commented Jul 31, 2016

It’s really freaky seeing the matched text disappear on its own after so many years of it not doing that, but I know it’s the right thing.

You’re going to hate me for this, but there’s no way I could have known until you had things working up to where they are now… What’s also freaky is when the match is in the name instead of the label. In that case, the thing that matched disappears when the search is reset. I’m not sure what the behavior should be, but it’s a little more jarring when this happens (whether you have “Always Show Name” enabled or not). What do you think?

What’s the iSelector bug you fixed? Can I reproduce it easily? ⌘X in the second pane or something?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 31, 2016

Wait... the long comment I wrote never posted. Fortunately QS had it in its clipboard history ;-)
See below:

Good catch on number 1. Fixing that actually fixed number 2, so that's good :)

I also found another bug with closing the iSelector in certain cases. To reproduce (on master):

  • Enable 'double delete clears search selection
  • type one letter which you know will bring up a web search in the 1st pane (e.g. 'g' for Google or DuckDuckGo)
  • wait for the 3rd pane to open
  • press delete twice to clear the 1st pane.
  • Notice how the 3rd pane stays open.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 31, 2016

I’m not sure what the behavior should be, but it’s a little more jarring when this happens (whether you have “Always Show Name” enabled or not). What do you think?

Yeah you're right, that is strange, maybe the behaviour to not clear the string is the right thing (so my commit changing that perhaps should be discarded)
But then again, resetting the search seems like a pretty weird thing to me (I've never used it), and I think that getting used to this behaviour should be ok no? Look at it this way: it's even more useful now because you know when the search has been reset

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jul 31, 2016

The iSelector bug is a little more tricky: I fix one problem, but introduce a new problem. If you switch between two directObjects that both have a default action that requires the 3rd pane (e.g. two Web Search URLs) then with these changes the 3rd pane will close then reopen

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 31, 2016

resetting the search seems like a pretty weird thing to me (I've never used it),

Aww, you deleted the video of your preferences. I could have sworn you had that checked. 😛

and I think that getting used to this behaviour should be ok no?

Yeah, I think that’s the answer for now.

Look at it this way: it's even more useful now because you know when the search has been reset

I think once people (like me) get used to it, it’ll be an improvement. I go back and forth on that setting. I enable it because I use ⌫ to clear so often, but then I hit ⌫ anyway because I forget the reset is automatic. Slim chance I’ll forget now!

The iSelector bug is a little more tricky

I’ve seen the behavior you were trying to fix. It never really registered consciously, but that I’ve seen your fix, it’ll bug me.

Too bad you’ve undone #1468 in the process. 🙁 That third pane is hard to get right.

@hmelman
Copy link

@hmelman hmelman commented Jul 31, 2016

So how does removing the last letter work? If I type "How", I'd expect QS to show whatever matches that. If I type ⌫, that should leave "Ho" and I'd expect QS to show whatever matches that. Is that what happens or is "Ho" with no match in the first pane and when I type "s" it shows the match for "Hos"? I'd prefer the former.

If I hit whatever key to clear the match (⎋, ^U or ⌫ if configured to do so), I'd expect the match to be cleared and the object in the pane to be cleared as well.

Also, it's worth checking, I'd expect this behavior in any of the three panes.

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 31, 2016

So how does removing the last letter work?

Typing “How⌫” will show exactly the same thing as typing “Ho”.

On the other stuff, clearing the string and clearing the selection out of the interface aren’t the same thing. Clearing the selection resets the search, but the opposite isn’t true.

  • ⌃U clears the selection entirely (and the search string goes with it)
  • If the search string is 1 or more characters, ⌫ will delete one or all of them (depending on your preferences). If the search string is blank, ⌫ will clear the selection (like ⌃U)
  • ⎋ neither resets the search string or clears the selection

None of that behavior is new (with the exception of being able to remove one character at a time), so it should match expectations.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 1, 2016

OK, I just force pushed. Think I fixed the iSelector thing. I've also added a test to make sure this doesn't happen again. A test for the bug as described in this comment is more tough

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 1, 2016

The third pane behavior looks good. Nice work!

I was about to declare this ready, but I just ran into something.

  1. Make sure “Reset search after” is off
  2. Do a search
  3. Dismiss the interface
  4. Call the interface back up
  5. Hit ⌫ as many times as you want, but nothing happens

The selection and matched characters can’t be removed. However, the string and scope appear to be reset because if you just start typing, you get a new search. It doesn’t matter what you have the ⌫ key configured to do.

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 1, 2016

can we get this out and save the clearSearch issue as an 'exercise for the reader’!?

Yeah, didn’t mean it had to be sorted out here. Recording it in an issue is good.

We still need to do something about “tab to the 2nd pane then tab again to go back to the 1st”, though. I might be able to look at it later if you don’t get to it before bed.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 1, 2016

We still need to do something about “tab to the 2nd pane then tab again to go back to the 1st”, though. I might be able to look at it later if you don’t get to it before bed.

When I said ‘leave it as an exercise for the reader’ I mean leave this tabbing bug - it’s not new and it’s probably not widely known, plus we need to decide what’s the best action: clear the search or allow you to continue the search.
Then there’s the question of refactoring all the ‘clear search’ code

On 1 Aug 2016, at 20:17, Rob McBroom notifications@github.com wrote:

can we get this out and save the clearSearch issue as an 'exercise for the reader’!?

Yeah, didn’t mean it had to be sorted out here. Recording it in an issue is good.

We still need to do something about “tab to the 2nd pane then tab again to go back to the 1st”, though. I might be able to look at it later if you don’t get to it before bed.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #2249 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAJLn4-P_UmqyMKgZFEawrWt7OA4_jRZks5qbePpgaJpZM4JXytc.

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 1, 2016

I mean leave this tabbing bug - it’s not new and it’s probably not widely known

The underlying cause probably isn’t new, but the behavior is. On master, if I tab to the second pane then back to the first, the ⌫ key still works. With more testing, I can see that the search scope is reset, so you can’t edit/append to your search, but I’m less worried about that for now.

I assume what’s happening is that you tab and the search string is reset (but not visually cleared), so when you go back, there’s nothing to delete and the ⌫ key does nothing. On master, where ⌫ represents dumb brute force, it still works. Now that it’s more sophisticated, it’s missing some places.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 1, 2016

On master, if I tab to the second pane then back to the first, the ⌫ key still works. With more testing, I can see that the search scope is reset, so you can’t edit/append to your search, but I’m less worried about that for now.

Oh I see, yeah. In that case I’ll do the simplest thing here and make the change so that it also clears the search when you tab. Whether this is the best solution is to be discussed in the other thread.
I should get time tonight before bed.

On 1 Aug 2016, at 20:56, Rob McBroom notifications@github.com wrote:

I mean leave this tabbing bug - it’s not new and it’s probably not widely known

The underlying cause probably isn’t new, but the behavior is. On master, if I tab to the second pane then back to the first, the ⌫ key still works. With more testing, I can see that the search scope is reset, so you can’t edit/append to your search, but I’m less worried about that for now.

I assume what’s happening is that you tab and the search string is reset (but not visually cleared), so when you go back, there’s nothing to delete and the ⌫ key does nothing. On master, where ⌫ represents dumb brute force, it still works. Now that it’s more sophisticated, it’s missing some places.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #2249 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAJLnzBR8NIxOB0eoo0JuOU54tikIudIks5qbe0EgaJpZM4JXytc.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 1, 2016

OK, one more commit pushed. See my comment over at #2253. As always, this is a can of worms...!

@tiennou
Copy link
Member

@tiennou tiennou commented Aug 2, 2016

I do feel like having ⌫ manage the search string like it does makes it more friendly, and I actually disabled the Extra here.

Would it be acceptable to have "hold" ⌫ do the "clear search" thing the Extra did, so we can remove that knob ? I'm just thinking that long-⌫ makes sense in that context...

Then there’s the question of refactoring all the ‘clear search’ code

Look, more cleanup ! I might take a look at this, as said on #2253, I have no idea what's the difference between all those strings anymore...

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 2, 2016

Would it be acceptable to have "hold" ⌫ do the "clear search" thing the Extra did, so we can remove that knob ?

Nope.

Besides, if we’re trying to make the behavior more normal, don’t we want holding ⌫ to just repeat?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 2, 2016

Yes, Rob you're on my side for once in this PR ;-)

Besides, if we’re trying to make the behavior more normal, don’t we want holding ⌫ to just repeat?

Yes, and this was my intention. You'll see in my comment that this wasn't the case (holding delete would actually execute the action), but I fixed that.

The only thing I did consciously do was to not let holding delete clear the object entirely. It would delete all the typed letters, but would leave the last object in place - until you press delete again. Although I'm not quite sure now if this is dresired

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 2, 2016

I think the current behavior is best. Let repeated ⌫ remove characters, but stop at clearing the selection. Removing the selection would become too easy to do accidentally otherwise.

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 3, 2016

Hmm. Holding the ⌫ key down does seem to wipe everything. (Apparently, I never do that naturally, so never noticed.)

I was going to merge this. If you don’t want to look into what I just described, merge away.

FWIW, on master holding the key is the same as pressing it quickly.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 3, 2016

Hmm. Holding the ⌫ key down does seem to wipe everything

Aah, you're right. I'd thought about it, but had never actually implemented it. I've done it now

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 3, 2016

That was fast! 😃

Has anyone verified that a clean install acts the way it’s supposed to? I’m 95% sure it will, but haven’t checked. I’m not going to hold this up waiting for the answer. Just wondering.

@tiennou
Copy link
Member

@tiennou tiennou commented Aug 3, 2016

Let repeated ⌫ remove characters

Unless you have the Extra default set, which is why I proposed changing the Extra to "Hold ⌫ clears search" or something.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 4, 2016

Has anyone verified that a clean install acts the way it’s supposed to?

No, but the it 100% will. In the code it checks for the 'double delete' pref (which is now misnamed, it means 'clear the whole string with one delete press', and if that doesn't exist then carries on with the default behaviour (delete a letter at a time

Unless you have the Extra default set, which is why I proposed changing the Extra to "Hold ⌫ clears search" or something.

So your idea is that if you have the pref set (so a single delete clears the whole string) you'd also like to have hold delete to clear the object as well? This could be done, but I think it'd be a bit of a user nightmare if the behaviour of 'hold delete' also changed when changing that delete pref!

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 4, 2016

I say merge away! :D

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 4, 2016

I say merge away!

I’m really close to merging.

I noticed something that felt “weird” when right-arrowing then hitting ⌫. It’s probably the same behavior as before and I just wasn’t scrutinizing so closely. Just want to confirm.

@tiennou
Copy link
Member

@tiennou tiennou commented Aug 4, 2016

So your idea is that if you have the pref set (so a single delete clears the whole string) you'd also like to have hold delete to clear the object as well?

If this is the current behavior, then it's now wrong. Which is why I'm advocating for the change : it makes the general behavior sane (everyone gets press-delete-remove-a-char), and people like me which have a tendency to get upset at things being slow to "reset" have a way to do it quickly instead of mashing keys.

EDIT I still consider this an Extra, and I'm using it because when I've testing stuff by performing lots of searches in the catalog, it can be painful to have to clear everything manually.

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 4, 2016

If you have the extra enabled, it’s still ⌫⌫ to clear everything just like before.

The only difference on this branch is that, if the search string is empty, ⌫ will clear the selection with or without the extra enabled. I don’t see that as a problem, but it’s something to be aware of. (I was going to mention it in the release notes.)

Personally, I like that it stops short of clearing the selection when I hold the key (which is the old behavior anyway).

For your “performing lots of searches” use case, you only need to clear the search string, not the selection, which is possible with just one ⌫. I know it doesn’t feel like starting over quite as much, but you know it is. 😃

@skurfer skurfer merged commit f360c0e into master Aug 5, 2016
1 check passed
@skurfer skurfer deleted the deleteKey branch Aug 5, 2016
@skurfer
Copy link
Member

@skurfer skurfer commented Aug 5, 2016

I think it’s pre-release time. Maybe I’ll list what I plan to include in Slack.

skurfer added a commit that referenced this issue Aug 5, 2016
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 5, 2016

Yay!

On 5 Aug 2016, at 09:59, Rob McBroom notifications@github.com wrote:

I think it’s pre-release time. Maybe I’ll list what I plan to include in Slack.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #2249 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAJLnxoHhkAvREDhZyQo0_Y1zS7PNXrJks5qcpkbgaJpZM4JXytc.

@skurfer skurfer mentioned this pull request Sep 2, 2016
pjrobertson added a commit that referenced this issue Jan 3, 2017
The problem is that clearing the search string too early means mnemonics are not saved, so if we move the mnemonic saving code earlier this fixes it.
The reason we want to clear the search string on interface close is a UI thing. See this comment: #2249
Fixes #2302
Fixes #2300
Related to #2268 #2269
@@ -904,7 +907,7 @@ - (void)partialStringChanged {
if ([resetTimer isValid]) {
[resetTimer setFireDate:[NSDate dateWithTimeIntervalSinceNow:resetDelay]];
} else {
resetTimer = [NSTimer scheduledTimerWithTimeInterval:resetDelay target:self selector:@selector(resetString) userInfo:nil repeats:NO];
resetTimer = [NSTimer scheduledTimerWithTimeInterval:resetDelay target:self selector:@selector(clearSearch) userInfo:nil repeats:NO];
Copy link
Member

@tiennou tiennou Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... isn't this what #2308 is about ?

Copy link
Member

@skurfer skurfer Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I wonder if reverting that is all we really need. I’ll look into it later.

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.

4 participants