Bug 386217 - Pressing Arrow keys behaves incorrectly in quicksearch
Summary: Pressing Arrow keys behaves incorrectly in quicksearch
Status: RESOLVED FIXED
Alias: None
Product: krusader
Classification: Applications
Component: general (show other bugs)
Version: 2.6.0
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Nikita Melnichenko
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-26 17:23 UTC by S-trace
Modified: 2018-02-14 06:31 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
revert patch (5.41 KB, patch)
2017-10-29 06:28 UTC, Nikita Melnichenko
Details

Note You need to log in before you can comment on or make changes to this bug.
Description S-trace 2017-10-26 17:23:00 UTC
Steps to reproduce:
Issue1:
1. Enter /var/ directory
2. Apply sort by name. For example:
../
backups
cache
crash
games
lib
local
lock
log
mail
...
3. Select ../
4. Type "lo" - quicksearch bar appeared and selected "local" directory
5. Press "right" arrow key
6. Nothing happened

Expected results:
6. Current directory changed to highlighted "local" dir

Issue2:
1. Enter /var/ directory
2. Apply sort by name. For example:
../
backups
cache
crash
games
lib
local
lock
log
mail
...
3. Select ../
4. Type "lo" - quicksearch bar appeared and selected "local" directory
5. Press "up" arrow key
6. "lib" directory selected
7. Press "right" arrow key
8. Current directory changed to "lib"

Expected results:
6. Quicksearch should wrap its direction and select "log" directory
8. Current directory should be changed to "log", but it was changed to "lib" (which even not matches "lo" search string!)

This bug was introduced in Krusader 2.6.0 by commit 210ca94555bd8d6213f11a605456c8895da34fe3
"Panel: don't switch focus to panel view when typing in search bar"
Comment 1 Nikita Melnichenko 2017-10-29 06:28:51 UTC
Created attachment 108621 [details]
revert patch

I confirm the regression. Thanks to S-trace for finding a commit. As a quick fix, I created a revert patch, resolved conflict on top of master and checked that it's back to normal. Please find the patch attached.

Alex, what was a motivation for the original commit 210ca94?
Comment 2 Alex Bikadorov 2017-10-29 14:58:46 UTC
About issue 1:
This is intended. While typing in the quicksearch field this is the widget which has the input focus. I would claim this is normal behaviour expected from any other text field. Because it has the focus all keyboard input it can handle is handled. And <arrow_left> and <arrow_right> are useful keys here because they can move the cursor.
Other users (at least myself) would like to keep this behaviour. And you can still type <Escape> to get the focus back to the panel. Keyboard-only usage is possible without any problems.

[And there are other potential conflicts, e.g. <CTRL+arrow_left>. It would be a mess to handle all these in a special way - for whatever the user expects it to be.]

About issue 2:
This is indeed a bug. I will look into it.

>Alex, what was a motivation for the original commit 210ca94?
It is referenced in the commit message -> Bug 375639.
Comment 3 Alex Bikadorov 2017-10-29 20:30:20 UTC
Git commit b569e558c72102886d7bd268b74ae59a0e5f0776 by Alexander Bikadorov.
Committed on 29/10/2017 at 20:29.
Pushed by abikadorov into branch 'master'.

Panel: Handle key events by search bar if it is open and text field is focused

And some code cleaning again.

FIXED: [ 386217 ] Fix moving cursor with up/down to item not matching in quicksearch

M  +6    -4    krusader/Panel/PanelView/krinterbriefview.cpp
M  +34   -19   krusader/Panel/PanelView/krview.cpp
M  +12   -2    krusader/Panel/PanelView/krview.h
M  +7    -1    krusader/Panel/krsearchbar.cpp
M  +23   -6    krusader/Panel/krsearchbar.h

https://commits.kde.org/krusader/b569e558c72102886d7bd268b74ae59a0e5f0776
Comment 4 Alex Bikadorov 2017-10-29 20:34:07 UTC
Issue 2 fixed. Please test.
Comment 5 S-trace 2017-10-29 21:07:35 UTC
(In reply to Alex Bikadorov from comment #4)
> Issue 2 fixed. Please test.

Works fine.

But about Issue1:
Of course, moving cursor with arrow keys is very useful.
But it's confusing when same key do different things in very similar usage patterns:

A:
1. Type a text for quick search
2. Matching directory was selected in the panel
3. Press right - nothing happened

B:
1. Type a text for quick search
2. Matching directory was selected in the panel
3. Press up to select another matching directory
4. Press right - current directory changed to the directory selected in panel

C:
1. Press up or down to select any directory
2. Press right - current directory changed to the directory selected in panel

Maybe it will be useful to add a check for this edge case?
I see several variants:
1. Check if right arrow was pressed when cursor already at the end of text in quicksearch and previous pressed key was not the left or right arrow - if it is, change directory to the selected (fixes a possible problem when user moves cursor using long press of arrow keys and just wanted to move it to end of typed in quicksearch field text).
2. Check if right arrow was pressed when cursor already at the end of text in quicksearch - if it is, change directory to the selected (very simple to implement, i think)
3. Check if any cursor movement by arrows was performed in this quicksearch session, if no - change directory to highlighted on right arrow press.

Thank you.
Comment 6 Nikita Melnichenko 2017-10-31 05:49:44 UTC
(In reply to S-trace from comment #5)

I agree with S-trace that the current behavior is slightly inconsistent. As a developer, I totally understand Alex's concern about text field behavior expectation, however from user point of view my daily usage pattern has been disrupted. For example, whenever I want to navigate to the dir data/projects/wdirs/krusader I simply press

d > p r > w > k r >

After the change I have to press

d Esc > p r Esc > w Esc > k r Esc >

As you see, the number of keystrokes has significantly increased. If I accidentally mistyped, I simply use Backspace and no problem.

Arrow navigation is one of the most powerful features in Krusader. I hope we can keep it this way. I also encourage the devs to consider different variants as S-trace suggested. Personally, I'm for #3 as it seems like it's reflecting user's intentions - for quick navigation simply press right arrow and go, however if the user pressed left arrow at least once, he/she wants to edit the search expression and accidental right arrow in the end won't spoil his/her efforts. It's also simple to implement with 2 states - initial A and text editing B. Start with A, switch to B whenever left arrow is pressed.
Comment 7 Toni Asensi Esteve 2017-11-01 06:36:01 UTC
Alex Bikadorov wrote:

> This is intended. While typing in the quicksearch field this is the widget 
> which has the input focus. I would claim this is normal behaviour expected 
> from any other text field. [...]

I expect the same behaviour, too.

Nikita Melnichenko wrote: 

> whenever I want to navigate to the dir data/projects/wdirs/krusader I 
> simply press

> d > p r > w > k r >

> After the change I have to press

> d Esc > p r Esc > w Esc > k r Esc >

> [...]

If it can be useful: in those cases I press

    d Enter p r Enter w Enter k r Enter

Perhaps it works in your case :-?
Comment 8 Nikita Melnichenko 2017-11-01 16:42:49 UTC
(In reply to Toni Asensi Esteve from comment #7)
> If it can be useful: in those cases I press
> 
>     d Enter p r Enter w Enter k r Enter
> 
> Perhaps it works in your case :-?

Toni, thanks for your suggestion. IMO, pressing Enter is insecure as you can unintentionally run some executable. Right arrow protects you from that.
Comment 9 Christoph Feck 2017-11-15 21:29:47 UTC
What is the status of this ticket? If I understand the comments correctly, it is unresolved?
Comment 10 Christoph Feck 2017-11-29 20:00:29 UTC
No response, changing status.
Comment 11 Nikita Melnichenko 2017-11-30 05:01:16 UTC
(In reply to Christoph Feck from comment #10)
> No response, changing status.

Hi Christoph,

IMO, it's more like CONFIRMED (as we all agree the product behaves much differently than it was for years before the change) or WONTFIX status - depends on the Krew decision. I hope they'll consider one of the S-trace's suggestions as they seem to be reasonable trade-offs. Thanks!
Comment 12 Nikita Melnichenko 2017-12-04 07:48:14 UTC
BTW, I don't mind to work on a change to bring the proposal from comment #6 to life, however we must agree on the behavior spec first. I need a confidence that my patch won't be declined because of differences in views on UI.
Comment 13 Toni Asensi Esteve 2017-12-06 10:40:17 UTC
> however we must agree on the behavior spec first.

Mmm... Alex Bikadorov explained the reasons of the behavior in:
https://bugs.kde.org/show_bug.cgi?id=386217#c2
Comment 14 Alex Bikadorov 2017-12-06 20:30:35 UTC
Well, it's not like i (or anybody else) has veto right. Under the assumption it is only this one case -just typed something in quick search and pressing right arrow key- and you even provide the patch yourself, I can live with it.

I'm only concerned about more complicated code and inconsistent user-experience, including another bug report in the future criticising this change again. And I only have little time for this project anymore. But if you wanna work on it, go ahead.

What do you think, Toni?
Comment 15 Toni Asensi Esteve 2017-12-07 08:35:11 UTC
I think the same, Alex, things would get more complicated. Also, it should be also taken into account the "brief view" (Alt+Shift+B) of Krusader, where the right arrow key is normally used to lead you to other files.
Comment 16 Nikita Melnichenko 2017-12-09 03:55:35 UTC
There is a space for a 4th checkbox in the Settings dialog (Panel -> General -> Search Bar) right under "Case sensitive" - I can add a setting to control the behavior. Toni, is it ok with you?

S-trace, Alex, Toni - please share your opinion on the best option. If get what Alex said in comment #14, he's for option #3.
Comment 17 S-trace 2017-12-09 05:52:59 UTC
I'm for option #3 too.
Comment 18 Alex Bikadorov 2018-01-07 19:28:33 UTC
Yes, option #3. A new checkbox is also great.
Comment 19 Nikita Melnichenko 2018-01-10 07:16:50 UTC
Got it. I'm pretty packed these days, however I scheduled to work on this on Jan 27th. In case anyone decides to pick this up before, please let me know.
Comment 20 Nikita Melnichenko 2018-01-28 07:24:31 UTC
The easiest part, i.e. code changes, is done. I also fixed another bug related to a search bar in my branch. Now I need to figure out how to create a pull request... bear with me.
Comment 21 Nikita Melnichenko 2018-02-14 06:31:26 UTC
Git commit 8d08925dac3f340a43476b9034e5072952a59fe6 by Nikita Melnichenko.
Committed on 28/01/2018 at 06:19.
Pushed by melnichenko into branch 'master'.

Panel: quick navigation with right arrow when search bar is used

FIXED: [ 386217 ] Pressing Arrow keys behaves incorrectly in quicksearch

Fixes a usability regression introduced in 210ca94.

Checking if cursor is in the end of the line
is needed for the case a mouse is used to position the cursor.

M  +21   -1    krusader/Panel/krsearchbar.cpp
M  +3    -1    krusader/Panel/krsearchbar.h

https://commits.kde.org/krusader/8d08925dac3f340a43476b9034e5072952a59fe6