Use placeholder for search action
ClosedPublic

Authored by ognarb on Mar 14 2019, 7:55 PM.

Diff Detail

Repository
R318 Dolphin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9717
Build 9735: arc lint + arc unit
ognarb created this revision.Mar 14 2019, 7:55 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 14 2019, 7:55 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
ognarb requested review of this revision.Mar 14 2019, 7:55 PM
ognarb edited the summary of this revision. (Show Details)Mar 14 2019, 7:56 PM
ognarb added reviewers: Dolphin, VDG.
yurchor added inline comments.
src/search/dolphinsearchbox.cpp
362

Should it be "Search..." or even "Search…" (GNOME has already switched to the Unicode ellipsis)?

Thanks in advance for your answer.

ognarb updated this revision to Diff 53920.Mar 14 2019, 8:29 PM

Use proper ellipsis not 3 points

ognarb marked an inline comment as done.Mar 14 2019, 8:42 PM
ognarb added inline comments.
src/search/dolphinsearchbox.cpp
362

Unicode ellipsis are better thanks ;)

ngraham requested changes to this revision.Mar 14 2019, 11:21 PM
ngraham added a subscriber: ngraham.

The menu item and the toolbar button say "Find". "Search" is really the correct term here, so we should change the text of the menu item and toolbar button to match it.

This revision now requires changes to proceed.Mar 14 2019, 11:21 PM

The menu item and the toolbar button say "Find". "Search" is really the correct term here, so we should change the text of the menu item and toolbar button to match it.

Interesting Dolphin uses the KStandardAction::Find enum (https://api.kde.org/frameworks/kconfigwidgets/html/namespaceKStandardAction.html#aa71041e1fa0a0c740e00b423ae684334a70ba4f3d9ae876b26883ea355627d236) . Should I create a non-standard action instead?

You should be able to override its text with something like action->setText(i18n("foo bar baz"));

The menu item and the toolbar button say "Find". "Search" is really the correct term here, so we should change the text of the menu item and toolbar button to match it.

Then it should be changed in kconfig/kconfigwidgets. But at least Firefox uses the term"Find" when I press CTRL+F, so I don't know...

elvisangelaccio requested changes to this revision.Mar 17 2019, 12:40 PM
elvisangelaccio added inline comments.
src/search/dolphinsearchbox.cpp
362

This will not be translatable, please read https://api.kde.org/frameworks/ki18n/html/prg_guide.html

ognarb updated this revision to Diff 54086.Mar 17 2019, 1:49 PM
  • Translate Search
  • Update doc
Restricted Application added a project: Documentation. · View Herald TranscriptMar 17 2019, 1:49 PM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
ognarb marked an inline comment as done.Mar 17 2019, 1:49 PM

Hmm, the menu and toolbar items still say "Find..."

src/search/dolphinsearchbox.cpp
362

I think we agreed in the VDG chat to use three periods instead of the actual ellipsis character for now.

ognarb updated this revision to Diff 54127.Mar 17 2019, 6:35 PM
  • Use 3 dots instead of unicode
  • Apply same change to filter bar
  • This time change Find to Search correctly
ognarb updated this revision to Diff 54128.Mar 17 2019, 6:37 PM

Use better variable name

ognarb edited the summary of this revision. (Show Details)Mar 17 2019, 6:39 PM
ognarb edited the summary of this revision. (Show Details)Mar 17 2019, 6:41 PM
ognarb marked 2 inline comments as done.

Now Dolphin doesn't compile:

/home/nate/kde/src/dolphin/src/dolphinmainwindow.cpp: In member function ‘void DolphinMainWindow::setupActions()’:
/home/nate/kde/src/dolphin/src/dolphinmainwindow.cpp:1145:5: error: invalid use of member function ‘void DolphinMainWindow::find()’ (did you forget the ‘()’ ?)
     find->setText(i18n("Search..."));
     ^~~~
/home/nate/kde/src/dolphin/src/dolphinmainwindow.cpp:1145:9: error: base operand of ‘->’ is not a pointer
     find->setText(i18n("Search..."));
         ^~
/home/nate/kde/src/dolphin/src/dolphinmainwindow.cpp:1144:14: warning: unused variable ‘search’ [-Wunused-variable]
     QAction *search = KStandardAction::find(this, &DolphinMainWindow::find, actionCollection());
              ^~~~~~

Looks like that should be search->setText(blablabla)

Make sure you're testing your changes! :)

ognarb updated this revision to Diff 54153.Mar 17 2019, 9:58 PM
ognarb edited the summary of this revision. (Show Details)

Now compile. Sorry :/

ngraham accepted this revision as: VDG, ngraham.Mar 17 2019, 10:02 PM

Thanks. :)

Let's wait for @elvisangelaccio now.

elvisangelaccio requested changes to this revision.Apr 20 2019, 3:18 PM

I still think that the rename from "Search" to "Find" should be done either upstream/globally or not at all.

Anyway, it's unrelated from this commit, which is just about moving to placeholders everywhere (which I agree with).

src/dolphinmainwindow.cpp
931–933

We shouldn't rename a standard action. From System Settings -> Global Shortcuts it will still be named "Find..."

This revision now requires changes to proceed.Apr 20 2019, 3:18 PM

I still think that the rename from "Search" to "Find" should be done either upstream/globally or not at all.

Anyway, it's unrelated from this commit, which is just about moving to placeholders everywhere (which I agree with).

Search and Find have different meanings because they do different things: locating files or items that match the given text in a data set of indeterminate or unclear size is "Search"; locating instances of text within the open document or page is "Find". But they should both have the same common ctrl+F keyboard shortcut. So it cannot be done globally.

Any idea how we move forward with this?

I still think that the rename from "Search" to "Find" should be done either upstream/globally or not at all.

Anyway, it's unrelated from this commit, which is just about moving to placeholders everywhere (which I agree with).

Search and Find have different meanings because they do different things: locating files or items that match the given text in a data set of indeterminate or unclear size is "Search"; locating instances of text within the open document or page is "Find". But they should both have the same common ctrl+F keyboard shortcut. So it cannot be done globally.

Any idea how we move forward with this?

Fair enough. Another thing I didn't consider is that the menubar is hidden by default, so most dolphin users probably never even see the text of the search action.

@ognarb Please rename the variables as mentioned inline and then we can ship it.

src/dolphinmainwindow.cpp
932

Please call this variable searchAction.

1144

And this one too.

ognarb updated this revision to Diff 56766.Apr 22 2019, 7:56 PM
ognarb marked 2 inline comments as done.

Rebase and rename search variable to searchAction. Still compile and run fine

ngraham accepted this revision.Apr 22 2019, 8:39 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 22 2019, 9:05 PM
This revision was automatically updated to reflect the committed changes.

JFYI it's generally considered polite to wait until all reviewers with an outstanding "Changes Requested" status to change it to "Accepted" before committing. :)