D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg, dfaure
Cc: ahmadsamir, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> kopenwithdialog.cpp:1009
> +if (QDir::isAbsolutePath(binaryName)) {
> +QFileInfo fi(binaryName);
> +if (fi.exists() && !fi.isExecutable()) {

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.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: ahmadsamir, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kopenwithdialog.cpp:1008
> https://doc.qt.io/qt-5.15/qfileinfo.html#isAbsolute ?

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

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: ahmadsamir, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> broulik wrote in kopenwithdialog.cpp:1008
> Actually, that doesn't exist

https://doc.qt.io/qt-5.15/qfileinfo.html#isAbsolute ?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: ahmadsamir, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> broulik wrote in kopenwithdialog.cpp:1008
> Forgot that also existed, will update

Actually, that doesn't exist

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: ahmadsamir, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kopenwithdialog.cpp:1008
> Why not QFileInfo::isAbsolute()?

Forgot that also existed, will update

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: ahmadsamir, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kopenwithdialog.cpp:1006
> Right, hence the isEmpty() here. This check passes if 1) it doesn't exist, or 
> 2) it's not executable.

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".

> kopenwithdialog.cpp:1008
> +// Maybe it exists but isn't executable
> +if (QDir::isAbsolutePath(binaryName)) {
> +QFileInfo fi(binaryName);

Why not QFileInfo::isAbsolute()?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: ahmadsamir, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> apol wrote in kopenwithdialog.cpp:1006
> This patch D29170  suggests that 
> findExecutable won't find non-executables.
> 
> Something's wrong.

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

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> kopenwithdialog.cpp:1006
>  // Ensure that the typed binary name actually exists (#81190)
>  if (QStandardPaths::findExecutable(binaryName).isEmpty()) {
> +// Maybe it exists but isn't executable

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

Something's wrong.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, #vdg
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29445: [KOpenWithDialog] When pointing at a non-executable file print more meaningful error

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Frameworks, VDG.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
broulik requested review of this revision.

REVISION 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

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/widgets/kopenwithdialog.cpp

To: broulik, #frameworks, #vdg
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns