Also remove the root decorator from the result list,
seems unneeded (will be separate commit, included for one-stop review).
Target: 5.1
Also remove the root decorator from the result list,
seems unneeded (will be separate commit, included for one-stop review).
Target: 5.1
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
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? |
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. 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: Any proposal how to make that more clear in the in-code comment? |
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! |
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 :) |