Use Places Panel code from KIO instead of private implementation
Closed, WontfixPublic

Description

KIO has its own code to render a Places Panel. It's used by the file dialogs and Gwenview (maybe more places too, ha ha). But Dolphin uses its own code its Places panel.

We should upstream any Dolphin-specific features and then use KIO's code instead. Otherwise features for one will need to be duplicated for the other, which is a pain (see for example D15929).

ngraham created this task.Oct 3 2018, 10:30 PM
ngraham triaged this task as Wishlist priority.

One thing that strikes me is that a lot of the PlacesItemModel code is just proxying KFilePlacesModel. Another large part of the code seems entirely duplicated. It would make sense to check if we can just use KFilePlacesModel directly in the Places Panel. @renatoo did a larger refactoring of this (D8855), maybe he can offer some insights on this.

One thing that strikes me is that a lot of the PlacesItemModel code is just proxying KFilePlacesModel. Another large part of the code seems entirely duplicated. It would make sense to check if we can just use KFilePlacesModel directly in the Places Panel.

I don't think we can. PlacesItemModel is a KStandardItemModel, which is a part of the Dolphin-specific view engine and is not available in KIO.

Zren added a subscriber: Zren.Mar 5 2019, 3:27 AM
Zren added a comment.Mar 5 2019, 6:41 AM

Here's a quick and dirty proof of concept. Selecting a place works, but I still need to bind:

  • middle mouse click
  • selecting a pinned place when you navigate in the file view
  • selecting the first item on init? selectClosestItem()

And also remove the old "PlaceView/PlaceModel".

diff --git a/src/panels/places/placespanel.cpp b/src/panels/places/placespanel.cpp
index d0dbcce88..d7633d133 100644
--- a/src/panels/places/placespanel.cpp
+++ b/src/panels/places/placespanel.cpp
@@ -39,6 +39,7 @@
 
 #include <KFilePlaceEditDialog>
 #include <KFilePlacesModel>
+#include <KFilePlacesView>
 #include <KIO/DropJob>
 #include <KIO/EmptyTrashJob>
 #include <KIO/Job>
@@ -138,9 +139,15 @@ void PlacesPanel::showEvent(QShowEvent* event)
         KItemListContainer* container = new KItemListContainer(m_controller, this);
         container->setEnabledFrame(false);
 
+        KFilePlacesModel *model = new KFilePlacesModel(this);
+        KFilePlacesView *placesView = new KFilePlacesView(this);
+        placesView->setModel(model);
+        connect(placesView, &KFilePlacesView::urlChanged, this, &PlacesPanel::placeActivated);
+
         QVBoxLayout* layout = new QVBoxLayout(this);
         layout->setContentsMargins(0, 0, 0, 0);
-        layout->addWidget(container);
+        // layout->addWidget(container);
+        layout->addWidget(placesView);
 
         selectClosestItem();
     }


Not sure if I need to wrap the url passed into placeActivated(...) like it does in placespanel.cpp. It seems to parse the search:/documents urls and convert the scheme to baloosearch:/documents.

if (button == Qt::MiddleButton) {
    emit placeMiddleClicked(KFilePlacesModel::convertedUrl(url));
} else {
    emit placeActivated(KFilePlacesModel::convertedUrl(url));
}

Awesome work so far, @Zren! I hope all goes well!

Zren added a comment.Mar 6 2019, 10:53 PM

After a deeper look into this:

Summary: There's a number of features that need to be refactored for a final KFilePlacesView patch. A middle mouse click event needs to be implemented. The "open in new tab/window" wouldn't be applicable for the upstream KIO filewidget, so we'd need to monkeypatch the contextmenu (placing it under "Unmount" and above "Properties"). We need to upstream the icon size submenu, or add at least upstream the ability to change the icon size and monkey patch the submenu into the contextmenu. The drop file into pinned folder needs to be refactored to support a folder that needs to be "setup" first.


Note: KFilePlacesView::contextMenuEvent calls QAction *result = menu.exec(event->globalPos()); in a blocking manner it seems, so we won't be able to monkey patch the context menu without modifying the KFilePlacesView to add a "before show" event or overloading the event.

  • itemContextMenuRequested
    • "Mount" / "Unmount" / "Safely Remove 'USB Device'"
    • I need to bind "Mount" / "Unmount" error messages.
    • Open in New Window is missing
    • Open in New Tab is missing
    • Properties
    • "Edit" => "Edit Entry 'Trash'"
    • "Hide" => "Hide Entry 'Trash'"
    • "Hide Section" was moved to the contextmenu that appears when you click the "heading".
  • viewContextMenuRequested
    • "Icon Sizes" is not a feature of KFilePlacesView, so we need to patch the KIO widget to support different icon sizes at the very least. We'll need to monkey patch the contextmenu if the setting isn't added to the upstream KFilePlacesView.
    • "Show Hidden Places" => "Show All Entries"
  • selecting a pinned place (selectClosestItem())
    • the initial url in the addressbar on init
    • when you navigate in the file view
  • Drag and Drop
    • Drag folder from file view to pin an item
    • Drag file into pinned folder
      • Needed to set KFilePlacesView::setDropOnPlaceEnabled(true)
      • Need to bind to KFilePlacesView::urlsDropped to PlacesPanel::slotUrlsDropped
      • Unfortunately it's not as easy as that, as Dolphin checks if the dropped folder needs storageSetupNeeded, so I'll need to refactor the drop code.
    • Not sure what the existing behavior is for "drag a pinned item" onto
      • the tab bar
      • the file view
  • The "Add to places" from a file view folder contextmenu updates the places panel.
Zren added a comment.May 20 2019, 5:32 PM

I've personally abandoned attempts to merge KIO's Places view in Dolphin for now. Upstreaming those features to KIO is good code practice in the long term though. I've copied the existing KFilePlacesView from KIO and embeded them in this dolphin branch for easy development:

https://github.com/Zren/dolphin/compare/af32a92...kioplacesfork

The main feature that sets KIO apart from Dolphin is the disk capacity bars for the devices. I plan on submitting a patch that basically draws 2 rectangles underneath the text in PlacesItemWidget.

elvisangelaccio closed this task as Wontfix.Sep 10 2019, 9:08 PM
elvisangelaccio claimed this task.

This task was discussed at today's Dolphin Bof at Akademy. It was agreed that there are techical limitations (custom view-engine) and also possible licensing issues (from GPL to LGPL).

So we are going to be stuck with the current situation, which is not so bad after all (dolphin is mostly proxying the KIO places panel, we no longer have the huge code duplication we had some years ago).