Select files instead of opening them when trying to open a QUrl as a directory when in fact it is a file.
Changes PlannedPublic

Authored by emateli on Mar 14 2018, 12:07 PM.

Details

Reviewers
ngraham
dfaure
elvisangelaccio
rkflx
Group Reviewers
Dolphin
Summary

When dolphin is executed with a parameter which happens to be a file, it will open a non existing directory and execute the file. This
patch changes the behaviour to instead select the file and open its parent directory.

This can occurr if you intentionally launch Dolphin with a file name as a parameter (without specifying --select), drag and dropping a file to its task bar icon or typing a file name instead of a directory at the url navigator.

This change is true for all files except archives, since due to a prior
patch(https://git.reviewboard.kde.org/r/119877/) all archives will be openened in a new tab.

Given that, I also propose a second change: To browse archives as a directory only when the appropriate setting("Open Archives as folders") is enabled.

Test Plan
  • Launch dolphin with a file as parameter or by dropping an icon to its task icon.

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D11304
Lint
No Linters Available
Unit
No Unit Test Coverage
emateli created this revision.Mar 14 2018, 12:07 PM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptMar 14 2018, 12:07 PM
emateli requested review of this revision.Mar 14 2018, 12:07 PM
broulik added inline comments.
src/dolphinviewcontainer.cpp
507 ↗(On Diff #29490)

Where do you select the file?

+1

Given that, I also propose a second change: To browse archives as a directory only when the appropriate setting("Open Archives as folders")

is enabled.
+2

ngraham accepted this revision.Mar 14 2018, 6:34 PM
ngraham added a reviewer: Dolphin.

+1, this seems like more expected behavior. Works fine with my tests:

  • Drag-and-drop file to Dolphin's icon
  • Drag-and-drop directory to Dolphin's icon
  • Drag-and-drop link to Dolphin's icon
  • Open dolphin on the command line with the path to a file
  • Open dolphin on the command line with the path to a directory
  • Open dolphin on the command line with the path to a link

Given that, I also propose a second change: To browse archives as a directory only when the appropriate setting("Open Archives as folders") is enabled.

+2

+1 to this as well.

This revision is now accepted and ready to land.Mar 14 2018, 6:34 PM
emateli added inline comments.Mar 14 2018, 10:11 PM
src/dolphinviewcontainer.cpp
507 ↗(On Diff #29490)

That is handled at DolphinViewContainer::slotUrlSelectionRequested which contains the original url.

emateli updated this revision to Diff 29542.Mar 14 2018, 10:24 PM
emateli edited the test plan for this revision. (Show Details)
  • Open archive in new tab only when the appropriate setting is selected.

I don't understand, this is a feature. Why would we want to remove it? If you want to select the file you can use --select, no?

I don't understand, this is a feature. Why would we want to remove it? If you want to select the file you can use --select, no?

Sure you can use --select, but this is for those instances where a file was passed but not the --select parameter, which may happen say when dropping stuff onto dolphin's icon or typing a file name in the kurlnavigator instead of a directory.

What feature am I missing here? That passing a file as a parameter to Dolphin will open the file using its default program and navigate to some non existing directory?

emateli edited the summary of this revision. (Show Details)Mar 16 2018, 8:56 AM

I don't understand, this is a feature. Why would we want to remove it? If you want to select the file you can use --select, no?

Sure you can use --select, but this is for those instances where a file was passed but not the --select parameter, which may happen say when dropping stuff onto dolphin's icon or typing a file name in the kurlnavigator instead of a directory.

What feature am I missing here? That passing a file as a parameter to Dolphin will open the file using its default program and navigate to some non existing directory?

@elvisangelaccio I think Emirald has a point here. What you refer to is perhaps better served by xdg-open, meaning that --select might ideally be only really relevant for folders in Dolphin's case.

I'm aware that dolphin -h outputs this:

Arguments:
  +[Url]                     Document to open

However, for me this makes sense only for folders and zip files (i.e. "compressed folders"). Or am I missing a use case here?

The feature is that you can open a file by typing its name in the url navigator, see e9463ffe2ac11019.

Sadly we don't know how many users are actually using this feature, but if we remove it someone could complain.

(Note that the fact that the dolphin view goes to a non-existing url is a bug, see Peter's comments here: https://bugs.kde.org/show_bug.cgi?id=301757#c6)

Sure you can use --select, but this is for those instances where a file was passed but not the --select parameter, which may happen say when dropping stuff onto dolphin's icon

@emateli Sounds like plasma should pass the --select paramater then?

The feature is that you can open a file by typing its name in the url navigator, see e9463ffe2ac11019.

That does sound like a feature we'd be breaking which I was unaware of.

@emateli Sounds like plasma should pass the --select paramater then?

I'm not sure why the shell should be aware on how to launch Dolphin with special parameters when the parameter is a file or a directory. The way it is now is consistent with all DE that i've used so far.

With that in mind however I'd like to propose the following:

  • We keep and fix the navigation from the KUrlNavigator. If a directory is typed in then the view will change, otherwise if it's a file, the application will launch it while remaining in the same directory. This IMO should work only for local files (and it's also much easier to implement, at that). If you type some ftp url it should not execute it (but with the current patch it will select it)
  • We change the behaviour of the parameter to what the patch proposes. No file execition when launched from the shell but rather selection.
  • When requesting to browse a file which is an archive it will be opened inside Dolphin only when the appropriate configuration is active, otherwise it will fallback to the default behaviour(run if via kurlnavigator) otherwise select. (This one I'm less convinced overall, but seems like a more consistent behaviour given that the BrowseThroughArchives setting exists)

Thoughts?

emateli updated this revision to Diff 31055.Mar 31 2018, 6:51 PM
This comment was removed by emateli.
emateli updated this revision to Diff 31056.Mar 31 2018, 6:55 PM

Revert to original state (Uploaded wrong diff via arcanist by using arc diff --update with the wrong differential number)

The feature is that you can open a file by typing its name in the url navigator, see e9463ffe2ac11019.

That does sound like a feature we'd be breaking which I was unaware of.

@emateli Sounds like plasma should pass the --select paramater then?

I'm not sure why the shell should be aware on how to launch Dolphin with special parameters when the parameter is a file or a directory. The way it is now is consistent with all DE that i've used so far.

With that in mind however I'd like to propose the following:

  • We keep and fix the navigation from the KUrlNavigator. If a directory is typed in then the view will change, otherwise if it's a file, the application will launch it while remaining in the same directory. This IMO should work only for local files (and it's also much easier to implement, at that). If you type some ftp url it should not execute it (but with the current patch it will select it)
  • We change the behaviour of the parameter to what the patch proposes. No file execition when launched from the shell but rather selection.
  • When requesting to browse a file which is an archive it will be opened inside Dolphin only when the appropriate configuration is active, otherwise it will fallback to the default behaviour(run if via kurlnavigator) otherwise select. (This one I'm less convinced overall, but seems like a more consistent behaviour given that the BrowseThroughArchives setting exists)

    Thoughts?

+1, sounds good!

@emateli Can you please click on "Plan Changes" if this review needs to be updated?

Restricted Application added a project: Dolphin. · View Herald TranscriptMay 26 2018, 4:23 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
emateli planned changes to this revision.May 27 2018, 6:30 PM
rkflx resigned from this revision.Aug 24 2018, 10:48 PM