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

Fri Jan 16 09:35:11 UTC 2015

> On Jan. 13, 2015, 12:41 nachm., 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 ;-)

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

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

On Jan. 13, 2015, 12:40 nachm., Wolfgang Bauer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121351/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 12:40 nachm.)
> 
> 
> 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/c6f10249/attachment.html>