Bug 404833

Can you make a screenshot of the issue?

Created attachment 118373 [details]
Scribus startup wizard with KDE platform dialog

Screenshots of the 2 examples mentioned above.

Some tracing shows that KDEPlatformFileDialogHelper::show() is called with windowflags `WindowTitleHint|WindowSystemMenuHint|WindowMinMaxButtonsHint|WindowCloseButtonHint|WindowFullscreenButtonHint`. The parent is as expected, but injecting Qt::Widget into the window flags does not have any effect. Also suspicious is the fact that the modal flag is false while Scribus does request a modal dialog.

Created attachment 118374 [details]
Scribus open file dialog

this also shows the preview feature not working

Created attachment 118375 [details]
Scribus open file dialog using hand-rolled KFileDialog

This is how the open file dialog could look, after I hand-rolled a KFileDialog implementation that embeds a KFileWidget into a generic QDialog.

Note that the startup wizard is not a QDialog but a QFrame; the open file dialog uses an `ScFileWidget` instance which inherits QFileDialog (in stock scribus).

PS: KIO really should reintroduce that class, KFileWidget is *less* than the platform dialog minus the QDialog!

From what I recall you don't actually get access to the KDE dialog through the QFileDialog. The helper creates the dialog as window without a parent and there's no getter for it.
That's also why the file dialog isn't modal in e.g. Plasma wallpaper dialog as I cannot access the dialog to set a transient parent on it, it only does so magically when "parented to a QWidget" which isn't the case in QML.

QML issues aren't relevant here, Scribus doesn't use that (at all AFAICT).

> The helper creates the dialog as window without a parent and there's no getter for it.

"It" being what? You mean that you cannot get at the actual QFileDialog instance?

Would it be possible to use an existing metadata feature (a la QAction::setData) to communicate special-needs information from the application to the integration plugin? I suppose something like QPlatformNativeInterface isn't usable from platform *theme* plugins, to expose specific functions to the application?

QFileDialog creates a platform file dialog helper (KDEPlatformFileDialogHelpern in plasma-integration) and then asks it to create a dialog which we do. However, the QFileDialog you create on the clientside is not the actual window that's being shown, instead it's an internal window created by the dialog helper. Check the code in plasma-integration and you'll see that this is more of an architectural limitation/design choice in Qt. If you find a solution for getting the actual window, that doesn't involve using private Qt headers, much appreciated.

I seem to recall discussing the use of private headers in the integration plugin before, and that that was deemed acceptable (was a while ago though).

It might also be possible for the client/application to get at the actual widget, to special-case the situation where a KFileWidget is found. I don't see what you'd be able to do with that, though.

I'm a bit confused. IIUYC, the client/application side sees a QFileDialog instance that is not the one that is actually being shown, while the dialog that does appear on the screen is the one created/inherited by KDEPlatformFileDialog?

In fact, what would you want to do with the "actual window"?

From looking at the integration code it looks like you'd need that information when you create the KDEPlatformFileDialog instance, to use instead of the new QDialog.

The application side could use findChildren() to dig up the KFileWidget instance and when one is found, use QObject::setProperty() to communicate the actual QFileDialog, but then what? What *can* you do if the QFileDialog instance seen by the application is not the window that's shown?

OTOH, if the actual QDialog being shown is the one created by KDEPlatformFileDialog, can the application get at that instance? If so it could use a QFileDialog proxy that transfers all QFileDialog specific calls to the KDEPlatformFileDialog instance, and use that proxy instead of inheriting QFileDialog directly? This does suppose that KDEPlatformFileDialog provides a complete enough QFileDialog API...

> and that that was deemed acceptable (was a while ago though).

It still is as that binds your application to a specific Qt version and requires recompilation on every Qt version update (even minor). Getting this done right in KWin (which uses quite some private Qt headers these days to provide Wayland integration) took a while.

> what would you want to do with the "actual window"?

Embed it like you asked for? Or set a transient parent on it to make it modal to a non-widget dialog

> The application side could use findChildren()

the window doesn't have a parent and isn't a child of the QFileDialog.

> QFileDialog proxy

We had that in KDE4, it was called KFileDialog, and got abandoned in favor of a QFileDialog + QPT approach which will get you native file dialogs on other platforms. I don't see how QFile*Dialog* would be something that is supported to be embedded elsewhere. In KDE code we would embed a KFileWidget. However, there's also the issue of not being able to add custom widgets to such dialogs, since the native dialogs aren't real Qt widgets you could add a widget to.

(In reply to Kai Uwe Broulik from comment #10)
> > and that that was deemed acceptable (was a while ago though).
> 
> It still is as that binds your application to a specific Qt version 

I was talking about the plasma integration plugin. Binding that one to a specific Qt version shouldn't be much of an issue - and usually isn't when you only upgrade the Qt version you're using it with.

> Embed it like you asked for?

How would you do that?

> We had that in KDE4, it was called KFileDialog, and got abandoned in favor
> of a QFileDialog + QPT approach which will get you native file dialogs on
> other platforms.

[OT]
If we disregard the question whether or not QFileDialog should only give platform-native dialogs there is the issue that this approach makes it cumbersome for applications to provide a "KDE-style" dialog everywhere by using something that would logically be called KFileDialog. There are lots of cross-platform KDE widgets that extend or improve on Qt counterparts, why wouldn't there be one for file dialogs? It's not like every platform has equally featurerich/convenient file dialogs as the KDE one. And it isn't like it's a shocking crime to want to offer something better to your users. Or to offer a homegeneous experience regardless of what platform you're running on.

I've gotten a glimpse of what it takes to implement a KFileDialog class that's based on KFileWidget, and that duplicates much of the code which is also in the integration plugin. To me it would make sense to reintroduce KFileDialog, and use that class in the integration plugin. KDE Applications can be coerced through the KDE HIG to keep using QFileDialog, but other applications no longer have to duplicate "jealously guarded" code ;)

> I don't see how QFile*Dialog* would be something that is
> supported to be embedded elsewhere.

My second patch on the Scribus ticket linked under "additional info" above does exactly that.

> KFileWidget. However, there's also the issue of not being able to add custom
> widgets to such dialogs, since the native dialogs aren't real Qt widgets you
> could add a widget to.

You could be wrong there. Qt's native widgets look native but aren't necessarily the exact same ones you get when running native non-Qt code - at least not on Mac. Or they are, but native widgets that are displayed in a window that's also some kind of QWidget.
I see that the Mac native file dialogs also go through QPlatformFileDialogHelper, and it seems you can indeed not embed those either. But that's not really relevant here...

I know you said "without use of private APIs" ... but it seems that
- the parent received in KDEPlatformFileDialogHelper::show() is a QWidgetWindow
- QWidgetWindow::widget() should give us the widget instance used in the application

I couldn't get the code to build when trying QWidgetWindow::widget(), but there's an easier, non-private way:

        QWidget *parentWidget = parent ? QWidget::find(parent->winId()) : nullptr;

This works on Mac, e.g. "QWidgetWindow(0x7fcd05b97fd0, name="KIconDialogClassWindow")" becomes "KIconDialog(0x7fff586d1ba8)" (in kicondialogtest from the KIconThemes tests; KIconDialog is used as a parent for QFileDialog).

If it also works on Linux:
- the application could call QFileDialog::setProperty() to communicate relevant information (the QFileDialog instance?)
- the KDEPlatformFileDialogHelper could also use setProperty() on the widget obtained, e.g. to signal that the KDE dialog is in use.

Created attachment 118390 [details]
testing mod

So this works on Linux too, see the attached patch.

I've been testing this with an application that does

        auto fDialog = new QFileDialog(this, tr("Pick a font file"), startDir);
        auto variant = QVariant(QVariant::UserType);
        variant.setValue(fDialog);
        fDialog->setProperty("QFileDialogInstance", variant);
        fDialog->setProperty("QFileDialogParentClass", metaObject()->className());
        qWarning() << "\t" << fDialog << fDialog->property("QFileDialogInstance")
            << fDialog->property("QFileDialogParentClass");
        if (fDialog->exec() && !fDialog->selectedFiles().isEmpty()) {
        }

This prints

         QFileDialog(0x1749cc0) QVariant(QFileDialog*, QFileDialog(0x1749cc0)) QVariant(QString, "Dialog")
virtual bool KDEPlatformFileDialogHelper::show(Qt::WindowFlags, Qt::WindowModality, QWindow *) KDEPlatformFileDialogHelper(0x16e5530) KDEPlatformFileDialog(0x170ba10) QWidgetWindow(0x16ba7f0, name="DialogClassWindow") Dialog(0x7fff972af318)
         QVariant(Invalid) QVariant(Invalid)
         (QFileDialog(0x1749cc0))

Here the 1st line comes from the application, the other 3 lines from KDEPlatformFileDialogHelper::show(). 
As you can see, the properties are set correctly but for some reason they cannot be read from the integration plugin. Maybe after a dynamic_cast to QFileDialog?
However, findChildren() is promising and could be the solution if we can assume that there are never multiple QFileDialog instances that share the same parent (or if we can distinguish them somehow).

FWIW, I've even seen "(ScFileWidget(0x2fa45b0))" when opening a native filedialog in Scribus, so findChildren() also works on classes that inherit QFileDialog.


Now, how do you think we could use this information to embed the KDEPlatformFileDialog into the parent of the instance we just retrieved?

(In reply to RJVB from comment #13)

>         fDialog->setProperty("QFileDialogInstance", variant);

Doh, that has to be

        setProperty("QFileDialogInstance", variant);

of course. That property *is* accessible from the plugin. And then the other property becomes pointless.

Created attachment 118399 [details]
"kinda works" mod

Here's a hack that almost works, without any checking whether the action is appropriate.

- find the parent QWidget
- from that, obtain the (first) (user-side) QFileDialog instance
- if the parent QWidget has a layout, replace the found QFileDialog instance with our own dialog
- save the original user-side QFileDialog instance
- in the dtor, either restore the original QFD in the layout, or call deleteLater() on it (and set it to NULL in case the action causes recursive calling of the dtor).

Glitches:
- dialogs may end up with 2 sets of OK/Cancel/etc. buttons
- if not, these buttons may actually close only the embedded QFD and not the enclosing dialog (seen with https://gist.github.com/eyllanesc/7cf200538e182b5662552700c27b998a)
- resizing works but the saveSize/restoreSize mechanism doesn't
- AFAICT all signals aren't connected properly (the preview in the Scribus open-file dialog doesn't react to selecting a file as it should). File opening does work though.

Just reparenting m_dialog (as done in the out-commented code) isn't sufficient because it will not show up correctly in the layout, and won't resize with the dialog.

Scribus is still going to break when they run in a flatpak and we use the XDG portal QPT where there isn't a window to embed.

Inevitably Scribus is going to need to change.  Putting hacks in just one QPT won't fix that.

I'm not really interested in what happens with distribution hacks like that, TBH; IMHO you can't expect those to give the same experience as a "normal" install will.
Currently Scribus would need to change from using non-native dialogs, and there must be ways for them to determine at runtime (or build-time) that they in fact keep using those.
And that will give a workable result if we can get the plasma-integration plugin to support embedding properly. And frankly, if at all feasible that's a goal worth pursuing independent from Scribus and how software is distributed. I spent some time on Google earlier today and the question of adding widgets to a QFileDialog (one of the reasons for embedding it) is a recurrent one. All answers point out you have to use non-native dialogs, or assume you are - and that's just not satisfactory.

You can't have a native UI and meddle with how the UI is presented. 
The two concepts are entirely at odds with each other.

If that's in reaction to the idea of adding things to a native file dialog, please think again. Mac and (unless I'm mistaken) MS Windows (where you have a real native file dialog, provided by the platform through the QPA plugin) provide native API to do this kind of thing. Those APIs allow exactly what applications like Scribus try to achieve in Qt: showing additional controls around the core feature of the dialog (the file selection widget with its controls).

Under X11 and (AFAIK) Wayland there is in fact no "native dialog" (if anything that would be Qt's...!). It is the QPT integration plugin which provides a dialog which is deemed more appropriate for the desktop environment in use than Qt's own file dialog.

According to your point of view KDE applications that construct their own enhanced file dialog from a KFileWidget plus whatever they find justified would be doing something wrong? I just cannot subscribe to that idea.

> Mac and (unless I'm mistaken) MS Windows provide native API to do this kind of thing. 

You still cannot just add a QWidget to a native dialog. Qt would need to add elaborate wrapper APIs around those native functions to enable you to add some custom content.

> According to your point of view KDE applications that construct their own enhanced file dialog from a KFileWidget plus whatever they find justified would be doing something wrong?

This bug report was about embedding random dialogs into random other dialogs. Constructing your own file chooser using KDirNavigator and whatever they are called is a valid and supported use-case.

Also, judging from the bug report the same happens with the GTK file dialog and others. Moreover, Scribus unconditionally disables native dialogs on all patforms, it's not specific to Plasma, as it can't be done.

Closing this now, as it's really out of scope for KDE, and more of a Qt architectural problem.

(In reply to Kai Uwe Broulik from comment #20)

> This bug report was about embedding random dialogs into random other
> dialogs.

Not embedding random dialogs but either way *I* didn't bring up the argument that you're supposedly not allowed to mess with native UIs. I just pointed out why that argument is moot here.

> and others. Moreover, Scribus unconditionally disables native dialogs on all
> patforms

That's also not really relevant. Scribus is just an example and the app that inspired this report.

> it's not specific to Plasma, as it can't be done.

> Closing this now, as it's really out of scope for KDE, and more of a Qt
> architectural problem.

And that's a pity because I'm not convinced that it can not be done with the KDE file dialog. That may be the sole exception where it *can* be done because it's built entirely out of Qt components. Whether or not it's possible on other platforms shouldn't be a concern here (otherwise the preview feature should go ;)).

My latest PoC hack is stable, and I'm almost convinced that someone more familiar with the intricacies of writing file dialog helpers and the implementation of this particular one could reconfigure the signal connections and get the size restore to work

> Not embedding random dialogs but either way *I* didn't bring up the argument that you're supposedly not allowed to mess with native UIs. I just pointed out why that argument is moot here.

It's not that you're not allowed to, it can. not. be. done. If Qt were wxwidgets which uses native controls on all platforms, maybe, but this is not the case with Qt.

> And that's a pity because I'm not convinced that it can not be done with the KDE file dialog. That may be the sole exception where it *can* be done because it's built entirely out of Qt components. 

Look at xdg-desktop-portal, it uses a QDialog, slaps a KFileWidget into it, wires up the QDialogButtonBox to interact with it, and done. 10 lines of code.

> My latest PoC hack is stable,

As you said, it's a hack, and we have supported usecases to add a file chooser to a custom wizard dialog, namely the one I outlined above. Since you can't do that stuff in a platform-independent way, you might as well do it properly and use the correct APIs directly rather than relying on some magic properties set by some plugin.

>It's not that you're not allowed to, it can. not. be. done. If Qt were

The only thing one can affirm here is that it cannot be done the same way on all platforms and possibly that there are platforms where it is entirely impossible to add anything to the native file widget. One can probably also say that on most platforms this might require changing the QPA rather than a QPT.
But we're not talking about those platforms here.

>Look at xdg-desktop-portal, it uses a QDialog, slaps a KFileWidget into it,
>wires up the QDialogButtonBox to interact with it, and done. 10 lines of code.

How is that relevant?

>As you said, it's a hack

And yet it's what you'd do if Qt gave access to the dialog that is actually being shown ... or if they hadn't gone with this sort of phantom sibling dialog situation (can we agree that's a hack in itself?).

>that stuff in a platform-independent way, you might as well do it properly and
>use the correct APIs directly rather than relying on some magic properties set
>by some plugin.

Again, that's like saying the preview widget shouldn't be there because it cannot be provided on all platforms, and so if code wants to rely on that feature it needs to implement it itself. Instead, the KDE QPT implements it, presumable because it's seen as a potential convenience for applications and users that somehow get to use the KDE file dialog. I don't see why that couldn't apply to support for embedding.

As to using the KDE APIs directly: you'll notice that I followed that approach before filing this ticket, and that it took considerably more than 10 lines. Every application would have to duplicate that (which is an already cited argument for bringing back a complete KDE file dialog class). You'll notice too that this is not acceptable for at least 1 of the Scribus people as just installing KIO requires about 100 additional packages.
That may not sound like a problem for anyone who already has KDE installed ... but if you do you probably also have the plasma integration plugin installed.