D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:9ae855e0dcbc: Kicker/RecentUsageModel: Allow to open 
smb:/ sftp:/ resources (authored by meven).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26582?vs=73275=75211

REVISION DETAIL
  https://phabricator.kde.org/D26582

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: meven, #plasma, broulik, aleksejshilin, hein
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven added a comment.


  Let's be safe and targe 5.19

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arcpatch-D26582

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin, hein
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven added a comment.


  5.18 or not ?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arcpatch-D26582

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin, hein
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Eike Hein]

hein accepted this revision.
hein added a comment.
This revision is now accepted and ready to land.


  Thanks :)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arcpatch-D26582

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin, hein
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven added a comment.


  In D26582#600469 , @hein wrote:
  
  > So does this fall through to the dialog when the type hasn't been determined
  
  
  Any file has always one mime type : "application/octet-stream", but KRun 
generally resolves mimetype throug extension then through kio.
  And then will use KOpenWithDialog when there is no app associated to this 
mimetype.
  
  And this code basically reverts :
  https://phabricator.kde.org/D10835
  Just adding `run->setRunExecutables` instead.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin, hein
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Eike Hein]

hein added a comment.


  So does this fall through to the dialog when the type hasn't been determined?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin, hein
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven added a comment.


  ping :) @hein

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin, hein
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Алексей Шилин]

aleksejshilin added a comment.


  In D26582#592469 , @meven wrote:
  
  > We may want to change KRun::runUrl when a native binary is to be run but 
with runExecutable is false, around line 400:
  >
  >   } else if (isNativeBinary) {
  >   // Show warning for executables that aren't scripts
  >   noRun = true;
  >   }
  >   
  
  
  KRun currently doesn't seem to consider that there may be a reason to open 
native binaries in an application - its execution confirmation dialog doesn't 
give such option even if some application is associated with the corresponding 
mime type. I believe, they thought that an average user is unlikely to ever 
need this - and they may be right. :)
  
  Given that such use case is probably orders of magnitude less likely than 
sftp: and especially smb:, I guess, it's OK to merge this, and address the 
issue with native binaries in KIO later. @broulik ?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven added a comment.


  In D26582#592266 , @aleksejshilin 
wrote:
  
  > In D26582#591959 , @meven wrote:
  >
  > > Well there was already KRun::setRunExecutables :)
  >
  >
  > I've already talked about it in my previous message:
  >
  > In D26582#591952 , 
@aleksejshilin wrote:
  >
  > > KRun currently doesn't seem to have a way to force opening of executables 
- one can only prohibit running them with setRunExecutables(), in which case an 
error message is displayed. That's why I decided to go the way it is now.
  >
  >
  > For example:
  >
  > 1. Install Okteta.
  > 2. Associate application/x-sharedlib mime type with Okteta.
  > 3. Open a shared library with Okteta.
  > 4. Try to open it again from 'Recent Documents'.
  >
  >   Instead of opening the file in Okteta, it'll show a message box saying 
that the executable file won't be started for security reasons.
  >
  >   I admit that it's not a likely scenario though, so let's see what others 
have to say.
  
  
  Thanks for testing this. There is a different treatment with nativeBinary in 
KRun,
  We may want to change KRun::runUrl when a native binary is to be run but with 
runExecutable is false, around line 400:
  
} else if (isNativeBinary) {
// Show warning for executables that aren't scripts
noRun = true;
}
  
  I guess in this case with could check for mime-type associated apps like 
oketa can be with any binary file, perhaps adding a flag.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Алексей Шилин]

aleksejshilin added a comment.


  In D26582#591959 , @meven wrote:
  
  > Well there was already KRun::setRunExecutables :)
  
  
  I've already talked about it in my previous message:
  
  In D26582#591952 , @aleksejshilin 
wrote:
  
  > KRun currently doesn't seem to have a way to force opening of executables - 
one can only prohibit running them with setRunExecutables(), in which case an 
error message is displayed. That's why I decided to go the way it is now.
  
  
  For example:
  
  1. Install Okteta.
  2. Associate application/x-sharedlib mime type with Okteta.
  3. Open a shared library with Okteta.
  4. Try to open it again from 'Recent Documents'.
  
  Instead of opening the file in Okteta, it'll show a message box saying that 
the executable file won't be started for security reasons.
  
  I admit that it's not a likely scenario though, so let's see what others have 
to say.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven updated this revision to Diff 73275.
meven added a comment.


  Use KRun::setRunExecutables

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26582?vs=73269=73275

BRANCH
  arcpatch-D26582

REVISION DETAIL
  https://phabricator.kde.org/D26582

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven added a comment.


  Well there was already KRun::setRunExecutables :)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  In D26582#591952 , @aleksejshilin 
wrote:
  
  > I thought about using KRun::setShowScriptExecutionPrompt() for D10835 
, but "Recent Documents" include only those 
files which were opened, and doesn't include ones which were executed, so an 
execution prompt there would be totally unexpected by a user. KRun currently 
doesn't seem to have a way to force opening of executables - one can only 
prohibit running them with setRunExecutables(), in which case an error message 
is displayed. That's why I decided to go the way it is now.
  >
  > I think, the best solution would be to add a new KRun::RunFlag (like 
OpenExecutables) which'd make it open executable files instead of running them, 
and then use it here.
  
  
  Good point, I will probably offer a KRun patch.
  This regression is very annoying to me.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Алексей Шилин]

aleksejshilin added a comment.


  I thought about using KRun::setShowScriptExecutionPrompt() for D10835 
, but "Recent Documents" include only those 
files which were opened, and doesn't include ones which were executed, so an 
execution prompt there would be totally unexpected by a user. KRun currently 
doesn't seem to have a way to force opening of executables - one can only 
prohibit running them with setRunExecutables(), in which case an error message 
is displayed. That's why I decided to go the way it is now.
  
  I think, the best solution would be to add a new KRun::RunFlag (like 
OpenExecutables) which'd make it open executable files instead of running them, 
and then use it here.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26582

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26582: Kicker/RecentUsageModel: Allow to open smb:/ sftp:/ resources

[Méven Car]

meven created this revision.
meven added reviewers: Plasma, broulik, aleksejshilin.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
meven requested review of this revision.

REVISION SUMMARY
  Since D10835  opening urls that needed 
mimetype determination such as smb:/ or sftp:/ urls were always opened with a 
"Open With" dialog.
  To keep the same security but adding some convenience, use 
KRun::setShowScriptExecutionPrompt to ask the user its intent (and can save it)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D26582

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp

To: meven, #plasma, broulik, aleksejshilin
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart