[Okular-devel] Review Request 121351: Make the printer configuration dialog non-modal

Fri Jan 16 17:08:34 UTC 2015

> On Jan. 13, 2015, 11:41 vorm., Christoph Feck wrote:
> > It looks like Daniel cannot provide feedback right now.
> > 
> > Wolfgang, does changing the modality of the configure dialog mean it can now be openend twice?
> 
> Wolfgang Bauer wrote:
>     As far as I can tell, no.
>     If I click on "Configure" for the same printer, the already opened dialog is given focus, no new dialog is opened.
>     
>     It is possible to open a new configure dialog for a different printer now though when one is open already, which even makes sense somehow IMHO.
> 
> Thomas Lübking wrote:
>     A modal window is ALWAYS modal for exactly one other window.
>     The assumption
>     "Launching a modal dialog (for example, as password dialog) from a modal dialog won't give the focus to the second dialog." in bug #314633 is -generally- wrong for sure.
>     
>     Beyond this, Qt supports client wide modality, but that's about event processing.
>     
>     You can set QWidget::setWindowModality(Qt::WindowModal) rather than QWidget::setModal(true) (the latter implies QWidget::setWindowModality(Qt::ApplicationModal) to impact this.
>     
>     Notice that Qt will set the main window to the parent window - you must use KWindowSystem::setMainWindow(QWidget*, WId) if you need anything special.
>     
>     KWin (unlike mutter) does not support some sort of system wide modality at all. I cannot say what exactly causes the behavior of bug #314633 but a modal dialog for MainWindow A has no impact on windows in clients B unless A and B have a common MainWindow C which is NOT the root window.
>     
>     
>     ==> The bug is here:
>     https://projects.kde.org/projects/kde/kdeutils/print-manager/repository/revisions/1595ef0614f824a6a871d51327eff3a108e6e251
>     
>     Why would the password dialog block plasma? Because it's ApplicationModal or because it was set modal for some common plasma dialog dummy window?
>     If the dialog has an actual (visible, real, probably not the desktop or a panel) parent window, it should be WindowModal for that one window only. (I strongly assume the the password dialog better should be modal since its input is probably expected by some other window?!) If not (ie. the dialog spawns out of the void), then of course it should be not modal at all.
> 
> Wolfgang Bauer wrote:
>     I fully agree that the password dialog should better be modal.
>     But as I understand bug #314633, the password dialog can also be shown directly by the plasmoid, i.e. it _can_ spawn out of the void apparently. 
>     I never saw that problem though (I don't use remote printers), and AFAICS the real cause was never identified sufficiently.
>     
>     OTOH, I don't really see a point for making the "Configure Printer" dialog modal.
>     Why should it be prevented to do some other stuff in the KCM while that dialog is open, like cleaning the heads or printing a test page.
> 
> Thomas Lübking wrote:
>     Modality should be client controlled then (ie. the printer dialog would set it QWidget::setWindowModality(Qt::WindowModal) - it MUST be a child of the printer dialog, resp. through KWindowSystem::setMainWindow(passwordDlg, printerDlg->winId()) )
>     
>     I would instinctively agree on the printer dialog modality, but can't say - I installed printer stuff only back then to check the modality constallation but am not in tree killing business otherwise ;-)
> 
> Wolfgang Bauer wrote:
>     So if I understand you correctly, dialog->setWindowModality(Qt::WindowModal) should be called instead of dialog->setModal(), and then KWindowSystem::setMainWindow() to specify the window that should be blocked (i.e. the "Configure Printer" dialog in this case).
>     
>     AFAICS KWindowSystem::setMainWindow() is actually called already (added by the mentioned commit), so all that would be left is change dialog->setModal() to dialog->setWindowModality(Qt::WindowModal) in KCupsPasswordDialog.cpp?
>     
>     But KWindowSystem::setMainWindow() should be called *before* dialog->show() (not afterwards as it is now) if I correctly understand the documentation, right?
>     
>     And I think the KWindowSystem::forceActiveWindow(dialog->winId()) should be removed as well, or not?
>     
>     ---
>     
>     Regarding *this* review request: configure-printer (i.e. the printer configuration dialog) is a separate application anyway (/usr/lib64/kde4/libexec/configure-printer on my system, it is started via DBUS). So the setModal(true) here is totally useless I'd say.
>     It has absolutely no effect on the KCM window at all, that one is not blocked even without this patch. I noticed this already when I came up with this patch, but somehow forgot to mention that here.
>     
>     So I think there's no need to discuss whether the "configure printer" dialog should be modal or not.
> 
> Wolfgang Bauer wrote:
>     PS: I did notice a change now:
>     When setModal(true) is called, all other open "Configure Printer" dialog windows are blocked when you open a new one.
>     I'm not sure whether this is desirable or not though. But I'd rather say no. It should not matter in which order you press OK/Cancel in the open windows I think...

> dialog->setWindowModality(Qt::WindowModal) should be called instead of dialog->setModal()

This will block only the event processing for the main window of the dialog (in the client) - other windows would not be affected.
So if you'd like to be able to open multiple printer config dialogs (in one process), this would be the correct statement - yes.

> and then KWindowSystem::setMainWindow()

NOTICE:
There are TWO INDEPENDENT states of MODALITY involved here
a) the window manager won't pass focus to a (usually ONE) window that is blocked by a modal dialog. The WM operates on the relations hinted by XWindow properties and is affected by KWindowSystem::setMainWindow()
b) the client (Qt) maintains its own modality concept. setWindowModality(Qt::WindowModal) does ONLY make sense if "modalDialog = new QDialog(m_mainWindow);"
If the dialog has no usable parent, this is useless (Qt can either stall eventprocessign while the dialog is up, or not)
If the dialogs parent is sane, Qt will implicitly do what KWindowSystem::setMainWindow() would do.
==> You do only have to mess around with KWindowSystem::setMainWindow() if mainwindow and modal dialog are in different processes. Otherwise just make the dialog qobject child of its mainwindow and set setWindowModality(Qt::WindowModal) as it pleases you.

> KWindowSystem::forceActiveWindow(dialog->winId()) should be removed

If a modal dialog is opened for a window tha has currently the focus, KWin will pass the input focus to the dialog - you don't have to manage focus if the windows hint their relations in the correct way (or it's a kwin bug ;-)

- Thomas

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

On Jan. 13, 2015, 11:40 vorm., Wolfgang Bauer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121351/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 11:40 vorm.)
> 
> 
> Review request for Okular, Print Manager, Daniel Nicoletti, and Thomas Lübking.
> 
> 
> Bugs: 328014
>     http://bugs.kde.org/show_bug.cgi?id=328014
> 
> 
> Repository: print-manager
> 
> 
> Description
> -------
> 
> [Commit 1595ef0614f824a6a871d51327eff3a108e6e251](https://projects.kde.org/projects/kde/kdeutils/print-manager/repository/revisions/1595ef0614f824a6a871d51327eff3a108e6e251) (a partial fix for bug 314633) changed the password dialog to be non-modal.
> But the printer configuration dialog is still modal, so if a password is needed to apply/save the configuration it cannot be entered because the password dialog cannot get focus.
> 
> This patch sets the configuration dialog to be non-modal as well to prevent this problem.
> 
> 
> Diffs
> -----
> 
>   configure-printer/ConfigureDialog.cpp ace91a2 
> 
> Diff: https://git.reviewboard.kde.org/r/121351/diff/
> 
> 
> Testing
> -------
> 
> Open the "Printer" KCM in systemsettings, select a printer, click on "Configure", change some setting and click "Apply" or "OK".
> A password dialog appears (unless you have the necessary privileges to change the CUPS settings of course), this has focus and you can actually enter the username/password now whereas you could not without this patch.
> 
> Also tested by other users and included in openSUSE's official packages, see https://bugzilla.opensuse.org/show_bug.cgi?id=889187#c7.
> 
> 
> Thanks,
> 
> Wolfgang Bauer
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20150116/2c9edfa8/attachment-0001.html>