Fixed interaction with DOS/Windows executables in KRun::runUrl
ClosedPublic

Authored by mdlubakowski on Sep 16 2019, 12:47 PM.

Details

Summary

Since D22510, executables are no longer checked by isExecutableFile, which excluded application/x-ms-dos-executable files on non-Windows platforms inside KRun::runUrl, which results in broken interaction.
This patch restores previous behaviour.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mdlubakowski created this revision.Sep 16 2019, 12:47 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 16 2019, 12:47 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mdlubakowski requested review of this revision.Sep 16 2019, 12:47 PM
mdlubakowski retitled this revision from Fixed DOS/Windows executable handling by KRun to Fixed interaction with DOS/Windows executables in KRun::runUrl.Sep 16 2019, 12:50 PM
mdlubakowski edited the summary of this revision. (Show Details)
mdlubakowski added reviewers: dfaure, Frameworks.
dfaure requested changes to this revision.Sep 16 2019, 1:19 PM

Thanks for the quick fix.

src/widgets/krun.cpp
366
const bool ...
370
const bool ...

Also, the name of this bool is very confusing in my opinion.
It's true if the file is NOT a windows executable...

Maybe name it something like supportsRunningExecutable?

395

Can you split this into two "else if" conditions? It will be more readable (both the condition, and the comment). This only duplicates a trivial "canRun = false" statement.

And then we could further simplify this by checking the new bool first, so it only needs to be checked once...

if (!supportsRunningExecutable) {
    canRun = false;
} else if (!isFileExecutable && !isTextFile) {
    [....]
} else if (!isFileExecutable && isTextFile) {
    canRun = false;
}

What do you think?

This revision now requires changes to proceed.Sep 16 2019, 1:19 PM
  • Incorporated feedback
mdlubakowski marked 3 inline comments as done.Sep 16 2019, 2:07 PM
mdlubakowski added inline comments.
src/widgets/krun.cpp
395

Agree, it looks cleaner like that.

mdlubakowski marked 2 inline comments as done.Sep 16 2019, 2:07 PM
dfaure accepted this revision.Sep 16 2019, 2:20 PM
This revision is now accepted and ready to land.Sep 16 2019, 2:20 PM
This revision was automatically updated to reflect the committed changes.

I wonder if this should maybe be backported to 5.62, since it represents a regression introduced in that release. Thoughts?

I wonder if this should maybe be backported to 5.62, since it represents a regression introduced in that release. Thoughts?

I would say yes, since I imagine this may be annoyance to some people (the patch basically traded one functionality for another, my fault though). Also not many distros have updated yet (well, Tumbleweed hasn't).

Thanks for landing!

Done, I released a KIO 5.62.1 and informed packagers.

Thanks a bunch!