GNOME Bugzilla – Bug 62988
Export dialog should be WM_TRANSIENT_FOR
Last modified: 2005-09-09 18:38:09 UTC
This is related to bug #61092 ("yes/no questions should be WM_TRANSIENT_FOR dialogs") but it is better to have a separate bug report for the Export dialogs because they have specific problems that will probably not be solved easily with the changes of the query box API planned for 1.4. I expect that bug #61092 could be solved early during the 1.3.x development, while this one will probably require more changes that could break the plug-in API. The "Export" dialogs and the "Confirm Save" dialog should be transients, because they are related to the Save As window. The window manager should therefore be instructed to decorate and position them appropriately, so the WM_TRANSIENT_FOR hint should be set. Unfortunately, what appears to the user as an extension of the "Save As" window (like the "File Exists" dialog) is actually handled by the plug-in and not by the main application (contrary to the "File Exists" dialog, which is created by the core). This technical detail is not apparent to the user but it makes it difficult to set the WM_TRANSIENT_FOR hint because the plug-in does not get the id of the parent window (the "Save As" dialog). We should add a way to pass the window id from the core to the plug-ins, either in the call parameters or as a separate libgimp function that could be called by the plug-ins when necessary. In addition, it will probably be necessary to implement our own function for setting the WM_TRANSIENT_FOR hint in the plug-ins. The GTK+ function gtk_window_set_transient_for() expects a GTK_WINDOW pointer for the parent window, which does not exist in the address space of the plug-ins.
Small comment about "Severity: minor" vs. "Severity: enhancement": As with bug #61092, this should considered to be a minor bug and not an enhancement, because the WM behavior is broken because of this. This may not be apparent with a WM that ignores WM_TRANSIENT_FOR hints or that only causes the decoration to be different, but this becomes significant when one uses a WM that sets other parameters of the window depending on whether a window is transient or not. For example, the differences are obvious if the placement is manual for top-level windows and automatic for transients, or if the default placement for top-level windows is to find some free space on any workspace while the transients always choose the same workspace as their parent. A user who has his WM configured in such a way (which works will with most of the other applications) will consider the GIMP to have a slightly broken user interface.
Why was this changed from "minor" to "enhancement" without a word of explanation? The severity "minor" is defined as: "The component mostly works, but causes some irritation to users. A workaround should usually exist." This is precisely the case here. The Inter-Client Communication Conventions Manual says that "dialogue windows [...] should have WM_TRANSIENT_FOR set" and the GIMP is currently not following the ICCCM for some windows. So this is a bug, not just a feature that is nice to have.
Chnaged the subject to refer only to Export dialogs since the Confirm Save dialog has been addressed.
This is certainly not a bug that deserves the easy-fix keyword.
Bumping to 2.2, unless someone says they will address this for 2.0 (in which case, the milestone should be changed to 2.0) - in any case, it's not a blocker for the pre-releases. Dave.
We can keep this open for the moment, but I think that we will eventually close it as WONTFIX if the solution proposed in bug #119545 is implemented. This would be a better way to solve this problem, by getting rid of the separate Export dialog.
I don't think bug #119545 proposes a reasonable alternative. IMO it would be a good thing to somehow pass the window ID of image displays to the plug-ins. Not only the export dialogs would benefit from this.
Created attachment 51836 [details] [review] draft of a patch to pass the display's window ID to plug-ins This patch is work in progress. Currently it crashes. I am attaching it here so that it doesn't get lost and to allow others to comment on the API that it introduces.
The patch got fixed and applied last night. However it doesn't fix the problem for load and save plug-ins. They aren't called with an image display and thus can't be set transient to it's window.
Fixed in CVS: 2005-09-09 Michael Natterer <mitch@gimp.org> Added parent window API to the GimpProgress interface and to the libgimp progress stuff. Might look strange, but does the right thing in almost all cases (image window, file dialog, script-fu dialog etc). Fixes bug #62988. * app/core/gimpprogress.[ch]: added GimpProgress::get_window() which should return a toplevel window ID if the progress is in a window that wants to be the transient parent of plug-in dialogs. * app/widgets/gimpwidgets-utils.[ch] (gimp_window_get_native): new function which returns the window handle of a GtkWindow's GdkWindow. * app/widgets/gimpfiledialog.c: implement ::get_window(). * app/display/gimpdisplay.[ch]: ditto. Removed window handle API. * app/gui/gui-vtable.c: changed accordingly. * libgimpbase/gimpbaseenums.[ch] (enum GimpProgressCommand): added GIMP_PROGRESS_COMMAND_GET_WINDOW. * app/plug-in/plug-in-progress.[ch] (plug_in_progress_get_window): new function. Also renamed some functions to match the GimpProgress interface, and not the legacy PDB procedure names. * tools/pdbgen/pdb/progress.pdb * app/core/gimppdbprogress.c: implement get_window() on both sides of the wire, keeping backward compatibility (hopefully). * libgimp/gimpprogress.[ch]: deprecated gimp_progress_install() and added gimp_progress_install_vtable() which takes a vtable with padding to be extensible. Added get_window() vtable entry and dispatch it accordingly. Also added pulse() which was implemented in a hackish way before. Everything is of course backward compatible. * libgimp/gimpprogressbar.c: inmplement the get_window() stuff so a plug-in dialog containing a progress can be the transient parent of another dialog in another plug-in. * libgimp/gimpui.[ch] (gimp_ui_get_progress_window): new function which returns a foreign GdkWindow of this plug-ins progress window. Renamed gimp_window_set_transient_for_default_display() to gimp_window_set_transient() and make it use the progress' window handle instead of the display's (which is the right thing to do in almost all cases). * libgimp/gimp.def * libgimp/gimpui.def: add the new functions. * tools/pdbgen/enums.pl * app/pdb/internal_procs.c * app/pdb/progress_cmds.c * libgimp/gimpprogress_pdb.[ch]: regenerated. * libgimp/gimpexport.c * plug-ins/*/*.c: follow API change.
Really fixed now: 2005-09-09 Michael Natterer <mitch@gimp.org> * plug-ins/common/aa.c * plug-ins/common/csource.c * plug-ins/common/gbr.c * plug-ins/common/gih.c * plug-ins/common/gtm.c * plug-ins/common/mng.c * plug-ins/common/pat.c * plug-ins/common/png.c * plug-ins/common/pnm.c * plug-ins/common/postscript.c * plug-ins/common/psp.c * plug-ins/common/raw.c * plug-ins/common/sunras.c * plug-ins/common/tga.c * plug-ins/common/tiff.c * plug-ins/common/xbm.c * plug-ins/common/xpm.c * plug-ins/gfli/gfli.c * plug-ins/jpeg/jpeg-save.c * plug-ins/sgi/sgi.c * plug-ins/winicon/icodialog.c: actually call gimp_window_set_transient() on save dialogs. Really fixes bug #62988.