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

String dance #2308

Merged
merged 2 commits into from Jan 18, 2017
Merged

String dance #2308

merged 2 commits into from Jan 18, 2017

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jan 10, 2017

Right now, “Reset search after” is crippling the training/learning process. (And it’s enabled by default, so this probably affects a lot of people.) Possible solutions:

  1. Keep the matched string until the interface is dismissed or the selection is explicitly cleared
  2. Rip out the “Reset search after” feature entirely
  3. Add a yet another string (a 4th?) to keep track of the abbreviation until the interface is dismissed

I’ve attempted 1. Thoughts?

The first commit fixes a bug I ran across while working on this. (You couldn’t clear the selection when using the comma trick unless the current object was in the collection.) Seemed like the best solution.

Fixes #2301

By adding it to the collection and letting uncollect do the right things. Cleaner than redefining half the behavior of uncollect here.
@tiennou
Copy link
Member

@tiennou tiennou commented Jan 10, 2017

Sorry, the code changes doesn't make that clear to me, how is it crippling it ?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jan 10, 2017

It wipes the abbreviation from existence before you have a chance to do anything that would trigger saveMnemonic.

@tiennou
Copy link
Member

@tiennou tiennou commented Jan 10, 2017

Sorry, I still don't get it. Isn't wiping the abbreviation the point of resetting the search ?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jan 10, 2017

Hard to say for sure, but I think there’s a reason for three or more strings to track what the user typed. I think the point of resetting the search is to allow you to start a new search, and that still works (by clearing searchString).

What it doesn’t do (with my changes) is clear the matchedString. So if the reset timer clears things, but the user hasn’t done anything yet, they still have an opportunity to set defaults in the results, and saveMnemonic still has enough information to do its job if they run an action or something.

@tiennou
Copy link
Member

@tiennou tiennou commented Jan 10, 2017

Ok, I think I understand. The reset timer can fire while the user is doing nothing, which will clear the search string without deselecting the found object. Then, if you tried to manipulate mnemonics for that object, you'd have no way to know what was typed in the first place, hence the crippling.

I'm not sure how to go about it (it just makes the learning process be execution-only, albeit if you pause for too long you would get that behavior anyway). I'd favor 2, actually, because User Input is King™, and the delete key now should make it explicit.

Also, I'm wondering : what did we change to cause that behavior to happen ? Reset Search has been around for quite some time, and I can't remember anything recent that could have cause it to go so badly...

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jan 11, 2017

Force-pushed a much simpler fix. Thanks.

This reintroduces the potentially confusing behavior where the search can be reset via timer or by tabbing to another pane, but there’s no visual indication. I’ll take that over losing almost all mnemonics.

And for what it’s worth @tiennou, I have no objection to ripping out the reset timer but I want to get a fix out ASAP. I’ll ask about it in Slack.

@skurfer skurfer merged commit cfde051 into master Jan 18, 2017
2 checks passed
@skurfer skurfer deleted the string-dance branch Jan 18, 2017
skurfer added a commit that referenced this issue Jan 18, 2017
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.

2 participants