Find/Replace in files: fix tooltip omitting the matched text in Find mode
ClosedPublic

Authored by kossebau on Feb 26 2017, 1:10 PM.

Details

Summary

Also remove the root decorator from the result list,
seems unneeded (will be separate commit, included for one-stop review).

Target: 5.1

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Feb 26 2017, 1:10 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptFeb 26 2017, 1:10 PM
kfunk requested changes to this revision.Feb 27 2017, 3:52 PM
kfunk added a subscriber: kfunk.

Rest LGTM

plugins/grepview/grepoutputmodel.cpp
153

Shouldn't this be isChecked() (doesn't exist, I know).

What exactly does this try to fix if not?

This revision now requires changes to proceed.Feb 27 2017, 3:52 PM
kossebau added inline comments.Feb 27 2017, 4:14 PM
plugins/grepview/grepoutputmodel.cpp
153

The Grep toolview has two modes: Find-only mode and Replace mode.

The Find-only mode is the default mode and entered on doing the search.
The Replace mode is triggered once one starts editing the "Replacement text" field.

In the Replace mode all items get changed to isCheckable = true, resulting in all items being shown with a checkbox, so one can select by toggling the checkboxes which of the found items will be included when doing the actual replacement.

So the isCheckable state reflects if in Find-only mode or Replace mode. The isChecked state only reflects if an item should be included in the replacement, it does not hint to the mode.

The bug is this:
Currently the tooltip only cares about the Replacement mode, previewing the version of the item with the replacement done. Which in Find-only mode does not make sense, especially as the replacement text is empty and thus the tooltip shows only incomplete content.
Now one option would be to just not show the tooltip in Find mode. But it still serves a purpose there, when the matched line is much longer than what is shown in the view, so the tooltip allows to see the complete text (like done in similar tree views).
So the fix here is to in Find mode (as reflected by the isCheckable == false) use the original text, not the replacement text, for the matched content in the line.

Any proposal how to make that more clear in the in-code comment?

kfunk accepted this revision.Feb 27 2017, 4:47 PM
kfunk added inline comments.
plugins/grepview/grepoutputmodel.cpp
153

Okay, understood, that's what I /thought/ as well.

My complaint is that using isCheckable() isn't going far enough. It should be isCheckable() && isChecked() -- since it only makes sense to show the to-be-replaced text when we actually want to replace it (IOW: when it is checked).

Anyhow, much better then before already: so go for it. Thanks!

This revision is now accepted and ready to land.Feb 27 2017, 4:47 PM
This revision was automatically updated to reflect the committed changes.
kossebau added inline comments.Feb 27 2017, 9:05 PM
plugins/grepview/grepoutputmodel.cpp
153

Okay, now got you. Hm, not sure if I agree, I would expect to always see the potential replaced text when in replacement mode. Will give that some observation with myself for a while to see if real world usage matches what I assume now :)