D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. See D28169 . REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. OK, I see why it's failing. KMimeTypeTrader::self()->preferredService() (from schemeService()) can't find ktelnetservice5.desktop in /usr/share/applications/ since KIO isn't installed on the CI system; so skip the test in that case? REPOSITORY R241 KIO

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. In D27999#631255 , @dfaure wrote: > This commit breaks krununittest in CI, can you take a look? > >

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. This commit breaks krununittest in CI, can you take a look? https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kio/job/kf5-qt5%20SUSEQt5.12/471/testReport/junit/projectroot/autotests/kiowidgets_krununittest/ Thanks. REPOSITORY R241 KIO

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

This revision was automatically updated to reflect the committed changes. Closed by commit R241:14b7f2c7ee72: [DesktopExecParser] Open {ssh,telnet,rlogin}:// urls with ktelnetservice (authored by ahmadsamir). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. In D27999#630756 , @feverfew wrote: > Quick testing with `fish` protocol doesn't break anything KIOFuse side. Going to page in @ngraham, if it works for him with `smb` it's an all good from the "KIOFuse people" ;)

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ngraham added a comment. Shipit! REPOSITORY R241 KIO BRANCH l-krun-ssh (branched from master) REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ngraham accepted this revision. REPOSITORY R241 KIO BRANCH l-krun-ssh (branched from master) REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew, ngraham Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

feverfew added a reviewer: ngraham. feverfew accepted this revision. feverfew added a subscriber: ngraham. feverfew added a comment. Quick testing with `fish` protocol doesn't break anything KIOFuse side. Going to page in @ngraham, if it works for him with `smb` it's an all good from the

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. In D27999#630329 , @dfaure wrote: > Only if an external browser is set, but yeah. Right, I missed the bit about the external browser; however with the recent change to fallback to mimeapps.list if external

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. Only if an external browser is set, but yeah. REPOSITORY R241 KIO BRANCH l-krun-ssh (branched from master) REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew Cc: kde-frameworks-devel, LeGast00n,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir updated this revision to Diff 77964. ahmadsamir added a comment. Rebase on master REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27999?vs=77538=77964 BRANCH l-krun-ssh (branched from master) REVISION DETAIL https://phabricator.kde.org/D27999

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. In D27999#630193 , @dfaure wrote: > OK with me, but check with the fuse people. > > You test ssh, telnet etc but hasSchemeHandler is also true for at least http. While I was debugging this issue, I found

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. OK with me, but check with the fuse people. You test ssh, telnet etc but hasSchemeHandler is also true for at least http. REPOSITORY R241 KIO BRANCH l-krun-ssh (branched from

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. Friendly ping. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

feverfew added a comment. don't merge yet, looking at the code I sense something might break here but I need some time to do my own testing first. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew Cc:

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir updated this revision to Diff 77538. ahmadsamir added a comment. As pointed out by dfaure, we have to make sure the unit test passes even if KIO isn't installed (which might be the case on the KDE CI system). Since we aren't actually executing the ktelnetservice5 binary and only

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

meven added a comment. Good to me REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. Thanks, looks better. One possible problem left: if KIO has never been installed, I guess QStandardPaths::findExecutable(QStringLiteral("ktelnetservice5")) won't find it in builddir/bin. You could push and see what CI says, or you could test locally what happens

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir marked 3 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir updated this revision to Diff 77533. ahmadsamir added a comment. - Use QStandardPaths::findExecutable(), as the path isn't hardcoded to /usr/bin/ on all systems - More QVERIFY() REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27999?vs=77521=77533

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > krununittest.cpp:245 > +QCOMPARE(KShell::joinArgs(parser.resultingArguments()), > + QStringLiteral("/usr/bin/ktelnetservice5 >

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir updated this revision to Diff 77521. ahmadsamir added a reviewer: feverfew. ahmadsamir added a comment. Tweak REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27999?vs=77520=77521 BRANCH l-krun-ssh (branched from master) REVISION DETAIL

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir updated this revision to Diff 77520. ahmadsamir edited the summary of this revision. ahmadsamir removed a reviewer: feverfew. ahmadsamir removed a subscriber: feverfew. ahmadsamir added a comment. Address comments, use QFINDTESTDATA() REPOSITORY R241 KIO CHANGES SINCE LAST

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

dfaure added a comment. Thanks for the unittest with the fix. INLINE COMMENTS > krununittest.cpp:238 > +{ > +const QString ktelnet = QStringLiteral(CMAKE_SOURCE_DIR) + > QStringLiteral("/src/ioslaves/telnet/ktelnetservice5.desktop"); > +const KService service(ktelnet);

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

feverfew added a reviewer: feverfew. feverfew added a comment. I'll review this when I have a chance (sometime this week). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew Cc: feverfew,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

sitter added a subscriber: feverfew. sitter added a comment. Tested fuse and it works as expected. Code looks reasonable, but I have little expertise with this class. @feverfew probably ought to comment as well since he introduced the fuse code. REPOSITORY R241 KIO REVISION DETAIL

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir added a comment. FTR, I don't have access to any smb:// servers, so couldn't test the KIOFuse stuff. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27999 To: ahmadsamir, #frameworks, dfaure, sitter, meven Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2,

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, dfaure, sitter, meven. Herald added a project: Frameworks. ahmadsamir requested review of this revision. REVISION SUMMARY Extend krununittest to test ktelnetservice. Use CMake target_compile_definitions() to set the