Shift+Del in order to remove cruft from the history of the "Search for" comboBox in the search dialog
ClosedPublic

Authored by asensi on Sep 22 2018, 4:22 PM.

Details

Summary

Normally, everybody likes good suggestions, and dislikes bad ones. For example, when Krusader users utilize the "Search for" comboBox in the search dialog: they often see bad suggestions because there are obsolete entries, mistakes and so on, but normal Krusader users can't remove those entries. For example, web browsers have Shift+Del in order to remove the thwarting entries.

With a new class that is introduced with this commit, Krusader users are able to delete wrong entries from history lists, and therefore they can remove remove those bad suggestions that they were seeing. In this commit the new class is used only in one place, but it's intended that (in next commits) the new class will be used in more places.

In other words:

  • Usually, the history of comboBoxes (like the "Search for" one in the search dialog) end up getting plenty of cruft. With the present change the user is able to get rid of obsolete entries, mistakes, and so on. When the user goes to the "Search for" comboBox, starts typing and he sees that a proposed suggestion shouldn't exist, then the user is able to press Shift+Del and that useless suggestion gets deleted. The user can also use the arrow keys in order to see all the history in the "Search for" comboBox, and delete the entries that are needless.

I'm no QWidgets expert, improvements are welcome :-)

Test Plan

Using the "Search for" comboBox in the Search dialog, several times: add a new search term, search with it, delete the search term a) using Shift+Del when the popup list is seen b) using the arrow keys and Shift+Del when the popup list is not visible; monitor the changes in the ~/.config/krusaderrc file. Try to delete all the search terms until the list is empty, press Shift+Del again.

Diff Detail

Repository
R167 Krusader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
asensi requested review of this revision.Sep 22 2018, 4:22 PM
asensi created this revision.
asensi updated this revision to Diff 42144.Sep 22 2018, 6:11 PM

An address was updated

asensi updated this revision to Diff 42164.Sep 23 2018, 6:55 AM

Another address was updated

abika requested changes to this revision.Oct 4 2018, 7:22 PM
abika added a subscriber: abika.

Deletion was working as expected during tests. Maybe a visual hint would be good so that the users know this feature exists.

krusader/GUI/krhistorcombobox.cpp
40 ↗(On Diff #42164)

have you tried using KHistoryComboBox::removeFromHistory()?

60 ↗(On Diff #42164)

See above: can be replaced by KHistoryComboBox::removeFromHistory()?

krusader/GUI/krhistorcombobox.h
27 ↗(On Diff #42164)

Please use

/** 
* ...
*/

comments everywhere like e.g. in the KF5 libraries. No text on the first /** line.
And line break after 100 or 120 chars.

30 ↗(On Diff #42164)

Stackoverflow has shorter "Share" links - https://stackoverflow.com/a/52459337/6286694

32 ↗(On Diff #42164)

Please move the class to the cpp file.

44 ↗(On Diff #42164)

Please move the class to the cpp file.

57 ↗(On Diff #42164)

Typo: KrHistoryComboBox

65 ↗(On Diff #42164)

Full variable name:
KHBoxEventFilter boxEventFilter;

66 ↗(On Diff #42164)

Full variable name here, too.
Wait, and why are these even fields? You can create the filters in the constructor

This revision now requires changes to proceed.Oct 4 2018, 7:22 PM
asensi updated this revision to Diff 43053.EditedOct 7 2018, 4:08 PM

Some changes were made after Alex Bikadorov made some propositions.

asensi marked an inline comment as done.Oct 7 2018, 4:43 PM
asensi added inline comments.
krusader/GUI/krhistorcombobox.cpp
40 ↗(On Diff #42164)

I tried using removeFromHistory() but it didn't work as it was expected :-(, the {code to delete the entry in the box} was still needed, and the entry still was seen in the krusaderrc file in the SearchFor Completion= section (although it was deleted in the SearchFor History= section).

60 ↗(On Diff #42164)

[This is being talked about in other answer].

krusader/GUI/krhistorcombobox.h
27 ↗(On Diff #42164)
30 ↗(On Diff #42164)

That type of address helps me to avoid confusions (e.g. writing the address of another answer, that mistake would be immediately seen because the subject is visible in the address).

32 ↗(On Diff #42164)

In that case this message is seen when compiling:

error: ‘KHBoxEventFilter’ does not name a type
KHBoxEventFilter boxEventFilter;
^~~~~~~~~~~~~~~~

and if, additionally, a class KHBoxEventFilter; line is added to the .h file then

field ‘boxEventFilter’ has incomplete type ‘KHBoxEventFilter’
KHBoxEventFilter boxEventFilter;
            ^~~~~

but perhaps it was meant another thing with "Please move the class to the cpp file" :-?

57 ↗(On Diff #42164)

I find that if the names of two classes are different only by one letter then misplacements (e.g. using one class instead of another) are more common. When two differences exist then those mistakes are caught much more frequently. And if the differences exist at the beginning (instead of at the end) then they are more clearly seen.

65 ↗(On Diff #42164)

That's OK for me :-)

66 ↗(On Diff #42164)

Event filters are needed after an object is constructed, e.g. in order to process what the user is typing.

asensi updated this revision to Diff 43062.Oct 7 2018, 5:01 PM
asensi marked an inline comment as done.

Some changes were made after Alex Bikadorov made some propositions.

asensi marked 3 inline comments as done.Oct 7 2018, 5:04 PM
asensi added a comment.Oct 7 2018, 5:26 PM

Maybe a visual hint would be good so that the users know this feature exists.

I think the same :-)

abika added inline comments.Oct 15 2018, 7:04 PM
krusader/GUI/krhistorcombobox.cpp
79 ↗(On Diff #43062)

installEventFilter(new KHBoxEventFilter(this));

83 ↗(On Diff #43062)

iv->installEventFilter(new KHBoxListEventFilter(this));

krusader/GUI/krhistorcombobox.h
27 ↗(On Diff #42164)

I just think we should try to stick to one kind of comments and "/** ... */" is imho best for most cases.

30 ↗(On Diff #42164)

This is also about link stability: The share links are less likely to change over time and point to unreachable pages.

32 ↗(On Diff #42164)

true. But it should be possible when removing these fields. See below.

57 ↗(On Diff #42164)

Then please find a different name instead of leaving out characters.

I mean when you write an essay you wouldn't omit characters in words only because the reader could mistake them for other, similar words.

66 ↗(On Diff #42164)

create them as new objects. See below

asensi updated this revision to Diff 62769.Jul 29 2019, 9:25 PM
asensi marked 2 inline comments as not done.

This is the new version of the source code, after some minor improvements, having updated it because of the changes to the source code that have happened since the last time (like in krsearchdialog.cpp), and having used daily the classes meanwhile.

asensi updated this revision to Diff 62779.Jul 30 2019, 8:48 AM

The new files were updated (copyright years were changed).

nmel requested changes to this revision.Jul 31 2019, 5:06 PM
nmel added a subscriber: nmel.

It's a useful feature for people that like to keep their environment clean. Thanks for working on this, Toni!

krusader/GUI/krhistorycombobox.cpp
63

Since obj == iv, you can use iv here and below for clarity.

64

dynamic_cast

82

QAbstractItemView *iv (pointer style)

krusader/GUI/krhistorycombobox.h
3

We don't include Shie & Rafi in new files since they haven't contributed to this code. We just use more generic Krusader Krew (see compat.h for example).

34

I agree with Alex, these classes are auxiliary and should be fully moved to cpp. You don't plan to use them in other files, do you?

This revision now requires changes to proceed.Jul 31 2019, 5:06 PM
asensi marked 3 inline comments as done.Aug 1 2019, 8:52 PM
asensi added inline comments.
krusader/GUI/krhistorycombobox.h
3

In the past I added some files (I remember krarcbasemanager.h and krdebuglogger.h), and assigned its copyright to Shie and Rafi. I feel uncomfortable writing only "Krusader Krew" as the copyright owner, would that allow future Krusader Krews to fork Krusader files with a proprietary license and more :-?. Including Shie and Rafi as copyright owners makes future scenarios safer as future Krusader Krews would need Shie and Rafi to perform some critical legal actions.

Even though I appreciate a lot our Krusader Krew, I also have to face that we not a company, nor an registered nonprofit organization, etc. :-( so I wouldn't perform some particular legal actions (perhaps some of them are not valid, or are unnecessary risky) in the name of our nowadays beloved Krusader Krew.

34

I suppose you are talking about two class declarations, I moved them to the cpp file, there were no longer any "does not name a type" error messages, thought a "[krusader/GUI/CMakeFiles/GUI_autogen] Error 1" one appeared. In order to solve it I added a

#include "krhistorycombobox.moc" // required for class definitions with Q_OBJECT macro in implementation files

line, in a similar way to the

#include "jobman.moc" // required for class definitions with Q_OBJECT macro in implementation files

line that already existed in the Krusader source code. I wonder if do you have any alternative :-?

asensi updated this revision to Diff 62939.Aug 1 2019, 8:56 PM

Minor changes were performed.

asensi added a comment.Sun, Sep 1, 6:29 PM

Kindly ping

abika accepted this revision.Sun, Sep 15, 5:13 PM

Code looks good and still works!

krusader/GUI/krhistorycombobox.cpp
89

i would recommend to not use pure abbreviations like iv but longer variable names like itemView. This is much more readable.
Same for box vs. comboBox. A bit longer is better in most cases.

asensi updated this revision to Diff 66145.Sun, Sep 15, 6:20 PM
asensi marked an inline comment as done.

Alex's recommendations were applied in order to ease the reading of the source code (and the changes could be applied quickly :-) ).

nmel accepted this revision.Sun, Sep 15, 6:36 PM
nmel added a subscriber: yurchor.

Works great and the code looks clean and neat! Thanks for working on this.

@yurchor, could you please update the docs once it's merged?

krusader/GUI/krhistorycombobox.h
3

I'm not a lawyer, but I see many orgs use names in copyright lines which are not companies or non-profits. For example, Gentoo build configs are authored as (# Copyright 1999-2019 Gentoo Authors) and Gentoo Authors is not an org.

In the same time, it doesn't hurt, so I have no issue here. I'm not sure if Shie and Rafi have any issue for having their names signed under the code they didn't write, but so far they haven't expressed any objections. ;)

This revision is now accepted and ready to land.Sun, Sep 15, 6:36 PM
In D15693#531988, @nmel wrote:

@yurchor, could you please update the docs once it's merged?

Sure. Thanks for the pointer.

Code looks good and still works!

Yes, I also use that code habitually :-)

Moritz Bunkus (from bunkus.org) has also been performing some tests, and agreed in starting using the new features. In the future someone may improve the source code, of course.

krusader/GUI/krhistorycombobox.h
3

I'm not a lawyer :-), and in a similar vein to previous comments, I have to trust more in Shie and Rafi than in future Krusader Krews because I still don't know about them :-(

This revision was automatically updated to reflect the committed changes.

And thanks Nikita, Alex, and Moritz! (and Yuri :-)