Bug 374913

Summary: Open does not use directory for currently open file with sftp://
Product: [Plasma] plasma-integration Reporter: Alex Richardson <arichardson.kde>
Component: generalAssignee: Martin Flöser <mgraesslin>
Status: RESOLVED FIXED    
Severity: normal CC: 68guns, alexander, arichardson.kde, asn, charles.vejnar, Darren.Lissimore, diese-addy, drankinatty, faure, gaaf, ilovekde, kde_bugs, kfunk, marco, nate, porton, pyrkosz, shimi.chen, sitter, toralf.foerster, wbauer1
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In:

Description Alex Richardson 2017-01-11 14:15:58 UTC
Instead it opens the file dialog in the CWD. The same thing happens for KWrite but strangely it works fine in KDevelop so I don't think it is related to the integration plugin. 

The only change that I can see affecting opening files is 2daae41a3738cc319f5925a96d3938779b3bd09b but that doesn't look like it should break this.

It seems to be working fine on my openSUSE Tumbleweed laptop but is broken in Ubuntu neon. Not sure if this is packaging issue or they are just using different versions.
Comment 1 Alex Richardson 2017-01-17 20:37:37 UTC
I just updated my openSUSE tumbleweed system and it is broken there now. So  it is definitely a regression in Kate and not a packaging issue.

new version (tumbleweed snapshot 20170112):
```
Kate Version 16.12.0
KDE Frameworks 5.30.0
Qt 5.7.1 (built against 5.7.1)
The xcb windowing system
```

Old version was a snapshot sometime in December still with 16.08 and Frameworks 5.29 or 5.28
Comment 2 Alex Richardson 2017-01-17 21:37:24 UTC
Seems like the problem might be in Qt. I added some debug output and it seems 

Qt is calling KDEPlatformFileDialogHelper::selectFile(const QUrl &filename) by simply concatenating filename with the CWD because it does QUrl::fromLocalFile("myfile.cpp") which results in "CWD/myfile.cpp"

I'll see if I can work around this in plasma-integration

https://code.woboq.org/qt5/qtbase/src/widgets/dialogs/qfiledialog.cpp.html#1048
```
    if (!d->usingWidgets()) {
        QUrl url = QUrl::fromLocalFile(filename);
        if (QFileInfo(filename).isRelative()) {
            QDir dir(d->options->initialDirectory().toLocalFile());
            url = QUrl::fromLocalFile(dir.absoluteFilePath(filename));
        }
        d->selectFile_sys(url);
        d->options->setInitiallySelectedFiles(QList<QUrl>() << url);
        return;
    }
```
Comment 3 Alex Richardson 2017-01-18 22:38:43 UTC
I have submitted https://codereview.qt-project.org/#/c/182661/ and https://phabricator.kde.org/D4193
Comment 4 Alex Richardson 2017-05-23 09:18:54 UTC
*** Bug 380024 has been marked as a duplicate of this bug. ***
Comment 5 Christoph Feck 2017-06-07 10:55:37 UTC
*** Bug 380104 has been marked as a duplicate of this bug. ***
Comment 6 pyrkosz 2017-07-24 20:48:24 UTC
Bug fix https://codereview.qt-project.org/#/c/182661/2 is still unreviewed because of coding style issues. Because it is serious issue for KDE, please resubmit it with required changes. Thanks.
Comment 7 Alexander Zhigalin 2017-07-29 16:22:05 UTC
*** Bug 374296 has been marked as a duplicate of this bug. ***
Comment 8 Christo 2017-09-09 17:16:28 UTC
(In reply to pyrkosz from comment #6)
> Bug fix https://codereview.qt-project.org/#/c/182661/2 is still unreviewed
> because of coding style issues. Because it is serious issue for KDE, please
> resubmit it with required changes. Thanks.

Patch3 seems to be style-consistent .. i see no reason to refuse it
Comment 9 irismartinek 2017-09-09 17:16:56 UTC
same problem here ;( still no solution?
Comment 10 Christo 2017-09-09 17:23:06 UTC
(In reply to Alexander Zhigalin from comment #7)
> *** Bug 374296 has been marked as a duplicate of this bug. ***

please submit a vote :) https://bugs.kde.org/page.cgi?id=voting/user.html&bug_id=374913#vote_374913
Comment 11 Christo 2017-09-09 17:34:36 UTC
*** Bug 384311 has been marked as a duplicate of this bug. ***
Comment 12 Darren Lissimore 2017-09-16 04:16:35 UTC
Is there other patches needed to than the one mentioned here 
https://codereview.qt-project.org/#/c/182661/2
to get the desired behavoir? 

Tested with qt5.9.1 and the above patch and still see the main issue at hand 

Test setup:
- distro - kde-neon - fully updated
- Qt - custom built qt-5.9.1 directly from git - with the above patch applied
- Kate - from kde-neon

Test: 
a) start new kate session
b) open file via sftp or fish 
c) once open - immediately try file->open 

Result: 
- you get the file-open dialog but instead of the sftp/fish'ed location; 
  the location displayed is the current local direct from where I launched kate
- the "Name" field of the dialog populated with the filename opened from step b)

Expected results:
- the remote directory location should be displayed
Comment 13 Christo 2017-09-16 17:04:22 UTC
(In reply to Darren Lissimore from comment #12) 
> Tested with qt5.9.1 and the above patch and still see the main issue at hand 

does that mean?: you patched a 5.9.1 with "patch set 3" and still experience that issue
Comment 14 Darren Lissimore 2017-09-16 18:07:22 UTC
(In reply to Christoph from comment #13)
> (In reply to Darren Lissimore from comment #12) 
> > Tested with qt5.9.1 and the above patch and still see the main issue at hand 
> 
> does that mean?: you patched a 5.9.1 with "patch set 3" and still experience
> that issue

Yes - with patch-set 3.  

I added in a qDebug() before the patch directory.isLocalFile() check in the patch just verify that the patch is being hit. 
I see the qDebug() output -- yet for a fish'ed file or sftp'ed file I still get as I described above.

Are any others having success with patch-set 3?
- I could be missing something - but it doesn't look like it.
Comment 15 Christoph Feck 2017-09-17 15:24:31 UTC
*** Bug 383963 has been marked as a duplicate of this bug. ***
Comment 16 Christoph Feck 2017-09-20 23:05:18 UTC
*** Bug 353214 has been marked as a duplicate of this bug. ***
Comment 17 Christo 2017-09-23 14:00:20 UTC
that would imply that Alex's patch doesnt work out and a new approach must be found :-o :-(
Comment 18 Alex Hermann 2017-09-23 15:56:48 UTC
I found that i needed both the Qt patch and the KDE patch for correct behavior.
Comment 19 Christo 2017-11-25 01:21:31 UTC
(In reply to Alex Hermann from comment #18)
> I found that i needed both the Qt patch and the KDE patch for correct
> behavior.

did you commit the patches back ?
Comment 20 Nate Graham 2017-12-06 21:04:02 UTC
*** Bug 372208 has been marked as a duplicate of this bug. ***
Comment 21 Christoph Feck 2018-03-30 16:01:13 UTC
*** Bug 391869 has been marked as a duplicate of this bug. ***
Comment 22 Alex Richardson 2018-04-08 10:06:59 UTC
Git commit bfd41a95530f90ee8d44cbcfd1fa8c62978334a2 by Alex Richardson.
Committed on 08/04/2018 at 10:06.
Pushed by arichardson into branch 'master'.

KDEPlatformFileDialog: Fix initial directory selection for remote files

Summary:
Previously KDEPlatformFileDialogHelper::selectFile() would change
options()->initialDirectory() unconditionally even if it was already
set by the QFileDialog code. Since Qt 5.7.1 it is no longer necessary
to derive initialDirectory from the selectFile() call. In fact it is
actuall harmful since it will now override the correct initial directory
that was set by Qt. Without this patch I got the following debug output:

```
KDEPlatformFileDialogHelper::setDirectory QUrl("sftp://server/home/alr48/cheri/build_sdk.sh")
KDEPlatformFileDialogHelper::setDirectory QUrl("sftp://server/home/alr48/cheri/build_sdk.sh")
KDEPlatformFileDialogHelper::selectFile QUrl("file:///home/alex/build_sdk.sh")
KDEPlatformFileDialogHelper::setDirectory QUrl("file:///home/alex/)
```
The final setDirectory() call is actually a call to
`setDirectory(options->initialDirectory())` which was set in `selectFile()`.

We now depend on Qt 5.9 so we can remove this code without a check for
version >= 5.7.1.

Test Plan: Remote directory is now opened correctly (tested with Qt 5.10.0)

Reviewers: #plasma, elvisangelaccio

Reviewed By: elvisangelaccio

Subscribers: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D4193

M  +0    -5    src/platformtheme/kdeplatformfiledialoghelper.cpp

https://commits.kde.org/plasma-integration/bfd41a95530f90ee8d44cbcfd1fa8c62978334a2
Comment 23 Alex Richardson 2018-04-08 14:06:15 UTC
Git commit cc064e81c6ed51f3b4422c2f2347e5f4e090e628 by Alex Richardson.
Committed on 08/04/2018 at 14:05.
Pushed by arichardson into branch 'Plasma/5.12'.

KDEPlatformFileDialog: Fix initial directory selection for remote files

Summary:
Previously KDEPlatformFileDialogHelper::selectFile() would change
options()->initialDirectory() unconditionally even if it was already
set by the QFileDialog code. Since Qt 5.7.1 it is no longer necessary
to derive initialDirectory from the selectFile() call. In fact it is
actuall harmful since it will now override the correct initial directory
that was set by Qt. Without this patch I got the following debug output:

```
KDEPlatformFileDialogHelper::setDirectory QUrl("sftp://server/home/alr48/cheri/build_sdk.sh")
KDEPlatformFileDialogHelper::setDirectory QUrl("sftp://server/home/alr48/cheri/build_sdk.sh")
KDEPlatformFileDialogHelper::selectFile QUrl("file:///home/alex/build_sdk.sh")
KDEPlatformFileDialogHelper::setDirectory QUrl("file:///home/alex/)
```
The final setDirectory() call is actually a call to
`setDirectory(options->initialDirectory())` which was set in `selectFile()`.

We now depend on Qt 5.9 so we can remove this code without a check for
version >= 5.7.1.

Test Plan: Remote directory is now opened correctly (tested with Qt 5.10.0)

Reviewers: #plasma, elvisangelaccio

Reviewed By: elvisangelaccio

Subscribers: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D4193

M  +0    -5    src/platformtheme/kdeplatformfiledialoghelper.cpp

https://commits.kde.org/plasma-integration/cc064e81c6ed51f3b4422c2f2347e5f4e090e628
Comment 24 Christo 2018-04-08 18:13:42 UTC
loving it :-) i hope it will get ported to KDE Frameworks 5.44.0 / Qt 5.9.4
Comment 25 Toralf Förster 2018-04-08 18:30:41 UTC
(In reply to Alex Richardson from comment #23)
> Git commit cc064e81c6ed51f3b4422c2f2347e5f4e090e628 by Alex Richardson.
> Committed on 08/04/2018 at 14:05.
> Pushed by arichardson into branch 'Plasma/5.12'.

works flawlessly with 5.11.5 too here at a stable Gentoo Linux and fixes a painful issue I had here with KDE - thx.
Comment 26 David Faure 2018-07-28 12:24:54 UTC
I believe this fix was wrong, it broke the testSelectUrl unittest, i.e. the case where the application would call selectUrl directly.

I have just submitted what I believe to be the right fix for this bug, in Qt:
https://codereview.qt-project.org/235473
Comment 27 Alex Richardson 2018-07-28 12:36:54 UTC
Sorry about that, I didn't test that case.
Comment 28 Christo 2018-07-29 21:21:02 UTC
thank you !
Comment 29 H Richardson 2018-09-25 14:23:47 UTC
Unfortunately after having been fixed a while, this bug has returned in the latest round of updates to openSuSE 15.0.

Opening from a file on SFTP drops you back in at your local home directory instead of browsing the server.

I'm using all the most up-to-date opeSuSE versions of relevant packages, as of today. I can't say if this is a openSuSE issue or a Plasma issue in this case. 

Anyone else got the same issue?
Comment 30 Christo 2018-09-25 16:51:11 UTC
(In reply to H Richardson from comment #29)
> Unfortunately after having been fixed a while, this bug has returned in the
> latest round of updates to openSuSE 15.0.

means you are using dolphin 17.12.3 now ?
Comment 31 Christo 2018-09-25 19:15:41 UTC
(In reply to H Richardson from comment #29)
> Unfortunately after having been fixed a while, this bug has returned in the
> latest round of updates to openSuSE 15.0.
> 
> Opening from a file on SFTP drops you back in at your local home directory
> instead of browsing the server.
> 
> I'm using all the most up-to-date opeSuSE versions of relevant packages, as
> of today. I can't say if this is a openSuSE issue or a Plasma issue in this
> case. 
> 
> Anyone else got the same issue?

i am indeed deeply shocked :-o @Alex did they drop your commit ?
Comment 32 H Richardson 2018-12-24 14:52:09 UTC
Working again for me in KDE 5.53.0. I had to upgrade OpenSUSE 15 KDE to separate KDE repository as vanilla OpenSUSE + updates still had the bug.
Comment 33 Christo 2019-03-19 00:26:39 UTC
(In reply to H Richardson from comment #32)
> Working again for me in KDE 5.53.0. I had to upgrade OpenSUSE 15 KDE to
> separate KDE repository as vanilla OpenSUSE + updates still had the bug.

can you please point out which (of your) repos exactly serve the fixed versions 

:) kindly -c-
Comment 34 Wolfgang Bauer 2019-03-19 11:30:12 UTC
I did some investigation today, and this does work in latest Plasma 5.12 LTS (with Qt 5.9.7) and also with all latest upstream versions (Plasma 5.15.3, Qt 5.12.2).

Closing as FIXED again therefore.

It got indeed broken in Leap 15.0 by a plasma-integration update that includes https://cgit.kde.org/plasma-integration.git/commit/?h=Plasma/5.12&id=4848bec177b2527662098f97aa745bb839bc15e3. The reason is that 15.0 ships with Qt 5.9.4 though and doesn't have this fix yet that's mentioned in comment#3:
https://code.qt.io/cgit/qt/qtbase.git/commit/?h=5.9&id=4cd90a3579986ae7441c3e982a5f34cdfd92c152

That's tracked downstream now though:
http://bugzilla.opensuse.org/show_bug.cgi?id=1129662