D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Go for it!

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: fvogt, ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, 
plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson added a comment.


  In D4193#242444 , @fvogt wrote:
  
  > What about `Plasma/5.12`? It has a minimum of Qt 5.9 as well.
  
  
  Good point. Any objections against me cherry-picking it to the 5.12 branch?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: fvogt, ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, 
plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Fabian Vogt]

fvogt added a comment.


  What about `Plasma/5.12`? It has a minimum of Qt 5.9 as well.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: fvogt, ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, 
plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:bfd41a95530f: KDEPlatformFileDialog: Fix initial 
directory selection for remote files (authored by arichardson).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4193?vs=31284=31656

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp

To: arichardson, #plasma, elvisangelaccio
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson updated this revision to Diff 31284.
arichardson edited the summary of this revision.
arichardson added a comment.


  - Removed version check since we now depend on Qt 5.9
  - Updated commit message

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4193?vs=21216=31284

BRANCH
  master

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp

To: arichardson, #plasma, elvisangelaccio
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Elvis Angelaccio]

elvisangelaccio added a comment.


  I'd just remove the code and explain why in the commit message.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson added a comment.


  I'll update and test this again when I get back to my work computer on 
Tuesday. Should I just remove the code or add a comment that since qt 5.8 it is 
no longer necessary to set the directory as well?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Elvis Angelaccio]

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


  Qt minimum version for plasma-integration is 5.9 these days, so this patch 
should be updated.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Nathaniel Graham]

ngraham added a comment.


  @elvisangelaccio, does this look sane?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Nathaniel Graham]

ngraham added a reviewer: elvisangelaccio.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma, elvisangelaccio
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> arichardson wrote in kdeplatformfiledialoghelper.cpp:365-370
> I don't think we can depend on that yet, can we? Also I'm not sure we really 
> need that runtime check. How likely is it that someone compiles plasma 
> integration against Qt 5.7 and runs it with 5.8? Is that even supported? 
> Aren't we using private APIs?

> How likely is it that someone compiles plasma integration against Qt 5.7 and 
> runs it with 5.8?

It's likely (hypothetical), complied with 5.8 and run it with 5.7 is 
unsupported.

> Is that even supported?

It's supported by Qt, by KDE pretty sure - not.
I'm not stopper of this change, Elvis should accept it.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Nathaniel Graham]

ngraham added a comment.


  @anthonyfieroni and @anthonyfieroni, looks like there are some inline 
questions for you guys.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson updated this revision to Diff 21216.
arichardson added a comment.


  - rebased
  - removed runtime check

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4193?vs=13383=21216

BRANCH
  arcpatch-D4193

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp

To: arichardson, #plasma
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Nathaniel Graham]

ngraham added a comment.


  What's next for this?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma
Cc: ngraham, krzyc, anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kdeplatformfiledialoghelper.cpp:365-370
> http://doc.qt.io/qt-5/qversionnumber.html

I don't think we can depend on that yet, can we? Also I'm not sure we really 
need that runtime check. How likely is it that someone compiles plasma 
integration against Qt 5.7 and runs it with 5.8? Is that even supported? Aren't 
we using private APIs?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma
Cc: anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kdeplatformfiledialoghelper.cpp:365-370
> Sorry but I didn't think about Qt 5.10.x which would break this check. So 
> please ignore my suggestion here.

http://doc.qt.io/qt-5/qversionnumber.html

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma
Cc: anthonyfieroni, elvisangelaccio, graesslin, plasma-devel, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kdeplatformfiledialoghelper.cpp:365-370
> What about `if (qVersion() < QStringLiteral("5.7.1")) {}`? It doesn't work in 
> the general case but it should work for this specific check.

Sorry but I didn't think about Qt 5.10.x which would break this check. So 
please ignore my suggestion here.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma
Cc: elvisangelaccio, graesslin, plasma-devel, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson updated this revision to Diff 13383.

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4193?vs=10338=13383

BRANCH
  arcpatch-D4193

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp

To: arichardson, #plasma
Cc: elvisangelaccio, graesslin, plasma-devel, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson marked an inline comment as done.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: arichardson, #plasma
Cc: elvisangelaccio, graesslin, plasma-devel, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> arichardson wrote in kdeplatformfiledialoghelper.cpp:365-370
> I want the version of Qt not KCoreAddons so that won't work.
> Do we really need a runtime Qt version check? We link to Qt5PlatformSupport, 
> is that API stable or do we need to recompile for new Qt versions anyway?

What about `if (qVersion() < QStringLiteral("5.7.1")) {}`? It doesn't work in 
the general case but it should work for this specific check.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: arichardson, #plasma
Cc: elvisangelaccio, graesslin, plasma-devel, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson added a comment.


  Ping?
  
  I work a lot with remote projects over sftp:// so this would be very 
important for me.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: arichardson, #plasma
Cc: elvisangelaccio, graesslin, plasma-devel, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kdeplatformfiledialoghelper.cpp:365-370
> > what is the easy way to do that?
> 
> There is KCoreAddons::version()

I want the version of Qt not KCoreAddons so that won't work.
Do we really need a runtime Qt version check? We link to Qt5PlatformSupport, is 
that API stable or do we need to recompile for new Qt versions anyway?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: arichardson, #plasma
Cc: elvisangelaccio, graesslin, plasma-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Changed Subscribers] D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> arichardson wrote in kdeplatformfiledialoghelper.cpp:365-370
> Yes it should be but what is the easy way to do that? Do I have to parse the 
> result of qVersion()? CMakeLists.txt says the minimum version is 5.5 so I 
> can't use QVersionNumber?

> what is the easy way to do that?

There is KCoreAddons::version()

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: arichardson, #plasma
Cc: elvisangelaccio, graesslin, plasma-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson added inline comments.

INLINE COMMENTS

> graesslin wrote in kdeplatformfiledialoghelper.cpp:365-370
> Shouldn't that be a runtime check?

Yes it should be but what is the easy way to do that? Do I have to parse the 
result of qVersion()? CMakeLists.txt says the minimum version is 5.5 so I can't 
use QVersionNumber?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: arichardson, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Changed Subscribers] D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Martin Gräßlin]

graesslin added inline comments.

INLINE COMMENTS

> kdeplatformfiledialoghelper.cpp:365-370
> +#if QT_VERSION < QT_VERSION_CHECK(5, 7, 1)
> +// Qt 5 at least <= 5.7.1 does not derive the directory from the passed 
> url
>  // and set the initialDirectory option accordingly, also not for known 
> schemes
>  // like file://, so we have to do it ourselves
>  options()->setInitialDirectory(m_dialog->directory());
> +#endif

Shouldn't that be a runtime check?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: arichardson, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Request, 4 lines] D4193: KDEPlatformFileDialog: Fix initial directory selection for remote files

[Alex Richardson]

arichardson created this revision.
arichardson added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  We currently get the following sequence of calls:
  
  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/)
  
  Previously KDEPlatformFileDialogHelper::selectFile() would change
  options()->initialDirectory() unconditionally even if it was already
  set by the QFileDialog code. The final setDirectory() call is actually a call
  to setDirectory(options->initialDirectory()) which was set in the selectFile()
  call. It no longer seems to be required to derive initialDirectory from the
  selectFile() call and this will now to override the correct initial directory
  that was set by Qt.
  Qt should not be passing a local URL when the actual directory URL is remote
  but the code in QFileDialogPrivate::init() unconditionally sets a local URL
  until https://codereview.qt-project.org/#/c/182661/ or another fix is 
submitted.
  
  BUG: 374913

TEST PLAN
  Remote directory is now opened correctly

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  master

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: arichardson, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas