Hide search extra options when not available
ClosedPublic

Authored by iasensio on Oct 6 2019, 6:45 PM.

Details

Summary

Hide the search extra options when they are not available, because the current location is not indexed or Baloo is not enabled.
The button is disabled and shows a tooltip to provide feedback of why it's not available.
Depends on D24478

FEATURE: 318580
FEATURE: 408680
CCBUG: 396898
FIXED-IN: 19.12

Test Plan
  • Ctrl-F on a non-indexed location
  • balooctl disable and Ctrl-F on an indexed location

On both cases the extra options are not shown, the button is disabled and the reason is shown in the tooltip
The user selection is remembered between indexed locations.

Diff Detail

Repository
R318 Dolphin
Branch
facets_hide
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17419
Build 17437: arc lint + arc unit
iasensio created this revision.Oct 6 2019, 6:45 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptOct 6 2019, 6:45 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
iasensio requested review of this revision.Oct 6 2019, 6:45 PM
iasensio edited the summary of this revision. (Show Details)Oct 6 2019, 6:47 PM

useBalooSearch() is not accurate; it's phrased as a command, not a question. I'd recommend changing it to usesBalooSearch or indexingEnabled or balooIndexingEnabled or something like that.

iasensio updated this revision to Diff 67453.Oct 7 2019, 7:40 PM
  • Name change

Would you mind to move the isIndexingEnabled() refactoring to another commit?

Would you mind to move the isIndexingEnabled() refactoring to another commit?

Ah, ok. Then I'd rather make this one depend on the other, since the method is used in a pair more places and all of the #ifdef HAVE_BALOO stuff would make it harder to read.

iasensio updated this revision to Diff 67460.Oct 7 2019, 9:00 PM
This comment was removed by iasensio.
iasensio updated this revision to Diff 67467.Oct 7 2019, 9:37 PM

Split out refactor of isIndexingEnabled and depend on D24478

meven accepted this revision.Oct 9 2019, 11:40 AM
meven added inline comments.
src/search/dolphinsearchbox.cpp
109–113

You could even add a function for those 4 lines, since it is repeated line 334

This revision is now accepted and ready to land.Oct 9 2019, 11:40 AM
iasensio added inline comments.Oct 9 2019, 7:33 PM
src/search/dolphinsearchbox.cpp
109–113

Agree. Just thinking on a proper method name. I'm thinking of setFacetsToSetting()

iasensio updated this revision to Diff 67579.Oct 9 2019, 9:20 PM
iasensio marked an inline comment as done.
This comment was removed by iasensio.
iasensio updated this revision to Diff 67582.Oct 9 2019, 11:31 PM
  • Add setFacetsVisible() method
  • Update toggle button on showEvent
elvisangelaccio requested changes to this revision.Oct 10 2019, 7:57 PM

More info on the i18n markers here: https://api.kde.org/frameworks/ki18n/html/prg_guide.html#good_ctxt

Looks good otherwise.

src/search/dolphinsearchbox.cpp
570

@action:button as marker.

571

@action:button as marker.

579

Typo: because (repetead below)

579

@info:tooltip as marker.

581

@info:tooltip as marker.

584

@info:tooltip as marker.

This revision now requires changes to proceed.Oct 10 2019, 7:57 PM
iasensio updated this revision to Diff 67656.Oct 10 2019, 9:12 PM
iasensio marked 6 inline comments as done.
  • Fix typo and correct i18n markers
elvisangelaccio accepted this revision.Oct 13 2019, 3:25 PM
This revision is now accepted and ready to land.Oct 13 2019, 3:25 PM
This revision was automatically updated to reflect the committed changes.