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

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

Details

Reviewers
abika
Group Reviewers
Krusader
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
Lint Skipped
Unit
Unit Tests Skipped
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
41

have you tried using KHistoryComboBox::removeFromHistory()?

61

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

krusader/GUI/krhistorcombobox.h
28

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.

31

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

33

Please move the class to the cpp file.

45

Please move the class to the cpp file.

58

Typo: KrHistoryComboBox

66

Full variable name:
KHBoxEventFilter boxEventFilter;

67

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
41

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).

61

[This is being talked about in other answer].

krusader/GUI/krhistorcombobox.h
28
31

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).

33

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" :-?

58

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.

66

That's OK for me :-)

67

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

installEventFilter(new KHBoxEventFilter(this));

83

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

krusader/GUI/krhistorcombobox.h
28

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

31

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

33

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

58

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.

67

create them as new objects. See below