D16019: Added missing icons to panel places items

[Alex Debus]

alexde added a comment.


  In D16019#338694 <https://phabricator.kde.org/D16019#338694>, @ngraham wrote:
  
  > 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 <https://github.com/KDE/kio/tree/master/src>. However, I found another cpp file which looked promising: <kio repo>/src/filewidgets/kfileplacesview.cpp <https://github.com/KDE/kio/blob/master/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 <https://github.com/KDE/dolphin/blob/master/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 <https://github.com/KDE/dolphin/blob/master/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?

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D16019

To: alexde, #vdg, ngraham
Cc: ngraham, acrouthamel, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20181008/7969a4cf/attachment.htm>