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

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

Details

Reviewers
dfaure
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.May 5 2020, 1:07 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 5 2020, 1:07 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
broulik requested review of this revision.May 5 2020, 1:07 PM
apol added a subscriber: apol.May 5 2020, 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.May 5 2020, 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.May 6 2020, 12:29 PM
src/widgets/kopenwithdialog.cpp
1008

Forgot that also existed, will update

broulik added inline comments.May 29 2020, 7:09 AM
src/widgets/kopenwithdialog.cpp
1008

Actually, that doesn't exist

ahmadsamir added inline comments.May 29 2020, 8:43 AM
broulik added inline comments.May 29 2020, 8:44 AM
src/widgets/kopenwithdialog.cpp
1008

But I don't want to create a QFIleInfo just because

ahmadsamir added inline comments.May 29 2020, 8:51 AM
src/widgets/kopenwithdialog.cpp
1009

It's created on the next line. Anyway my initial comment was a nit-pick... it's a micro-optimisation for when the code goes through the if block, technically DesktopExecParser should get the absolute path in most cases.

dfaure requested changes to this revision.May 29 2020, 10:27 PM

I have modified DesktopExecParser in kio commit d1e9325fba37eddb9cb44173edfc1fee122a0c57 to return an error string so that its users don't need to do the work again of figuring out what went wrong (see DesktopExecParser::errorMessage()).
I just realized that this would be very useful here as well. AFAICS the current patch only works if the user types a full path, not with a relative path.

This revision now requires changes to proceed.May 29 2020, 10:27 PM