[KOpenWithDialog] When pointing at a non-executable file print more meaningful error
Needs ReviewPublic

Authored by broulik on Tue, May 5, 1:07 PM.

Details

Reviewers
None
Group Reviewers
Frameworks
VDG
Summary

QStandardPaths::findExecutable only finds executable binaries, so when explicitly pointing it at an existing file, it would complain about it not existing, rather than telling you it's not executable.

Test Plan
  • Tried to associate a non-executable shell script with a file: it now told me what was up

Before it would tell me file doesn't exist

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Tue, May 5, 1:07 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptTue, May 5, 1:07 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
broulik requested review of this revision.Tue, May 5, 1:07 PM
apol added a subscriber: apol.Tue, May 5, 1:44 PM
apol added inline comments.
src/widgets/kopenwithdialog.cpp
1006

This patch D29170 suggests that findExecutable won't find non-executables.

Something's wrong.

dfaure added a subscriber: dfaure.Tue, May 5, 2:14 PM
dfaure added inline comments.
src/widgets/kopenwithdialog.cpp
1006

Right, hence the isEmpty() here. This check passes if 1) it doesn't exist, or 2) it's not executable.

ahmadsamir added inline comments.
src/widgets/kopenwithdialog.cpp
1006

I was wondering how the QString returned by KIO::DesktopExecParser::executablePath() would work if it returned /usr/bin/foo (as it doesn't strip the path), so how does findExecutable() work in that case? ... so I tested and it turns out, findExecutable() will work with:

  • foo, if it can find it in $PATH and it's executable
  • /usr/bin/foo, if it's executable (though I have to wonder how that qualifies as "executableName"?)
  • /some/path/foo, as long as "foo" is executable, the docs don't say anything about this behaviour, however the implementation does indeed support this

This change covers the use case of an absolute path to a file that _exists_ but isn't _executable_.

And again, findExecutable() would be more useful if it reported some sort of error saying "I found it, but it's not executable".

1008

Why not QFileInfo::isAbsolute()?

broulik added inline comments.Wed, May 6, 12:29 PM
src/widgets/kopenwithdialog.cpp
1008

Forgot that also existed, will update