Review Request 127827: open/fetch project: restore broken workflows after native file dialog port

Thu May 12 20:58:02 UTC 2016

> On May 11, 2016, 12:24 a.m., Aleix Pol Gonzalez wrote:
> > Doesn't that change the behavior for the regular non-native use-case?
> 
> Sven Brauch wrote:
>     What is the "regular non native-use case"?
>     It changes behaviour, but right now the behaviour is wrong, so that is a good thing ;)
> 
> René J.V. Bertin wrote:
>     Maybe Aleix meant non-native = not pure QFileDialog, referring to the fact that under X11 (and Wayland?) KDevelop uses KDE file dialogs rather than QFileDialogs?
> 
> Sven Brauch wrote:
>     No, instantiating QFileDialog under Plasma uses the KDE file dialog because of the platform plugin which is being loaded. The code before doing this differentitation again made no sense whatsoever in the first place.
> 
> Aleix Pol Gonzalez wrote:
>     No, we use the KFileWidget directly.
> 
> Sven Brauch wrote:
>     But is that worth keeping two code paths?
> 
> René J.V. Bertin wrote:
>     FWIW, one thing I've always appreciated is the fact that the directory selection dialog you get when configuring a cmake project (selecting the build dir) is native on OS X. I vastly prefer that one over KDE's version which is (pardon, seems) inspired by the one from MS Windows.
> 
> Kevin Funk wrote:
>     @scummos: I'm with Aleix here. Please restore the behavior we had prior to the change (use KFileWidget) for regular KDE users (or those who want to have KDE-like file dialogs). Worth having the two code paths here, IMO, yes.
> 
> René J.V. Bertin wrote:
>     You mean the behaviour which prompted me to send a message to kdevelop-devel?
>     
>     >I just noticed that KDevPlatform now falls back to using QFileDialog when KDE_FULL_SESSION isn't set.
>     >
>     >Not that this is completely devoid of justification, but isn't that what happens automatically anyway when the framework integration plugin isn't installed, or the KDE platform theme isn't being used (for instance because KDE_FULL_SESSION isn't set, on Linux)?
>     
>     Having to set `KDE_FULL_SESSION` to get KDE file dialogs is a bit problematic on non-Linux platforms, IMHO. It's semantically wrong (as it implies a Plasma session) and can have unwanted side-effects.
>     I'd vote for a behaviour where `KDE_FULL_SESSION` isn't checked on OS X, MSWin etc, but assumed to be true ... and then let it depend on how KF5 is installed and configured what file dialog is used.
>     
>     Actually seems a good principle to me. Applications should only rarely be concerned about the kind of dialog they present but rather leave that to the session or OS configuration (or else provide an option, like OpenOffice/LibreOffice does).
>     
>     This is the patch I've had to add to my port for KDevPlatform, to get back the "old" behaviour:
>     
>     ```
>     diff --git shell/openprojectdialog.cpp shell/openprojectdialog.cpp
>     index 9ccca43..34eefad 100644
>     --- shell/openprojectdialog.cpp
>     +++ shell/openprojectdialog.cpp
>     @@ -75,7 +75,7 @@ OpenProjectDialog::OpenProjectDialog( bool fetch, const QUrl& startUrl, QWidget*
>      {
>          resize(QSize(700, 500));
>      
>     -    const bool useKdeFileDialog = qEnvironmentVariableIsSet("KDE_FULL_SESSION");
>     +    const bool useKdeFileDialog = true; /*qEnvironmentVariableIsSet("KDE_FULL_SESSION")*/
>          QStringList filters, allEntry;
>          QString filterFormat = useKdeFileDialog
>                               ? QStringLiteral("%1|%2 (%1)")
>     diff --git shell/documentcontroller.cpp shell/documentcontroller.cpp
>     index 0176c2e..5b44837 100644
>     --- shell/documentcontroller.cpp
>     +++ shell/documentcontroller.cpp
>     @@ -138,7 +138,7 @@ struct DocumentControllerPrivate
>              auto parent = Core::self()->uiControllerInternal()->defaultMainWindow();
>      
>              // use special dialogs in a KDE session, native dialogs elsewhere
>     -        if (qEnvironmentVariableIsSet("KDE_FULL_SESSION")) {
>     +        /*if (qEnvironmentVariableIsSet("KDE_FULL_SESSION"))*/ {
>                  const auto result = KEncodingFileDialog::getOpenUrlsAndEncoding(QString(), dir,
>                      filter, parent, caption);
>                  return {result.URLs, result.encoding};
>     ```
> 
> Kevin Funk wrote:
>     Well, either way: We need to have *some* input to know whether we should show KDE file dialogs or not. Currently we use the `KDE_FULL_SESSION` env var (which is suboptimal, I agree), but it could be *anything*. It should be a runtime option nevertheless. Not a compile-time one (as you seem to want). User preferences vary.
>     
>     Note: This is somewhat unrelated to this patch, please discuss that on the mailing list. Let's keep this one focussed on the issue we're trying to fix.

But this is a problem common to all applications. It is the same issue in every other KDE application I run outside plasma. Yes, we could introduce our own workaround, but is that really a good plan?

- Sven

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127827/#review95373
-----------------------------------------------------------

On May 3, 2016, 10:32 p.m., Sven Brauch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127827/
> -----------------------------------------------------------
> 
> (Updated May 3, 2016, 10:32 p.m.)
> 
> 
> Review request for KDevelop and Kevin Funk.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> The native dialog has two significant restrictions: it cannot be embedded, and it cannot be told to accept both a directory _or_ a file at once. The previous change to the open project dialog broke the (important) option to open a directory as a project. This fixes that through introduction of an extra step: you select the method you want to open your project with first. This has the advantage of making it more clear to the user what the options are; many users are still not aware you can simply tell KDevelop to open a folder as a project.
> 
> I'm well aware this is far from an optimal solution, but right now it's just broken and this is certainly an improvement over the current situation. Better ideas welcome.
> 
> 
> Diffs
> -----
> 
>   shell/CMakeLists.txt 83d4db0 
>   shell/openprojectdialog.h d39ff8e 
>   shell/openprojectdialog.cpp 9ccca43 
>   shell/openprojectdialog.ui PRE-CREATION 
>   shell/openprojectpage.h 1e0ff60 
>   shell/openprojectpage.cpp 42d836f 
>   shell/projectsourcepage.h a45ee19 
>   shell/projectsourcepage.cpp 43ab6e9 
>   shell/projectsourcepage.ui 79699aa 
> 
> Diff: https://git.reviewboard.kde.org/r/127827/diff/
> 
> 
> Testing
> -------
> 
> can open from CMakeLists.txt, from foo.kdev5 or from a folder; also fetch works again (before simply random stuff happened)
> 
> 
> File Attachments
> ----------------
> 
> the added dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/7b6cdfc2-c4d7-4394-9a39-2ccc923f28fa__Screenshot_20160504_001810.png
> 
> 
> Thanks,
> 
> Sven Brauch
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160512/7ea06a95/attachment-0001.html>