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

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.Sep 1 2019, 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.

Sorry for the long delay. I don't think we should add an option to let the user choose what to do on double clicks.

But I could accept a patch that triggers the "Go up" action because:

  • we have a feature request for that with a bunch of votes (#307505)
  • in the past we had already 2 different patches trying to implement this feature: 1, 2
fbg13 added a comment.Sep 29 2019, 8:01 PM

Sorry for the long delay. I don't think we should add an option to let the user choose what to do on double
But I could accept a patch that triggers the "Go up" action because:

  • we have a feature request for that with a bunch of votes (#307505)
  • in the past we had already 2 different patches trying to implement this feature: 1, 2

Is there a reason why you're against letting the user choose the action to trigger?
What if there was a feature request with people wanting this?
I'm asking since I made this patch for the custom action and not the go up one.

Another thing that I forgot: the double click trigger should only be enabled in single-click mode, probably.

Sorry for the long delay. I don't think we should add an option to let the user choose what to do on double
But I could accept a patch that triggers the "Go up" action because:

  • we have a feature request for that with a bunch of votes (#307505)
  • in the past we had already 2 different patches trying to implement this feature: 1, 2

Is there a reason why you're against letting the user choose the action to trigger?

Adding a new setting makes the code and the UI more complex and harder to maintain in the long term. We should add new settings only when there is no other way (this usually happens if a new feature makes sense only for a subset of users).

What if there was a feature request with people wanting this?

Then we might reconsider this choice, of course.

I don't see a compelling reason why this feature shouldn't work when you're using the systemwide double-click mode. I use that mode and this feature works perfectly fine with it.

fbg13 added a comment.Oct 6 2019, 7:45 PM

Adding a new setting makes the code and the UI more complex and harder to maintain in the long term. We should add new settings only when there is no other way (this usually happens if a new feature makes sense only for a subset of users).

But even if we go with just the "Go up" action, I think there should be an option to turn it off.
I often miss the file or folder when double clicking and going up would be annoying.

Then we might reconsider this choice, of course.

I asked on r/kde if there's interest for this feature and the response was good.
https://www.reddit.com/r/kde/comments/de4sun/dolphin_are_you_interested_in_a_double_click_on/

@elvisangelaccio Hope that's enough.

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

You should use static_cast only if you know 100% sure the type. But in this case the line below even checks against nullptr. So it seems you are not sure what you get. That's why using qobject_cast is the better/correct approach here.

836

const missing, otherwise the range based for loop detaches.

src/settings/navigation/navigationsettingspage.cpp
64

Wrap with QStringLiteral to avoid memory allocations.

Adding a new setting makes the code and the UI more complex and harder to maintain in the long term. We should add new settings only when there is no other way (this usually happens if a new feature makes sense only for a subset of users).

But even if we go with just the "Go up" action, I think there should be an option to turn it off.
I often miss the file or folder when double clicking and going up would be annoying.

If a feature can be triggered by accident, we should fix that instead of adding an option to disable the feature. In this case, the double click should trigger the action only if it's not "near an item".

Then we might reconsider this choice, of course.

I asked on r/kde if there's interest for this feature and the response was good.
https://www.reddit.com/r/kde/comments/de4sun/dolphin_are_you_interested_in_a_double_click_on/

@elvisangelaccio Hope that's enough.

Interesting. I personally like the idea of "selecting all" on double click. I also see very good arguments in favor of not adding the feature as-is ;)

fbg13 added a comment.Dec 18 2019, 6:38 PM

So is this (the feature not the current patch) going to be accepted or not?

elvisangelaccio added a comment.EditedDec 21 2019, 6:11 PM

Not in the current version, I'm afraid.

As already said, we need a very good reason to justify the addition of a new setting that will make dolphin harder to maintain in the long-run.

TBH the "increased maintenance burden" argument doesn't sit well with me. Every new feature or net addition to the code adds to the maintenance burden. In and of itself, this is not a problem; you have to weigh it against the value brought by the addition or change in question. We should be thinking about whether this feature is worth adding. If we find that it is, we'll find ways to offset or reduce or deal with the increased maintenance burden, the way we do for all net code additions.

fbg13 abandoned this revision.Dec 26 2019, 5:15 AM
maxts added a subscriber: maxts.Jan 26 2020, 7:20 AM

TBH the "increased maintenance burden" argument doesn't sit well with me. Every new feature or net addition to the code adds to the maintenance burden. In and of itself, this is not a problem; you have to weigh it against the value brought by the addition or change in question. We should be thinking about whether this feature is worth adding. If we find that it is, we'll find ways to offset or reduce or deal with the increased maintenance burden, the way we do for all net code additions.

What can be a better value then improving user experience? In most of Linux DEs middle mouse click is set to work as Ctrl+V, so isn't it great to provide the same functionality in Dolphin?
Double click action would be a very neat addition too. It can be seen from this discussion on Reddit, how many people actually liked this idea and will be interested in using it...
I think this feature suggestion must be reconsidered.

https://bugs.kde.org/show_bug.cgi?id=307505 has two duplicates. We might want to reconsider.

maxts added a comment.Apr 1 2020, 12:57 AM

https://bugs.kde.org/show_bug.cgi?id=307505 has two duplicates. We might want to reconsider.

Any updates on this?

meven added a subscriber: meven.Apr 1 2020, 12:53 PM

! In D20532#601023, @ngraham wrote:
https://bugs.kde.org/show_bug.cgi?id=307505 has two duplicates. We might want to reconsider.

I agree with Nate, I think we should reconsider, based on user feedback.
This is a power-user feature that should please this part of our user base.

One use case appealed a lot to me in https://www.reddit.com/r/kde/comments/de4sun/dolphin_are_you_interested_in_a_double_click_on/ : the touchpad and touchscreen use case.
Touchpad mouse are slow to move and are less precise, this would make sense for those use case.

Plus the code is not that scary (just a few corrections left)

So @elvisangelaccio Has your opinion changed ?

src/kitemviews/kitemlistcontroller.cpp
821

Move to line 835.

+1, I agree

emvaized added a subscriber: emvaized.EditedApr 13 2020, 9:20 PM

@elvisangelaccio
Please make it happen. This feature is very useful - especially middle button click on empty space, which can be assigned to "Paste file" function, as mentioned above.

ashark added a subscriber: ashark.Apr 25 2020, 11:27 PM

I came here because Nate closed the bug 307505 with link to this discussion.
I wanted only go up by double click action. I see I commented the bug in 2018 january, but actually, I wanted it earlier. And I still want it very much.
But actually, adding more general feature with ability to choose action looks nice to me. Please, reconsider, at least for level up action.

lbenes added a subscriber: lbenes.Thu, May 7, 5:13 PM

I came here because Nate closed the bug 307505 with link to this discussion.

Adding ME TOO comments to a code review tool is not going to make this feature happen. There are valid concerns with implementing this issue. So unless you can help with the maintenances, you're just going to have to wait.