KDEPlatformFileDialog: Fix initial directory selection for remote files
ClosedPublic

Authored by arichardson on Jan 18 2017, 10:19 PM.

Details

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.

BUG: 374913

Test Plan

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

Diff Detail

Repository
R135 Integration for Qt applications in Plasma
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
arichardson retitled this revision from to KDEPlatformFileDialog: Fix initial directory selection for remote files.
arichardson updated this object.
arichardson edited the test plan for this revision. (Show Details)
arichardson added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptJan 18 2017, 10:19 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
graesslin added inline comments.
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

Shouldn't that be a runtime check?

arichardson added inline comments.Jan 23 2017, 10:52 AM
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

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?

elvisangelaccio added inline comments.
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

what is the easy way to do that?

There is KCoreAddons::version()

arichardson added inline comments.Feb 1 2017, 11:42 PM
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

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?

Ping?

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

src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

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

arichardson marked an inline comment as done.
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

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

anthonyfieroni added inline comments.
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412
arichardson added inline comments.Apr 13 2017, 2:20 PM
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

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?

krzyc added a subscriber: krzyc.Jun 7 2017, 4:54 PM

What's next for this?

  • rebased
  • removed runtime check
ngraham added a comment.EditedDec 6 2017, 9:00 PM

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

anthonyfieroni added inline comments.Dec 7 2017, 4:46 AM
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

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.

@elvisangelaccio, does this look sane?

elvisangelaccio requested changes to this revision.Mar 30 2018, 5:48 PM

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

This revision now requires changes to proceed.Mar 30 2018, 5:48 PM

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?

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

arichardson edited the summary of this revision. (Show Details)
  • Removed version check since we now depend on Qt 5.9
  • Updated commit message
arichardson edited the summary of this revision. (Show Details)Apr 4 2018, 2:25 PM
arichardson edited the test plan for this revision. (Show Details)
elvisangelaccio accepted this revision.Apr 7 2018, 11:35 AM
This revision is now accepted and ready to land.Apr 7 2018, 11:35 AM
This revision was automatically updated to reflect the committed changes.
fvogt added a subscriber: fvogt.Apr 8 2018, 10:41 AM

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

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?