Use QFileDialog in the "Open Project" dialog if we are not in KDE.
ClosedPublic

Authored by arrowd on Apr 2 2016, 6:12 PM.

Details

Summary

I've almost managed to get this work. The problem is

  • I haven't managed to hide window with pages while QFileDialog is shown. It ignores hide() and setVisible()
  • The most freaking part is that after accepting project file in QFileDialog the pages window DISAPPEARS. And when i click where it was it reappears again.

I'll look into it these problems myself, but any help with this would be welcome.

Test Plan

Checked the functionality on Windows. Haven't done tests with full kde environment, so please test it.

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
arrowd updated this revision to Diff 3083.Apr 2 2016, 6:12 PM
arrowd retitled this revision from to Use QFileDialog in the "Open Project" dialog if we are not in KDE..
arrowd updated this object.
arrowd edited the test plan for this revision. (Show Details)
arrowd added a reviewer: KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptApr 2 2016, 6:12 PM
arrowd updated this revision to Diff 3120.Apr 4 2016, 7:28 AM

Fix the issue with disappearing dialog.

arrowd edited the test plan for this revision. (Show Details)Apr 4 2016, 7:29 AM
arrowd updated this revision to Diff 3122.Apr 4 2016, 10:09 AM

Move native dialog execution into OpenProjectDialog::exec().
Style fixes.

kfunk added a subscriber: kfunk.Apr 4 2016, 6:25 PM

Still quite a few coding style issues:

Note, please use:
if {
} else {
etc.

(reasoning: let's not waste vertical screen space)

shell/openprojectdialog.cpp
32

Nitpick: Don't indent inside namespaces. Aids readability in case of multi-page-long namespaces.

42

Just check KDE_FULL_SESSION once.

E.g.:
const bool useNativeFileDialog = qEnvironmentVariableIsSet("KDE_FULL_SESSION");

97

if (nativeDialog) ...

99

Should probably factor out this block into a separate function for increased readability.

126

if (openPage)

shell/openprojectdialog.h
64

/// -> then Clang recognizes it as apidoc

65

= nullptr;

70

= nullptr;

71

= nullptr;

kfunk requested changes to this revision.Apr 4 2016, 6:25 PM
kfunk added a reviewer: kfunk.
This revision now requires changes to proceed.Apr 4 2016, 6:25 PM
apol added a subscriber: apol.Apr 4 2016, 6:36 PM

Can you please explain why is this required?
It would also be good to know what's the end result, UI-wise.

kfunk added a comment.Apr 4 2016, 6:47 PM

@apol: We'd like to show the native file dialog on OS X + Windows.

Right now we're using the file open widget (KFileWidget) embedded in the open project wizard. In order to show the native file open dialog, you need to show it as separate top-level window though.

So with this patch we'll first show a standalone file-open dialog, then proceed with the "open project" wizard as usual.

apol added a comment.Apr 4 2016, 10:01 PM

@kfunk so the idea is to actually embed the QFileDialog as a widget? O.o

kfunk added a comment.Apr 4 2016, 10:08 PM
In D1298#24627, @apol wrote:

@kfunk so the idea is to actually embed the QFileDialog as a widget? O.o

No, no. As I said, we'd first like to show a standalone + native file open dialog.

apol added a comment.Apr 4 2016, 10:13 PM

Ok, interesting. :) Looking forward to see what it feels like.

arrowd updated this revision to Diff 3134.Apr 5 2016, 6:32 AM
arrowd edited edge metadata.
arrowd marked 8 inline comments as done.

Address some comments, more style fixes.

arrowd added inline comments.Apr 5 2016, 6:33 AM
shell/openprojectdialog.cpp
99

Uh, sorry, didn't get that. You mean factoring whole while loop?

kfunk added inline comments.Apr 5 2016, 11:35 AM
shell/openprojectdialog.cpp
97

if (nativeDialog) -> then you also don't need to make useKdeFileDialog global.

99

Yes, so this reads:

if (nativeDialog) {
   if (!execNativeDialog()) {
      reject();
      return QDialog::Rejected;
   } 
}

...
arrowd updated this revision to Diff 3157.Apr 6 2016, 8:59 AM
arrowd edited edge metadata.

Address comments.

arrowd marked 2 inline comments as done.Apr 6 2016, 9:01 AM
kfunk requested changes to this revision.Apr 8 2016, 8:22 PM
kfunk edited edge metadata.

I just gave this a try. The "All Projects" filter does not work on Linux when KDE_FULL_SESSION is not set. It filters out everything.

shell/openprojectdialog.cpp
65

Are you sure you need ";;" here? Not " "?

http://doc.qt.io/qt-5/qfiledialog.html#setNameFilters

This revision now requires changes to proceed.Apr 8 2016, 8:22 PM
arrowd updated this revision to Diff 3230.Apr 9 2016, 6:03 AM
arrowd edited edge metadata.

Speculative fix for file filters.

arrowd added a comment.EditedApr 9 2016, 6:20 AM
In D1298#25356, @kfunk wrote:

I just gave this a try. The "All Projects" filter does not work on Linux when KDE_FULL_SESSION is not set. It filters out everything.

Hum. It works for me and i haven't touched the logic here. If you want, i could try to set up a Unix environment and debug this myself, but this would take quite a time.

kfunk accepted this revision.Apr 10 2016, 5:35 PM
kfunk edited edge metadata.

Looks good to me. Great work!

Tested both with and without KDE_FULL_SESSION on Linux. Please do the same on Windows, and check if everything works as expected.

shell/openprojectdialog.cpp
108

Style:

} else {
    return false;
}
This revision is now accepted and ready to land.Apr 10 2016, 5:35 PM
arrowd updated this revision to Diff 3270.Apr 11 2016, 6:23 AM
arrowd marked an inline comment as done.
arrowd edited edge metadata.

Style fix.

Tested on windows with and without KDE_FULL_SESSION,