Changeset View
Standalone View
src/widgets/kopenwithdialog.cpp
| Show First 20 Lines • Show All 990 Lines • ▼ Show 20 Line(s) | 997 | if (m_pService) { | |||
|---|---|---|---|---|---|
| 998 | // Existing service selected | 998 | // Existing service selected | ||
| 999 | serviceName = m_pService->name(); | 999 | serviceName = m_pService->name(); | ||
| 1000 | initialServiceName = serviceName; | 1000 | initialServiceName = serviceName; | ||
| 1001 | fullExec = m_pService->exec(); | 1001 | fullExec = m_pService->exec(); | ||
| 1002 | } else { | 1002 | } else { | ||
| 1003 | const QString binaryName = KIO::DesktopExecParser::executablePath(typedExec); | 1003 | const QString binaryName = KIO::DesktopExecParser::executablePath(typedExec); | ||
| 1004 | // qDebug() << "binaryName=" << binaryName; | 1004 | // qDebug() << "binaryName=" << binaryName; | ||
| 1005 | // Ensure that the typed binary name actually exists (#81190) | 1005 | // Ensure that the typed binary name actually exists (#81190) | ||
| 1006 | if (QStandardPaths::findExecutable(binaryName).isEmpty()) { | 1006 | if (QStandardPaths::findExecutable(binaryName).isEmpty()) { | ||
apol: 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. dfaure: Right, hence the isEmpty() here. This check passes if 1) it doesn't exist, or 2) it's not… | |||||
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:
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". ahmadsamir: I was wondering how the QString returned by KIO::DesktopExecParser::executablePath() would work… | |||||
| 1007 | // Maybe it exists but isn't executable | ||||
| 1008 | if (QDir::isAbsolutePath(binaryName)) { | ||||
ahmadsamir: Why not QFileInfo::isAbsolute()? | |||||
broulik: Forgot that also existed, will update | |||||
broulik: Actually, that doesn't exist | |||||
ahmadsamir: https://doc.qt.io/qt-5.15/qfileinfo.html#isAbsolute ? | |||||
broulik: But I don't want to create a `QFIleInfo` just because | |||||
| 1009 | QFileInfo fi(binaryName); | ||||
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. ahmadsamir: It's created on the next line. Anyway my initial comment was a nit-pick... it's a micro… | |||||
| 1010 | if (fi.exists() && !fi.isExecutable()) { | ||||
| 1011 | KMessageBox::error(q, i18n("The file '%1' is not executable.", binaryName)); | ||||
| 1012 | return false; | ||||
| 1013 | } | ||||
| 1014 | } | ||||
| 1015 | | ||||
| 1007 | KMessageBox::error(q, i18n("'%1' not found, please type a valid program name.", binaryName)); | 1016 | KMessageBox::error(q, i18n("'%1' not found, please type a valid program name.", binaryName)); | ||
| 1008 | return false; | 1017 | return false; | ||
| 1009 | } | 1018 | } | ||
| 1010 | } | 1019 | } | ||
| 1011 | 1020 | | |||
| 1012 | if (terminal->isChecked()) { | 1021 | if (terminal->isChecked()) { | ||
| 1013 | KConfigGroup confGroup(KSharedConfig::openConfig(), QStringLiteral("General")); | 1022 | KConfigGroup confGroup(KSharedConfig::openConfig(), QStringLiteral("General")); | ||
| 1014 | preferredTerminal = confGroup.readPathEntry("TerminalApplication", QStringLiteral("konsole")); | 1023 | preferredTerminal = confGroup.readPathEntry("TerminalApplication", QStringLiteral("konsole")); | ||
| ▲ Show 20 Lines • Show All 164 Lines • Show Last 20 Lines | |||||
This patch D29170 suggests that findExecutable won't find non-executables.
Something's wrong.