Differential D4193

KDEPlatformFileDialog: Fix initial directory selection for remote files
ClosedPublic
Actions

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

Details

Reviewers
elvisangelaccio
Group Reviewers
Plasma
Commits
R135:cc064e81c6ed: KDEPlatformFileDialog: Fix initial directory selection for remote files
R135:bfd41a95530f: 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.

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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
arichardson updated this revision to Diff 10338.Jan 18 2017, 10:19 PM
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 a subscriber: graesslin.Jan 19 2017, 6:13 AM
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 a subscriber: elvisangelaccio.Jan 23 2017, 5:52 PM
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?

arichardson added a comment.Feb 22 2017, 9:39 AM

Ping?

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

elvisangelaccio added inline comments.Feb 22 2017, 11:07 AM
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 updated this revision to Diff 13383.Apr 13 2017, 11:41 AM
arichardson marked an inline comment as done.
elvisangelaccio added inline comments.Apr 13 2017, 1:29 PM
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 a subscriber: anthonyfieroni.Apr 13 2017, 2:10 PM
anthonyfieroni added inline comments.
src/platformtheme/kdeplatformfiledialoghelper.cpp
411–412

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

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
ngraham added a subscriber: ngraham.Oct 23 2017, 3:14 AM

What's next for this?

arichardson updated this revision to Diff 21216.Oct 24 2017, 8:13 AM
  • 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.

ngraham added a comment.Mar 30 2018, 4:07 PM

@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
arichardson added a comment.Mar 31 2018, 8:28 AM

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?

elvisangelaccio added a comment.Mar 31 2018, 9:40 AM

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

arichardson updated this revision to Diff 31284.Apr 4 2018, 11:02 AM
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
Closed by commit R135:bfd41a95530f: KDEPlatformFileDialog: Fix initial directory selection for remote files (authored by arichardson). · Explain WhyApr 8 2018, 10:06 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.

arichardson added a comment.Apr 8 2018, 12:55 PM
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?

Revision Contents
Changeset List

Diff 31284

View Options

src/platformtheme/kdeplatformfiledialoghelper.cpp

Loading...