quicksilver / Quicksilver

Quicksilver Project Source
http://qsapp.com
Apache License 2.0
2.74k stars 285 forks source link

Search is not always visually cleared when resetting the search string #2253

Closed pjrobertson closed 2 years ago

pjrobertson commented 8 years ago

In various cases where we reset the search string, the interface is not always reset (selected string, matched string shown in the results window).

This came to light when discussing #2249

Perhaps fundamentally, the problem is that littered throughout the code we are individually resetting the matchedString or partialString but not updating the interface. Ideally, when resetting either of these strings then the interface should also be reset.

More work is needed to distinguish the difference between these: (I for one do not fully understand):

This should really include some form of unit tests, or perhaps writing unit tests will be a really good exercise in refactoring the code and fixing this issue

pjrobertson commented 8 years ago

I discovered this when looking through the final comments on #2249, but thought I'd post here.

I've realised why clearSearch is not always called (or at least one reason) - it's to ensure matchedString is kept 'saved'. matchedString is what's used to store the mnemonic a given search, so if it were cleared when the interface is closed/the user tabs to another pane then the mnemonic information would be lost.

This brings up a question of what the ideal solution is:

I'm inclined to go with option 2

tiennou commented 8 years ago

I'm inclined for 1, because that preserves what the user typed, so I can "tex", tab to action, notice I selected "TextEdit" while I expected "TextMate", go back and tweak my search string so I select the correct thing*. IMHO, the only time where clearing anything feels acceptable to me is when the command gets finally executed.

pjrobertson commented 8 years ago

IMHO, the only time where clearing anything feels acceptable to me is when the command gets finally executed.

Or when the interface gets dismissed.

You mentioned in #2249 you couldn't quite remember what all the ...String iVars are for. I think my last comment sums them up pretty well

tiennou commented 8 years ago

Or when the interface gets dismissed.

Right.

So, without looking at the code, -partialString is what's typed in, -matchedString is what comes out after performing the search (a NSAttributedString with underlines ?) and is used for mnemonics on validation, and -visibleString is what's shown in the object view ? I'm still confused 😉 .

pjrobertson commented 8 years ago

The trick is that matchedString is only set if there’s an actual match. So, if you type asdfasdfasdf and the last object selected is As daft as Duck, then matchedString is not set to asdfasdfasdf (it’ll be nil) meaning when your command is executed QS won’t incorrectly assign a non-existant string to the object As daft as Duck.

Get it?

So visibleString is what’s shown in the UI, not matchedString. That’s solely used for mnemonics^

^Take what I’ve said with a pinch of salt actually, I haven’t checked if matchedString is used for anything else

On 2 Aug 2016, at 20:47, Etienne Samson notifications@github.com wrote:

Or when the interface gets dismissed.

Right.

So, without looking at the code, -partialString is what's typed in, -matchedString is what comes out after performing the search (a NSAttributedString with underlines ?) and is used for mnemonics on validation, and -visibleString is what's shown in the object view ? I'm still confused 😉 .

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/quicksilver/Quicksilver/issues/2253#issuecomment-236893199, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJLn-x49FgPYZCJ1lN6uBd6gog74N9Eks5qbzxJgaJpZM4JZOg6.

skurfer commented 8 years ago

This can be closed, right?

stale[bot] commented 2 years ago

This issue hasn't been updated in over 2 years. It has been marked as 'stale' and will be closed in 30 days. Please check whether this is still an issue with the latest version of Quicksilver. If so, update or comment on this issue to keep it open.