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

[Ahmad Samir]

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, bruns

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

[Ahmad Samir]

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

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, bruns

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

[Ahmad Samir]

ahmadsamir added a comment.


  In D27999#631255 , @dfaure wrote:
  
  > 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.
  
  
  I did pass locally :/; I'm on it.

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, bruns

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

[David Faure]

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

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, bruns

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

[Ahmad Samir]

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
  https://phabricator.kde.org/D27999?vs=77964&id=78043

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

AFFECTED FILES
  autotests/krununittest.cpp
  autotests/krununittest.h
  src/core/desktopexecparser.cpp

To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

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

[Ahmad Samir]

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" ;)
  >
  > Sorry for the delay!
  
  
  No problem. :)

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, bruns

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

[Nathaniel Graham]

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, bruns

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

[Nathaniel Graham]

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, bruns

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

[Alexander Saoutkin]

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 
"KIOFuse people" ;)
  
  Sorry for the delay!

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, bruns

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

[Ahmad Samir]

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 browser is empty, it "should" 
be OK...

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, cblack, GB_2, michaelh, ngraham, bruns

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

[David Faure]

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, cblack, GB_2, michaelh, ngraham, bruns

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

[Ahmad Samir]

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&id=77964

BRANCH
  l-krun-ssh (branched from master)

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

AFFECTED FILES
  autotests/krununittest.cpp
  autotests/krununittest.h
  src/core/desktopexecparser.cpp

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

[Ahmad Samir]

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 that the code gets here starting 
from this runApplication() call 
https://cgit.kde.org/kio.git/tree/src/widgets/krun.cpp#n843, which, IIUC, means 
that "http" is caught in KRun::init() before we ever get here.

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, cblack, GB_2, michaelh, ngraham, bruns

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

[David Faure]

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 master)

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

[Ahmad Samir]

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

[Alexander Saoutkin]

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: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

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

[Ahmad Samir]

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 checking what 
DesktopExecParser will try to use, this should work.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27999?vs=77533&id=77538

BRANCH
  l-krun-ssh (branched from master)

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

AFFECTED FILES
  autotests/krununittest.cpp
  autotests/krununittest.h
  src/core/desktopexecparser.cpp

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

[Méven Car]

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

[David Faure]

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 
when running this test method with PATH=/does/not/exist.
  It seems to me that desktopexecparser.cpp won't find it then, which should 
probably be fixed with a fallback to "if it exists in 
QCoreApplication::applicationDirPath(), use that" -- and then the unittest can 
do the same.

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

[Ahmad Samir]

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

[Ahmad Samir]

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&id=77533

BRANCH
  l-krun-ssh (branched from master)

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

AFFECTED FILES
  autotests/krununittest.cpp
  autotests/krununittest.h
  src/core/desktopexecparser.cpp

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

[David Faure]

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 
> %1://root@10.1.1.1").arg(protocol));
> +}

Wait, I didn't notice before, but this is broken.
On my system it's in another prefix.
On Windows it won't be in /usr/bin either.
You need to either strip out the path, or use 
QStandardPaths::findExecutable("ktelnetservice5") in an arg() for that expected 
value.

> dfaure wrote in krununittest.cpp:238
> QFINDTESTDATA() is more flexible than a solution like CMAKE_SOURCE_DIR [e.g. 
> it allows deploying unittests to android devices]. It's also simpler to use 
> (no buildsystem change).

Good practice is to follow that with QVERIFY(!ktelnet.isEmpty());

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

[Ahmad Samir]

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&id=77521

BRANCH
  l-krun-ssh (branched from master)

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

AFFECTED FILES
  autotests/krununittest.cpp
  autotests/krununittest.h
  src/core/desktopexecparser.cpp

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

[Ahmad Samir]

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 UPDATE
  https://phabricator.kde.org/D27999?vs=77479&id=77520

BRANCH
  l-krun-ssh (branched from master)

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

AFFECTED FILES
  autotests/krununittest.cpp
  autotests/krununittest.h
  src/core/desktopexecparser.cpp

To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns, 
feverfew

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

[David Faure]

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);

QFINDTESTDATA() is more flexible than a solution like CMAKE_SOURCE_DIR [e.g. it 
allows deploying unittests to android devices]. It's also simpler to use (no 
buildsystem change).

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

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

[Alexander Saoutkin]

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, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

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

[Harald Sitter]

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
  https://phabricator.kde.org/D27999

To: ahmadsamir, #frameworks, dfaure, sitter, meven
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

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

[Ahmad Samir]

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, michaelh, ngraham, bruns

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

[Ahmad Samir]

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 CMAKE* vars that need
  to be accessed from the code in krununittest.

TEST PLAN
  $ kde-open5 ssh://user@1.1.1.1
  
outputs:
command= "ktelnetservice5 %u" args= ("ktelnetservice5 %u", 
"ssh://user@1.1.1.1")
kf5.kio.core: couldn't create slave: "klauncher said: Unknown protocol 
'ssh'.\n"
  
  and you get a message box with "File not found: ssh://user@1.1.1.1".
  
  Apply diff then try again, now a terminal window is launched by
  ktelnetservice5 and ssh is invoked as expected.
  
  BUG: 418258
  
  FIXED-IN: 5.69

REPOSITORY
  R241 KIO

BRANCH
  l-krun-ssh (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/krununittest.cpp
  autotests/krununittest.h
  src/core/desktopexecparser.cpp

To: ahmadsamir, #frameworks, dfaure, sitter, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns