End a quicksearch after an item has been found
ClosedPublic

Authored by asensi on Jun 12 2018, 6:31 PM.

Details

Summary

When someone uses the search bar to quicksearch for a file: after he presses Return (or Enter) because he has found the file, he still have to press Esc to end the quicksearch. That doesn't happen when using Total Commander, Double Commander or Krusader 2.4.0-beta3, which automatically end those quicksearches.

There are similar use cases: for example when the user quicksearchs for a file, finds it, and (like in Total Commander) presses F4 to edit the file, etc.

Of course, as the KrSearchBar code is being talked about, the improvements of KrSearchBar experts are welcome :-)

FIXED: [ 395285 ] The quicksearch bar is still opened after deleting a file
BUG: 395285

Test Plan

Using a Krusader with its original configuration: Quicksearching (not quickselecting nor quickfiltering) for a file or folder, pressing Return (not Enter because of https://phabricator.kde.org/D13443 was still not officially applied), Del, F2, F3, F4, F5, F6, F8, Shift+F3 (the quicksearch didn't have to end in this case), Shift+F4 (the quicksearch didn't have to end in this case), Shift+F5, Shift+F6, etc.

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
asensi requested review of this revision.Jun 12 2018, 6:31 PM
asensi created this revision.
nmel requested changes to this revision.Jun 13 2018, 8:22 AM
nmel added a subscriber: nmel.

I think the current behavior is useful as is, i.e. keep the search active unless the dir is changed. Let's discuss more and consider implementing an option to keep current behavior.
Also, why have you decided to apply only to Search mode? Following the logic regarding Total Commander, it uses Filter mode for its quicksearch. In addition, for Select + F8/Del it might be useful to hide the bar, as there is nothing remain to select, but I'm not sure if it's good in terms of consistency.

In the same time, I completely agree the bar shouldn't be green when file is deleted and nothing is found. The current item should move to the next match or become red.

krusader/Panel/panelfunc.cpp
439

As I mentioned in another discussion, it's a useful feature to not close the bar in this and a few other cases.

This revision now requires changes to proceed.Jun 13 2018, 8:22 AM

I think the current behavior is useful as is, i.e. keep the search active unless the dir is changed. Let's discuss more and
consider implementing an option to keep current behavior.

Mmm... Krusader's behavior was that, after quicksearching a file, when pressing Return (or Enter), the search bar was hidden (like Total Commander or Double Commander) but after Krusader 2.4.0-beta3 the behavior was changed.

Also, why have you decided to apply only to Search mode?

I wouldn't say that :-) , it could be applied to other modes, if it ends up saving time (and attention) to users.

Following the logic regarding Total Commander, it uses Filter mode for its quicksearch. In addition, for Select + F8/Del
it might be useful to hide the bar, as there is nothing remain to select, but I'm not sure if it's good in terms
of consistency.
In the same time, I completely agree the bar shouldn't be green when file is deleted and nothing is found. The current item
should move to the next match or become red.

Also the search bar could disappear, since the files were found and deleted. In the end... it saves time, attention, Esc key presses, etc. to most users (if not all users) and it would be consistent with the "close the search bar when items are found" way of working of Total Commander, Double Commander (and Krusader until recently) :-?

krusader/Panel/panelfunc.cpp
439

Mmm... in that discussion there were two persons (https://bugs.kde.org/show_bug.cgi?id=394939#c2 , https://bugs.kde.org/show_bug.cgi?id=394939#c3) that agreed that it's better that users don't have to press Esc every time after those quicksearchs. Total Commander, Double Commander (and Krusader 2.4.0-beta3) also work this way.

nmel added a comment.Jun 14 2018, 4:11 AM

I think the current behavior is useful as is, i.e. keep the search active unless the dir is changed. Let's discuss more and
consider implementing an option to keep current behavior.

Mmm... Krusader's behavior was that, after quicksearching a file, when pressing Return (or Enter), the search bar was hidden (like Total Commander or Double Commander) but after Krusader 2.4.0-beta3 the behavior was changed.

Was it changed for a reason?

Also, why have you decided to apply only to Search mode?

I wouldn't say that :-) , it could be applied to other modes, if it ends up saving time (and attention) to users.

Following the logic regarding Total Commander, it uses Filter mode for its quicksearch. In addition, for Select + F8/Del
it might be useful to hide the bar, as there is nothing remain to select, but I'm not sure if it's good in terms
of consistency.
In the same time, I completely agree the bar shouldn't be green when file is deleted and nothing is found. The current item
should move to the next match or become red.

Also the search bar could disappear, since the files were found and deleted. In the end... it saves time, attention, Esc key presses, etc. to most users (if not all users) and it would be consistent with the "close the search bar when items are found" way of working of Total Commander, Double Commander (and Krusader until recently) :-?

That's the question, whether it saves time or adds more time. Total Commander is not a ground truth, it's just an app developed by some dev, and we don't know how he came up with the decision - has he conducted a user study or he just set it up according his personal preferences.

I don't claim that current way is the best default. I simply state it's useful. Let me elaborate how I use it. Say, I want to manually check text files ending with .txt. I type txt and F3 them one by one. I may want to F4 some of them. Currently it creates kind of virtual subset without going through Search + Feed to list box. I do it for .ini, .conf, sometimes for parts of names, so I don't want it to reset every time I press view or edit since I need to re-type the search string on every instance.

in that discussion there were two persons that agreed that it's better that users don't have to press Esc every time after those quicksearchs

It was only regarding Enter (which may be fine since it changes context) and 3 person is too small group anyway. v2.5+ is here for a long time, so we may have people who liked new behavior. We are talking about changing it again for 100s of users. That's why I proposed to discuss it and I'd like to listen to at least a few opinions before we make a move.

asensi added a comment.EditedJun 14 2018, 11:20 AM

Was it changed for a reason?

Yes, that's a question, I don't remember having read about a proposal for that modification. Maybe it happened like in the https://phabricator.kde.org/D13497 case (it was not an intended change). :-?

Total Commander is not a ground truth, [...]

Nobody said it was :-), though a lot of people use Total Commander, so they are used to that way of working (end quicksearches after users have found the items), which avoids pressing a lot of Esc key presses, saves time, attention, etc. to most users (if not all users), and it's consistent with usual Krusader's way (e.g. 2.4.0-beta3: end quicksearch after pressing Return, or Enter, or entering a folder, or a compressed file...).
By the way, for Linux, Total Commander even recommends Krusader (https://www.ghisler.com/efaqgeneral.htm , https://www.ghisler.com/dfaqgeneral.htm) :-)

Let me elaborate how I use it [...]

Thanks for the use case!! Mmm... in that case I would use a quickfilter (Ctrl+I), I wonder if that would that be serviceable for your use case.
Note: That would also be useful when having to work under Windows, with Total Commander.

Say, I want to manually check text files ending with .txt. I type txt [...]

For me that works but using a quickfilter (Ctrl+I). If I quicksearch I have to add a "*" in front of the "txt".

It was only regarding Enter (which may be fine since it changes context) [...]

Note: Enter also allows executing a file, opening a text file, etc.

and 3 person is too small group anyway.

I never wrote that three users were enough :-) It's just that you wrote "As I mentioned in another discussion" and I wrote about two other mentions :-O :-)

That's why I proposed to discuss it and I'd like to listen to at least a few opinions before we make a move.

Yes, this review tool is meant to talk about it when needed :-). The prior "do not end the quicksearch when an item is found" modification was done and wasn't discussed :-(

v2.5+ is here for a long time, so we may have people who liked new behavior.

Mmm... Krusader 2.5 was launched only one year and seven months ago, prior Krusader programs were working the aforementioned way for years (I've tried an old virtual machine (from ten years ago :-) ) and Krusader 1.9.0 also ends a quicksearch when pressing Return).
If you have a use case where quickfiltering isn't useful, what do you think about that I implement an option to keep the behavior that you mention :-?

Added more people explicitly - guys, what's your opinion on this subject and do you know if the behavior is changed intentionally or not?

Regarding the filter, I think the behavior should be consistent across all modes of quick search, i.e. decision of closing the search bar should not depend on the mode. BTW, that's what Total Commander does.

To clear possible doubts regarding my opinion on this issue, I don't mind reverting to the previous behavior, however, in my view, that would be a minor regression in usability. It's fine for Enter (because the context most likely is moved to an external app; for dirs it's already hiding the bar as the dir changes - no question here) but for view/edit/copy/move and maybe delete there is a value in not resetting the search (see the use case in my previous comment). In the same time, I consider it only a minor usability regression because it could be worked around by a regular search + vfs and it's not a frequent need. That's why an option may be an overkill if nobody beside me sees a value in it. Every option slightly increases a maintenance burden.

In D13499#280459, @nmel wrote:

Added more people explicitly - guys, what's your opinion on this subject and do you know if the behavior is changed intentionally or not?

Just used to press Esc. But it's a habit. For sure, it is more convenient to have fewer keystrokes... Just my 2 cents.

nmel added a comment.Jun 28 2018, 6:58 AM

Just used to press Esc. But it's a habit. For sure, it is more convenient to have fewer keystrokes... Just my 2 cents.

Thanks for the feedback! Do you think it should be applied to all bar modes?

In D13499#284250, @nmel wrote:

Just used to press Esc. But it's a habit. For sure, it is more convenient to have fewer keystrokes... Just my 2 cents.

Thanks for the feedback! Do you think it should be applied to all bar modes?

It turns out that those days Yuri can not answer through phabricator.kde.org, though about this subject Yuri has just written that "Yes, it would be convenient for me if all the modes work consistently. Can you answer for me? The nearst day I can answer by myself is 10 of July".

nmel resigned from this revision.Jul 6 2018, 8:07 AM

Thanks Toni and Yuri.

Regarding the filter, I think the behavior should be consistent across all modes of quick search, i.e. decision of closing the search bar should not depend on the mode. BTW, that's what Total Commander does.

Unfortunately, I need to take this back. I recently tested TC and it doesn't cancel the filter in this case, only hides the bar (which was actually confusing to me when I discovered it). I previously looked at the bar so I haven't noticed. We don't have to mimic TC, however this brings one more dimension to the usability discussion.

I see there is no much dev activity these days, so discussion is not happening. Maybe we should ask in our user group to see if anyone cares or not.

asensi added a comment.Jul 6 2018, 8:48 AM

Unfortunately, I need to take this back. I recently tested TC and it doesn't cancel the filter in this case, only hides the bar

You don't have to worry, I suppose that nobody was thinking about e.g. ending a quickfilter after F4 is used.

nmel requested changes to this revision.Jul 8 2018, 6:31 AM

Unfortunately, I need to take this back. I recently tested TC and it doesn't cancel the filter in this case, only hides the bar

You don't have to worry, I suppose that nobody was thinking about e.g. ending a quickfilter after F4 is used.

Actually, this is what Yuri (according to his "Yes, it would be convenient for me if all the modes work consistently.") and myself were thinking about.

However, I thought more and more on this and now I'm more inclined to accept this change almost as is (please check my inline comment). You were right that we can use quick filter mode to aim my use case and I'm not sure why I didn't use filter as much before. It's more convenient to track what you've viewed/edited this way. I think it makes sense to use different behaviour for search and filter.

krusader/Panel/panelfunc.h
26 ↗(On Diff #36067)

Please move to cpp.

asensi updated this revision to Diff 37386.Jul 8 2018, 8:40 PM

An include was moved to the cpp file, as Nikita wrote.

nmel added a comment.Jul 15 2018, 7:02 AM

I have trouble applying the patch to current master:

$ arc patch D13499
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D13499.
Checking patch krusader/Panel/panelfunc.cpp...
error: while searching for:

void ListPanelFunc::deleteFiles(bool moveToTrash)
{
    if (files()->type() == FileSystem::FS_VIRTUAL && files()->isRoot()) {
        // only virtual deletion possible
        removeVirtualFiles();
                                                                                                                                                                                             
error: patch failed: krusader/Panel/panelfunc.cpp:673
Checking patch krusader/Panel/krsearchbar.h...
Checking patch krusader/Panel/krsearchbar.cpp...
Applying patch krusader/Panel/panelfunc.cpp with 1 reject...
Hunk #1 applied cleanly.
Hunk #2 applied cleanly.
Hunk #3 applied cleanly.
Hunk #4 applied cleanly.
Hunk #5 applied cleanly.
Rejected hunk #6.
Applied patch krusader/Panel/krsearchbar.h cleanly.
Applied patch krusader/Panel/krsearchbar.cpp cleanly.

 Patch Failed! 
Usage Exception: Unable to apply patch!

Please fix.

asensi updated this revision to Diff 37797.Jul 15 2018, 11:09 AM

The diff was updated because of some posterior changes.

nmel accepted this revision.Jul 17 2018, 5:11 AM

Tested, works as expected. Thanks a lot Toni!

This revision is now accepted and ready to land.Jul 17 2018, 5:11 AM
This revision was automatically updated to reflect the committed changes.
nmel added a comment.Jul 18 2018, 7:00 AM

Thanks! Please use git cherry-pick -x when updating 2.7 branch in the future.