Add option to allow execution as well as opening of scripts and desktop files

Reviews
Diff

- Download Diff

Information
Submitter:	Arjun AK
Repository:	kde-baseapps
Branch:	master
Bugs:	275405
Depends On:
Reviewers
Groups:	dolphin, usability
People:	dfaure

Description

This patch allows the user to execute (in background, within the embedded terminal, external terminal) or open an executable file (scripts, desktop files...). When the user clicks on an executable file, a dialog pops up which contains options to either open it or execute it. Clicking on execute runs the program in the background (dolphin's current behaviour).

The settings dialog has further options to fine-tune how the file is executed. The program can either be executed in the background or inside the embedded konsole or in an external terminal.

In case there is a process already running in the embedded terminal, with user's permission it will be terminated and the selected program will be started.

See also:
https://git.reviewboard.kde.org/r/118939/

Testing Done

Files

Issues

1
11
17
All Issues: 29

Description	From	Last Updated
missing space after if	David Faure	Sept. 15, 2014, 8:38 p.m.
mime->name() is simpler to read	David Faure	Sept. 15, 2014, 8:38 p.m.
should be mime->is("application/x-desktop") to take inheritance into account (just in case)	David Faure	Sept. 15, 2014, 8:38 p.m.
strange indentation, no? (I don't know the dolphin coding style though)	David Faure	Sept. 15, 2014, 8:38 p.m.
the nested switch makes the method a bit hard to read, maybe split the whole handling of "always ask" into ...	David Faure	Sept. 15, 2014, 8:38 p.m.
I guess you mean url.toLocalFile() (path and toLocalFile do the same on Unix for local paths but not on Windows, ...	David Faure	Sept. 15, 2014, 8:38 p.m.
toLocalFile() (in all cases this assumes that the url is local, so either assert that here or verify callers)	David Faure	Sept. 15, 2014, 8:38 p.m.
Description please	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Remove the empty line	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Just remove the KRun pointer "run" if you don't need it.	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
switch () { case Open: { ... break; } case AlwaysAsk: ...; break; case Execute: ...; break; }	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Remove the unused KRun pointer + curly brackets	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Maybe you want a scoped pointer instead? See http://qt-project.org/doc/qt-5/qscopedpointer.html If you use the scoped pointer you can remove the "delete ...	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
.data() is not needed	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
switch () { case Open: { ... break; } case AlwaysAsk: ...; break; case Execute: ...; break; }	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Remove unused KRun* run	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
I think we should name this signal "fileActivated" instead of "fileClicked".	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
only 1 space between	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
revert the unrelated new line change	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Remove empty new line	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
m_ prefix	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Can be removed	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Can be removed	Emmanuel Pescosta	Oct. 28, 2014, 2:39 p.m.
Why by file content and not the more general findByFile, which also takes extensions into account? Users generally name things ...	David Faure	Oct. 28, 2014, 2:39 p.m.
Nitpick: One space too much	Emmanuel Pescosta	Oct. 31, 2014, 6:15 p.m.
The m_confirmScriptExecution check isn't needed, it is guaranteed that the pointer is valid. Maybe you mean m_confirmScriptExection->isChecked() instead? Can you ...	Emmanuel Pescosta	Oct. 31, 2014, 6:12 p.m.
I think we should set it to alwaysAsk instead of deleting it, makes it more obvious.	Emmanuel Pescosta	Oct. 31, 2014, 6:12 p.m.
const	Emmanuel Pescosta	Oct. 31, 2014, 6:12 p.m.
Final nitpick: please use a proper name instead of value, behaviourOnLaunch maybe?	Emmanuel Pescosta	Oct. 31, 2014, 7:15 p.m.

Change Summary:

Quote the command

Diff:

Revision 2 (+348 -5)

Show changes

	dolphin/src/CMakeLists.txt
	dolphin/src/dolphinmainwindow.h
	dolphin/src/dolphinmainwindow.cpp
	dolphin/src/dolphinviewcontainer.h
	dolphin/src/dolphinviewcontainer.cpp
	dolphin/src/panels/terminal/terminalpanel.h
	dolphin/src/panels/terminal/terminalpanel.cpp
	dolphin/src/settings/dolphin_generalsettings.kcfg
	4 more

Change Summary:

Oops, missed a return statement. Also revert to dolphin's old behaviour when opening remote files.

Diff:

Revision 3 (+348 -5)

Show changes

	dolphin/src/CMakeLists.txt
	dolphin/src/dolphinmainwindow.h
	dolphin/src/dolphinmainwindow.cpp
	dolphin/src/dolphinviewcontainer.h
	dolphin/src/dolphinviewcontainer.cpp
	dolphin/src/panels/terminal/terminalpanel.h
	dolphin/src/panels/terminal/terminalpanel.cpp
	dolphin/src/settings/dolphin_generalsettings.kcfg
	4 more

The general direction looks ok to me (but the dolphin maintainer will have the final word on that). Some comments.

dolphin/src/dolphinmainwindow.cpp (Diff revision 3)

Show all issues

missing space after if

dolphin/src/dolphinmainwindow.cpp (Diff revision 3)

Show all issues

mime->name() is simpler to read

dolphin/src/dolphinmainwindow.cpp (Diff revision 3)

Show all issues

should be mime->is("application/x-desktop") to take inheritance into account (just in case)

dolphin/src/dolphinmainwindow.cpp (Diff revision 3)

Show all issues

strange indentation, no? (I don't know the dolphin coding style though)

dolphin/src/dolphinmainwindow.cpp (Diff revision 3)

Show all issues

the nested switch makes the method a bit hard to read, maybe split the whole handling of "always ask" into a separate method?

dolphin/src/dolphinmainwindow.cpp (Diff revision 3)

Show all issues

I guess you mean url.toLocalFile()
(path and toLocalFile do the same on Unix for local paths but not on Windows, e.g. file:///c:/a.txt leads to path=/c:/a.txt (directly extracted from the path component of the URL) and toLocalFile=c:/a.txt (which QFile can use))

dolphin/src/panels/terminal/terminalpanel.cpp (Diff revision 3)

Show all issues

toLocalFile()
(in all cases this assumes that the url is local, so either assert that here or verify callers)

Arjun AK Sept. 15, 2014, 8:42 p.m.

Remote files aren't handled by this. I have added an assert anyway.

I had planned to work on this for quite some time, but there was always something that looked more urgent, so I never got round to it. I apprecitate that you took this effort now, and I'm sure that many users will appreciate it too!

I strongly recommend to add the usability team as reviewers to this request though, to discuss with them what the best solution for this issue is. I am not sure if we really want to provide that many options to the user. Adding more options always looks great at first sight, but they may confuse the user, make the settings dialog less usable, and they add more code paths, which make the long-term maintenance harder and possibly lead to lots of new subtle bugs.

In particular, the "embedded terminal" and "external terminal" options offer very similar functionality, so it seems questionable if we really want both. Even more so if one of them ("embedded") requires more changes to the code and a new "do you want to replace the running program" dialog, and will most likely introduce new bugs (in my experience, any change to the terminal panel is likely to cause unexpected problems, which may be very hard to fix, see, e.g., https://bugs.kde.org/show_bug.cgi?id=305085 ).

Moreover, I think it might be worth considering to offer all options in the "What do you want to do" dialog already (Execute/Open/Open in terminal/Cancel), but these are just my 2 cents - this should really be discussed with the usability team.

Arjun AK Sept. 15, 2014, 8:43 p.m.

In particular, the "embedded terminal" and "external terminal" options offer very similar functionality

The embedded terminal can execute only one program at a time. User will have to wait for it finish execution before he can run another one.

Thomas Pfeiffer Sept. 15, 2014, 9:32 p.m.

"Usability person" here, thanks for pinging us.

Let's see what we're trying to accomplish here.
My take is that this feature is foremost useful for executables which do not need any further input from the user (and the user would not need the stdout output). Just click it, run it, done.
If that is what we're aiming for, then just defaulting to "background" is completely sufficient.
If people want to run the application interactively, they'd have to go the traditional route and enter the command directly in the terminal.

If we also want to support interactive shell execution with this, then of course we'd need the option to run it in a visible terminal. However, I don't see this being a global choice. People would still run executables that don't need any interaction in the background, but run those that need further input (or where they need to see the output) in a visible terminal.

Therefore, I agree with Frank that the "Execute in" sub-options add to the complexity of the UI without adding much value.

Personally, I would suggest aiming this feature only at executables that can run in the background just fine, and have users type it into the terminal themselves if they need additional input or output.
If you absolutely want to support input/output, I fear this choice has to be made in the dialog itself (so it would only work with the "Always Ask" option to be useful.

Heiko Tietze Sept. 16, 2014, 6:59 a.m.

I'm not sure that your considerations meet all use cases. Or perhaps I don't understand the feature at all. What is an 'executable' that opens as a file? I guess a document that has been registered to an application, or a file with executable flag set. And if I chose "Open file" what happens in case of a real executable? In respect to use cases: Doesn't users want to configure such behavior for every type of file/document individually?

However, I have two comments to the prototypes:

The text "A program is running. Do you want to replace it?" might be misleading. Use the real program name, if possible, and text something like "Do you want to stop/kill/terminate it?" Perhaps users want to wait for the process too (and build a stack of executions thereby).

Second comment is about the configuration. The subordinate radio button group looks weird and is not according to the HIG style. A solution would be to put all suboptions into a dropdown and place this right of the "execute in" item. And, of course, disable the dropdown unless the "Execute in" option is selected.

Arjun AK Sept. 16, 2014, 9:53 a.m.

What is an 'executable' that opens as a file.

Shell scripts

The text "A program is running. Do you want to replace it?" might be misleading. Use the real program name, if possible, and text something like "Do you want to stop/kill/terminate it?" Perhaps users want to wait for the process too (and build a stack of executions thereby).

All those features have been removed. See the fifth revision.

Is this on the right layer in Dolphin? What about Folder View in Plasma?

Arjun AK Sept. 15, 2014, 8:43 p.m.

Is this on the right layer in Dolphin

What do you suggest?

Kai Uwe Broulik Sept. 16, 2014, 2:40 p.m.

Moving that to KIO which usually does prompts like these(?)

Emmanuel Pescosta Sept. 16, 2014, 6:35 p.m.

I also think that moving this to KIO makes a lot of sense.

@David: Would you accept this in KIO?

Change Summary:

Fixed mentiond issues.

Diff:

Revision 4 (+241 -5)

Show changes

	dolphin/src/CMakeLists.txt
	dolphin/src/dolphinmainwindow.h
	dolphin/src/dolphinmainwindow.cpp
	dolphin/src/dolphinviewcontainer.h
	dolphin/src/dolphinviewcontainer.cpp
	dolphin/src/panels/terminal/terminalpanel.h
	dolphin/src/panels/terminal/terminalpanel.cpp
	dolphin/src/settings/dolphin_generalsettings.kcfg
	2 more

Groups:

+	usability

Change Summary:

Removed execute in external and embedded terminal

Diff:

Revision 5 (+270 -3)

Show changes

	dolphin/src/CMakeLists.txt
	dolphin/src/dolphinmainwindow.h
	dolphin/src/dolphinmainwindow.cpp
	dolphin/src/dolphinviewcontainer.h
	dolphin/src/dolphinviewcontainer.cpp
	dolphin/src/settings/dolphin_generalsettings.kcfg
	dolphin/src/settings/general/behaviorsettingspage.h
	dolphin/src/settings/general/behaviorsettingspage.cpp
	2 more

dolphin/src/dolphinmainwindow.h (Diff revision 5)

Show all issues

Description please

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

Remove the empty line

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

Just remove the KRun pointer "run" if you don't need it.

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

switch () {

case Open: {

    ...

    break;

}

case AlwaysAsk:

    ...;

    break;
case Execute:

    ...;

    break;

}

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

Remove the unused KRun pointer + curly brackets

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

Maybe you want a scoped pointer instead? See http://qt-project.org/doc/qt-5/qscopedpointer.html

If you use the scoped pointer you can remove the "delete dialog;" at the end of this method

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

.data() is not needed

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

switch () {

case Open: {

    ...

    break;

}

case AlwaysAsk:

    ...;

    break;
case Execute:

    ...;

    break;

}

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

Remove unused KRun* run

dolphin/src/dolphinviewcontainer.h (Diff revision 5)

Show all issues

I think we should name this signal "fileActivated" instead of "fileClicked".

dolphin/src/settings/general/behaviorsettingspage.h (Diff revision 5)

Give them better names please.

dolphin/src/settings/general/behaviorsettingspage.cpp (Diff revision 5)

Show all issues

only 1 space between

dolphin/src/settings/general/behaviorsettingspage.cpp (Diff revision 5)

Show all issues

revert the unrelated new line change

dolphin/src/settings/general/behaviorsettingspage.cpp (Diff revision 5)

Show all issues

Remove empty new line

dolphin/src/views/executablefileopendialog.h (Diff revision 5)

Show all issues

m_ prefix

dolphin/src/views/executablefileopendialog.cpp (Diff revision 5)

Show all issues

Can be removed

dolphin/src/views/executablefileopendialog.cpp (Diff revision 5)

Show all issues

Can be removed

I have found a problem with your patch. The "executable file open dialog" doesn't only appear for
executable scripts, it appears for all different kinds of executables, which is wrong I guess.

So you have to check if the file is an executable text file.

Emmanuel Pescosta Sept. 16, 2014, 6:56 p.m.

So all for today!

Arjun, thanks for working on this, I think a lot of people will appreciate these changes! ;)

Arjun AK Sept. 19, 2014, 12:03 p.m.

it appears for all different kinds of executables, which is wrong I guess.

One of the main reasons for adding this is to prevent the user from accidently running an executable file (especially if you have 'single click to open' enabled). I guess in case the file being opened is a binary file, the open button should be disabled.

Emmanuel Pescosta Sept. 20, 2014, 8:25 a.m.

prevent the user from accidently running an executable file

IMHO we should do that only for scripts.

dolphin/src/settings/general/behaviorsettingspage.cpp (Diff revision 5)

@usability team:
What do you think about the label and radio buttons text?

e.g. Nautilus has the following settings:
Executable Text Files
(*) Run executable text files when they are opened
( ) View executable text files when they are opened
( ) Ask each time

Heiko Tietze Sept. 16, 2014, 7:09 p.m.

Why don't you call the 'executable text file' just a script? That fits better the non-Linux variants, as a side-effect. And since options are subordinate you can shorten the caption a little bit:

Behavior for scripts on opening
(x) Execute/Run
( ) View/Edit
( ) Always ask

Alternatively, you can apply a dropdown and have something like
[Execute] scripts on opening

Yep, this could go into KIO for sharing between file managers.
I'm not sure how/where though; probably KRun. But the real question is whether the config should be shared between apps or not.

It's easier to code, but it might be better to let each app handle the configuration part of it, otherwise we have more weird "shared settings which don't belong in any app's KCM because it's unexpected that they affect all apps", right?

Well, one solution is using KGlobal::config() (KSharedConfig::openConfig in KF5) in KRun and letting apps write to that.

Arjun AK Sept. 19, 2014, 2:35 p.m.

But if 'KRun::setRunExecutables()' is set to true, then the file should definitely be executed. If not, it should be opened in another app. In either of these cases, there is no point in prompting the user on what to do with the file, since we have already specified that.

David Faure Sept. 19, 2014, 7:50 p.m.

OK, so one layer above that. A new class, say KIO::ScriptLauncher class (deriving from QObject), which takes care of showing an internal dialog if necessary, in a NON-MODAL way (unlike the current code), and to call a slot in the script launcher class which takes care of doing what the user chose -- i.e. basically the equivalent of DolphinMainWindow::openExecutableFile, but split into a constructor and a slot, so that apps are not forced to use a modal dialog. Plasma developers will appreciate this very much. Then dolphin can be ported to "new KIO::ScriptLauncher". (auto-deletion after use, like jobs and like KRun, is probably the best way of doing it)
Ah this functionality includes desktop files, it seems? Then maybe not "Script" but "ExecutableFileLauncher" to keep your current naming. But to the unaware reader it sounds like the way to launch any program, while it's specific to executable text files....

ExecutableTextFileLauncher ?

Arjun AK Sept. 20, 2014, 11:02 a.m.

How about having a KRun::promptUser(bool) function, which defaults to false, but if set shows the dialog box? But if you move it to KIO, how do you handle "Do not ask again" checkbox?

David Faure Oct. 18, 2014, 10:07 a.m.

I would much rather see something non modal (as I said, for plasma), and separated (KRun is getting a bit bloated).

KIO and other frameworks have plenty of "do not ask again"; indeed the setting is just global across all apps in general, but that's easily fixed with a dontAskAgain name like "run_executables_" + QCoreApplication::name()

Arjun AK Oct. 18, 2014, 6:11 p.m.




and separated (KRun is getting a bit bloated).

Do we really need an entirely new class that does the exact same thing as KRun with the only difference being that it shows a dialog?

David Faure Oct. 19, 2014, 8:11 a.m.

The idea was of course to end up calling KRun, not to duplicate anything.

But I see what you mean, from an API point of view it doesn't sound very streamlined for apps.

OK, put it in KRun.

dolphin/src/dolphinmainwindow.cpp (Diff revision 5)

Show all issues

Why by file content and not the more general findByFile, which also takes extensions into account? Users generally name things properly, better trust them.

Change Summary:

Use a single checkbox in Settings->General->Confirmations->"Ask for confirmation in all KDE applications"

Diff:

Revision 6 (+16)

Show changes

	dolphin/src/settings/general/confirmationssettingspage.h
	dolphin/src/settings/general/confirmationssettingspage.cpp

Change Summary:

Added screenshot

Added Files:

So in Plasma we have icon launcher which uses KRun to open the applications http://quickgit.kde.org/?p=plasma-workspace.git&a=blob&h=a2a3f81d48ff72543d97c70b5e457b75087310b3&hb=f8e1012c7f1c9a8adfb42ff161ebec379e24a037&f=applets%2Ficon%2Fplugin%2Ficon_p.cpp

Now in case of launchers this is obvious that one want to launch that application and not do something else, so can we add some option to disable this confirmation box in KRun?

David Faure Oct. 28, 2014, 10:02 p.m.

Urgh, yes, the open-or-execute behavior only makes sense in file managers.

So the default in KRun should probably be to always execute (as it was before), and a setter should be added for filemanagers to enable the open-or-execute logic.

dolphin/src/settings/general/confirmationssettingspage.cpp (Diff revision 6)

Show all issues

Nitpick: One space too much

dolphin/src/settings/general/confirmationssettingspage.cpp (Diff revision 6)

Show all issues

The m_confirmScriptExecution check isn't needed, it is guaranteed that the pointer is valid. Maybe you mean m_confirmScriptExection->isChecked() instead?

Can you please move the scriptExecutionGroup below the confirmationGroup sync?

And you need to sync the scriptExecutionGroup as well.

Arjun AK Oct. 31, 2014, 6:30 p.m.




Maybe you mean m_confirmScriptExection->isChecked() instead?

Yep, thats what i meant. My bad.

dolphin/src/settings/general/confirmationssettingspage.cpp (Diff revision 6)

Show all issues

I think we should set it to alwaysAsk instead of deleting it, makes it more obvious.

dolphin/src/settings/general/confirmationssettingspage.cpp (Diff revision 6)

Show all issues

const

Diff:

Revision 7 (+28 -1)

Show changes

	dolphin/src/dolphinviewcontainer.cpp
	dolphin/src/settings/general/confirmationssettingspage.h
	dolphin/src/settings/general/confirmationssettingspage.cpp

Ship it! Fix it!

Thanks for working on this, awesome! :D

dolphin/src/dolphinviewcontainer.cpp (Diff revision 7)

Show all issues

Final nitpick: please use a proper name instead of value, behaviourOnLaunch maybe?

dolphin/src/dolphinviewcontainer.cpp (Diff revision 7)

Hmm ... this is already done by KRun, isn't it?
KRun *run = new KRun(item.targetUrl(), this);

run->setShowScriptExecutionPrompt(true);
Should be enough

You have a pending review.

Review Board 2.5.15

Screenshots

Files

Issues

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Groups:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Added Files:

Diff:

Status: Closed (submitted)