Proof of Concept: Add double click actions to empty space in the folder view
Needs ReviewPublic

Authored by fbg13 on Apr 14 2019, 11:39 AM.

Details

Reviewers
elvisangelaccio
ngraham
Group Reviewers
Dolphin
VDG
Summary

FEATURE: 307505

Allows users to set an action to be triggered
when double clicking on an empty space in the folder view.
The possible actions are actions from

  • the actionCollection from dolphin main window (not all of them);
  • actions defined by the user (terminal command, scripts);
  • no action;

The current active view path can be passed to
the action/command defined by the user: {path}.

This is a proof of concept, there are things that can be improved,
but first I want to know if this is something wanted in Dolphin.

Demo:

Test Plan

Set an action in Dolphin settings > General.
Double clicking an empty space triggers the correct action.

Diff Detail

Repository
R318 Dolphin
Branch
empty-space-double-click
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14886
Build 14904: arc lint + arc unit
fbg13 created this revision.Apr 14 2019, 11:39 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptApr 14 2019, 11:39 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
fbg13 requested review of this revision.Apr 14 2019, 11:39 AM
ngraham added a subscriber: ngraham.

To be honest, I'm kind of torn on this. On one hand, it seems like a very powerful feature, and that experts could get a lot out of it.

Oh the other hand it seems like feature creep, and it's always a warning sign when a new optional feature is added that's off by default.

Since the requested feature from Windows Explorer was itself implemented as an add-on, maybe this should be a plugin too?

fbg13 added a comment.Apr 16 2019, 4:33 PM

Since the requested feature from Windows Explorer was itself implemented as an add-on, maybe this should be a plugin too?

Is there any documentation on plugin development for dolphin?
Do I have access to all the necessary parts (double click event, action collection, folder view, settings etc.).

I'm afraid I don't have the answers to those questions. :(

However note that I'm not formally objecting to this feature, just expressing a concern. If you want to keep it in Dolphin itself, maybe it should be on the Navigation page instead of General. What do you think?

fbg13 added a comment.Apr 16 2019, 6:16 PM

I don't think the navigation is a suitable place, since it's not limited only to going up one folder.

Here is a demo

ngraham requested changes to this revision.Aug 4 2019, 2:56 PM

Let's put this on the Navigation page. It's not a perfect fit, but I think makes sense to hide it there so it doesn't clutter up the General page. Also, most if the possible actions are navigation-related, and it's invoked by double-clicking in the view, which is a navigation area.

We need a UI to expose that the {path} token gets replaced with the actual path. Maybe a label below the text field that appears when using a custom action?

Finally, it feels quite slow. When I double-click, there's a noticeable delay before the action gets executed. Maybe this is because of all the scanning widgets to find the main window? That doesn't feel like the correct approach, just browsing the diff.

Oh and one last thing: every action seems to have an ampersand at the beginning of its name:

src/kitemviews/kitemlistcontroller.cpp
54

Put these new includes in the right places (e.g. Qt includes in the section with other Qt includes)

809

coding style: space after the if

src/kitemviews/kitemlistcontroller.h
307

I'd name this function doubleClickViewBackground

src/settings/general/behaviorsettingspage.h
32 ↗(On Diff #56182)

When you add new includes, maintain alphabetical ordering

This revision now requires changes to proceed.Aug 4 2019, 2:56 PM
fbg13 added a comment.Aug 4 2019, 5:13 PM

Finally, it feels quite slow. When I double-click, there's a noticeable delay before the action gets executed. Maybe this is because of all the scanning widgets to find the main window? That doesn't feel like the correct approach, just browsing the diff.

For me there is no delay, but I also don't like the loop. The alternatives I found are:

(DolphinMainWindow *)QApplication::activeWindow();
(DolphinMainWindow *)QApplication::topLevelAt(QCursor::pos());

Both work. Which should I use? If there's a better way let me know.

Oh and one last thing: every action seems to have an ampersand at the beginning of its name

That is the text set for the action.
I think Qt hides the & in widgets that support actions, but QComboBox doesn't support actions.
Since I just take the action text when creating the QComboBox item that is what is shown, will fix.


Also, since I'm at it, I think it will be good to add support for middle click too.
So there will be an action for double click and one for middle click.
What do you think?

fbg13 edited the summary of this revision. (Show Details)Aug 4 2019, 5:14 PM

The alternatives I found are:

(DolphinMainWindow *)QApplication::activeWindow();

Probably this one.

Also, since I'm at it, I think it will be good to add support for middle click too.
So there will be an action for double click and one for middle click.
What do you think?

How about going with just double-click for now, but implementing it in a generic enough way that middle-click support could be added in a second patch?

fbg13 added a comment.Aug 5 2019, 6:11 PM

The actions in the current patch were a few I selected for showcase.
These are the actions I think are useful to have for the double click:

"file_new" "New &Window"
"new_tab" "New Tab"
"edit_paste" "Paste Clipboard Contents..."
"edit_find" "&Find..."
"edit_select_all" "Select &All"
"split_view" "Split"
"editable_location" "Editable Location"
"replace_location" "Replace Location"
"go_back" "&Back"
"go_up" "&Up"
"go_home" "&Home"
"show_filter_bar" "Show Filter Bar"
"open_terminal" "Open Terminal"
"create_dir" "Create Folder..."
"show_preview" "Preview"
"show_in_groups" "Show in Groups"
"show_hidden_files" "Hidden Files"
"view_properties" "Adjust View Properties..."
"show_terminal_panel" "Terminal"

Thoughts?
@ngraham Do you think it still fits better in the Navigation page?

How about going with just double-click for now, but implementing it in a generic enough way that middle-click support could be added in a second patch?

Can you clarify this a bit? Do you want it added only if people ask for middle click too?

fbg13 updated this revision to Diff 63315.Aug 7 2019, 8:02 PM

make requested changes and some cleanup

anthonyfieroni added inline comments.
src/kitemviews/kitemlistcontroller.cpp
817

Check against nullptr parent() as well

818

Don't use C-style cast.

fbg13 added inline comments.Aug 8 2019, 5:54 AM
src/kitemviews/kitemlistcontroller.cpp
818
DolphinMainWindow *mainWindow = dynamic_cast<DolphinMainWindow *>(QApplication::activeWindow());
DolphinMainWindow *mainWindow = qobject_cast<DolphinMainWindow *>(QApplication::activeWindow());

The above don't work. But these do:

DolphinMainWindow *mainWindow = static_cast<DolphinMainWindow *>(QApplication::activeWindow());
KXmlGuiWindow *mainWindow = qobject_cast<KXmlGuiWindow *>(QApplication::activeWindow());

Which should I use?

anthonyfieroni added inline comments.Aug 8 2019, 5:56 AM
src/kitemviews/kitemlistcontroller.cpp
818

static_cast we know what the type of main window is, there is no need of dynamic dispatch.

fbg13 updated this revision to Diff 63330.Aug 8 2019, 6:08 AM

check parent() against nullptr, use static_cast when getting the DolphinMainWindow

There we go, now the action triggers instantly. Looking better!

src/settings/navigation/navigationsettingspage.cpp
113

I'd change this text to "Double-clicking in view background triggers action:"

fbg13 updated this revision to Diff 63374.Aug 8 2019, 8:44 PM

change label text

fbg13 marked 8 inline comments as done.Aug 8 2019, 8:45 PM
ngraham accepted this revision as: VDG, ngraham.Aug 8 2019, 9:13 PM

I'm good with the UI and functionality now. I'll let others take over for the rest of the code review and let Dolphin's maintainer @elvisangelaccio make the call regarding whether we should add the feature or not. Having used it now, I'm in favor because it has the potential to be an explosively powerful feature for power users and a major draw for the software among this crowd. But since it's hidden away on the Navigation page, I don't think it adds a lot of UI complexity for regular users.

Regardless, thank you very much for quite a lot of work and a very cool power user feature!

rizzitello added inline comments.
src/kitemviews/kitemlistcontroller.cpp
821

The Qt docs would say to use qobject_cast here in place of static_cast.

fbg13 added inline comments.Aug 9 2019, 1:06 PM
src/kitemviews/kitemlistcontroller.cpp
821

Well I asked which to use and have been told to use static_cast.

fbg13 added a comment.EditedAug 9 2019, 7:05 PM

Regarding qobject_cast this is the error I get:

DolphinMainWindow *mainWindow = qobject_cast<DolphinMainWindow *>(QApplication::activeWindow());
src/CMakeFiles/dolphinprivate.dir/kitemviews/kitemlistcontroller.cpp.o: in function `KItemListController::doubleClickViewBackground(Qt::MouseButton)':
Dolphin/src/kitemviews/kitemlistcontroller.cpp:-1: error: undefined reference to `DolphinMainWindow::staticMetaObject'
collect2: error: ld returned 1 exit status
[3/70 8.8/sec] Linking CXX shared library bin/libdolphinprivate.so.5.0.0
FAILED: bin/libdolphinprivate.so.5.0.0
fbg13 added a comment.Sun, Sep 1, 6:14 PM

So what's happening? Do I have to change static_cast to qobject_cast?
If so then I don't know how to fix the error mentioned above.

We are waiting for @elvisangelaccio's review.