dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Thanks for the quick fix.
INLINE COMMENTS
> krun.cpp:366
> + // On Windows, run all executables normally
> + bool runWindowsExecutable = true;
> +#else
const bool ...
> krun.cpp:370
> + // from being run, so that programs like Wine can handle
> them instead.
> + bool runWindowsExecutable =
> !mime.inherits(QStringLiteral("application/x-ms-dos-executable"));
> +#endif
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?
> krun.cpp:390
> }
> - } else if (!isFileExecutable && isTextFile) {
> - // Don't try to run scripts without execute bit, instead
> - // open them with default application
> + } else if ((!isFileExecutable && isTextFile) ||
> !runWindowsExecutable) {
> + // Don't try to run scripts without execute bit or any
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?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23989
To: mdlubakowski, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
