Added missing icons to panel places items
ClosedPublic

Authored by alexde on Oct 7 2018, 3:51 PM.

Details

Summary

Screenshots of the changes:

This contribution was motivated by a comment on Nate's blog.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
alexde created this revision.Oct 7 2018, 3:51 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptOct 7 2018, 3:51 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
alexde requested review of this revision.Oct 7 2018, 3:51 PM
alexde updated this revision to Diff 43055.Oct 7 2018, 4:29 PM

Changed the icon from "visibility" to "hint" as it describes the action better. Also added another missing icon to "Show all entries".

alexde retitled this revision from Added missing 'visibility' icons to panel places items to Added missing icons to panel places items.Oct 7 2018, 4:30 PM

Could you add some screenshots?

Also, it may not be best to choose a Breeze icon based on look, rather than name. Other icon themes may use something entirely different for hint.

alexde added a comment.EditedOct 7 2018, 4:39 PM

Could you add some screenshots?

Please see opening post.

Also, it may not be best to choose a Breeze icon based on look, rather than name. Other icon themes may use something entirely different for hint.

Maby you can help me with that, as I am not sure how to do that or where I can find information about it. :-)

alexde edited the summary of this revision. (Show Details)Oct 7 2018, 7:02 PM
alexde edited the summary of this revision. (Show Details)Oct 7 2018, 7:05 PM
ngraham accepted this revision.Oct 7 2018, 8:32 PM
ngraham added a subscriber: ngraham.

Also, it may not be best to choose a Breeze icon based on look, rather than name. Other icon themes may use something entirely different for hint.

For the moment this is probably the best we can do. Dolphin uses the same visibility/hint switcheroo for the Hidden Files action which I agree is not ideal. I've been mildly irritated by this myself when browsing the code, but never enough to do anything about it. I just tried submitting a patch to change it (D16028) but ran into arc problems and gave up.

For now, please stay with visibility and hint, this is fine.

Typically it's not ideal to use the same icon for two different adjacent things, but I think it's fine for now, and better than no icon.

This looks good! But there's one more thing...

Dolphin currently does not use the KIO code for parts of its places panel, so this change needs to be replicated for the KIO code so that Places panels in the open/save dialogs and Gwenview (etc.) get the change too. Can you submit a patch for KIO too? The code is in <kio repo>/src/panels/places/placespanel.cpp Thanks! Once you submit that patch and both are accepted, we can land them both.

This revision is now accepted and ready to land.Oct 7 2018, 8:32 PM

Fair enough, just something that I noticed after dealing with some of that with Gwenview.

Thanks for contributing @alexde!

shubham set the repository for this revision to R318 Dolphin.Oct 8 2018, 3:21 AM
shubham added a subscriber: shubham.EditedOct 8 2018, 3:28 AM

There is too much duplication in dolphin, which should't be there ideally. Why dolphin cant use the same copy from KIO , instead of its seperate copy?

There is too much duplication in dolphin, which should't be there ideally. Why dolphin cant use the same copy from KIO , instead of its seperate copy?

Because this hasn't been done yet: T9795: Use Places Panel code from KIO instead of private implementation

shubham removed a subscriber: shubham.Oct 8 2018, 4:08 AM
alexde added a comment.Oct 8 2018, 1:07 PM

Dolphin currently does not use the KIO code for parts of its places panel, so this change needs to be replicated for the KIO code so that Places panels in the open/save dialogs and Gwenview (etc.) get the change too. Can you submit a patch for KIO too? The code is in <kio repo>/src/panels/places/placespanel.cpp Thanks! Once you submit that patch and both are accepted, we can land them both.

Are you sure about the path and file? I could not find it in the kio repository. However, I found another cpp file which looked promising: <kio repo>/src/filewidgets/kfileplacesview.cpp.

There I would propose the following change:

diff --git a/src/filewidgets/kfileplacesview.cpp b/src/filewidgets/kfileplacesview.cpp
index eb41c6ae..b10eef70 100644
--- a/src/filewidgets/kfileplacesview.cpp
+++ b/src/filewidgets/kfileplacesview.cpp
@@ -742,7 +742,7 @@ void KFilePlacesView::contextMenuEvent(QContextMenuEvent *event)
     const bool clickOverHeader = delegate->pointIsHeaderArea(event->pos());
     if (clickOverHeader) {
         const KFilePlacesModel::GroupType type = placesModel->groupType(index);
-        hideSection = menu.addAction(i18n("Hide Section"));
+        hideSection = menu.addAction(QIcon::fromTheme(QStringLiteral("hint")), i18n("Hide Section"));
         hideSection->setCheckable(true);
         hideSection->setChecked(placesModel->isGroupHidden(type));
     }
@@ -778,7 +778,7 @@ void KFilePlacesView::contextMenuEvent(QContextMenuEvent *event)
             add = menu.addAction(QIcon::fromTheme(QStringLiteral("document-new")), i18n("Add Entry..."));
         }
 
-        hide = menu.addAction(i18n("&Hide Entry '%1'", label));
+        hide = menu.addAction(QIcon::fromTheme(QStringLiteral("hint")), i18n("&Hide Entry '%1'", label));
         hide->setCheckable(true);
         hide->setChecked(placesModel->isHidden(index));
         // if a parent is hidden no interaction should be possible with children, show it first to do so
@@ -790,7 +790,7 @@ void KFilePlacesView::contextMenuEvent(QContextMenuEvent *event)
 
     QAction *showAll = nullptr;
     if (placesModel->hiddenCount() > 0) {
-        showAll = new QAction(i18n("&Show All Entries"), &menu);
+        showAll = new QAction(QIcon::fromTheme(QStringLiteral("visibility")), i18n("&Show All Entries"), &menu);
         showAll->setCheckable(true);
         showAll->setChecked(d->showAll);
         if (mainSeparator == nullptr) {

Furthermore, I found that at some places in the Dolphin source code QStringLiteral is inconsistently used, for instance:

<dolphin repo>/src/panels/places/placespanel.cpp#L211

editAction = menu.addAction(QIcon::fromTheme("edit-entry"), i18nc("@item:inmenu", "Edit..."));

and
<dolphin repo>/src/panels/places/placespanel.cpp#L216

removeAction = menu.addAction(QIcon::fromTheme(QStringLiteral("edit-delete")), i18nc("@item:inmenu", "Remove"));

Which version is correct / preferred in which cases?

Argh, you are correct! I gave you the wrong path, sorry.

Using QStringLiteral("icon-name") is correct and it should be done everywhere. In places where this is not done, we should fix that. Good material for the next patch, perhaps? :)

ngraham closed this revision.Oct 9 2018, 9:28 PM