After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 700880 - Closing an image will not cause plug-in dialogs to close as well
Closing an image will not cause plug-in dialogs to close as well
Status: RESOLVED OBSOLETE
Product: GIMP
Classification: Other
Component: User Interface
2.8.4
Other All
: Normal normal
: ---
Assigned To: GIMP Bugs
GIMP Bugs
: 724447 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-05-23 10:34 UTC by gedon
Modified: 2018-05-24 13:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
plugin-cancel-when-close.patch (5.55 KB, patch)
2013-06-18 18:58 UTC, Hartmut Kuhse
none Details | Review
plugin-cancel-when-close_1 (14.99 KB, patch)
2013-06-22 20:31 UTC, Hartmut Kuhse
needs-work Details | Review
prevent_item_close_plugin (17.85 KB, patch)
2013-06-22 20:34 UTC, Hartmut Kuhse
none Details | Review

Description gedon 2013-05-23 10:34:03 UTC
It's not a bug, but some cosmetics:

How to reproduce:
- open a picture
- select a Plugin/Script/Filter, but don't apply it
- close the picture

In Windows, the Plugin/Script/Filter-dialog box is still „activ“ and can be opened from Windows Task Bar. This makes no sense, as the picture was already closed. A Gimp-Error-Message „tries reading from invalid drawable 2 (killing)“ will be shown.

Expected Behavior: 
Closing a picture should also close related dialog boxes.
Comment 1 Michael Schumacher 2013-05-24 11:58:10 UTC
If someone fixes this:

The close must not happen for dialogs which do not need an image, of course.
Comment 2 Max Mustermann 2013-06-12 03:36:59 UTC
Confirming for GIMP on OS X.
Increasing importance as it's not only a cosmetic flaw, but the open dialog itself becomes useless to use (it's useless to provide a 'feature' that _will_ fail).
Comment 3 Hartmut Kuhse 2013-06-18 18:58:40 UTC
Created attachment 247202 [details] [review]
plugin-cancel-when-close.patch

The proposed patch will fix this for plug-ins, not for scripts.
I chose the way, that every open plug-in "knows" which items are open.
I connected to the "removed" signal of the item and if a "known" item is removed, the plugin will close also.
I chose the way, because it it possible to open more than one plugin to an item.
It is also possible to have more than one plugin open for different items.
I don't know, why somebody should do this, but it is possible.
So deleting just one item will only close the plug-ins, that are related to the item.

For the file-open/save dialog, there is no modality defined. Defining the modality would kill that problem. I didn't add that to the patch, because there must be a reason, why the dialogs are not modal.
I suggest changing the modality for that dialogs.

For scripts the problem is much deeper and I don't have the knowledge, sorry.
Comment 4 Michael Natterer 2013-06-18 21:27:05 UTC
Nice approach, but IMO belongs into GimpPlugProcFrame.
Comment 5 Hartmut Kuhse 2013-06-22 20:31:12 UTC
Created attachment 247529 [details] [review]
plugin-cancel-when-close_1

Yeah, right. I did. But I get to a point, where I can not go further:
If the "removed" signal from GimpItem is fired, I try to close related plugins. That works. But I cannot close script-fu scripts. I don't know, how to stop a running script savely. If there is an error, the script-fu plug-in will be closed. So, if anybody has an idea: be my guest!
Comment 6 Hartmut Kuhse 2013-06-22 20:34:23 UTC
Created attachment 247531 [details] [review]
prevent_item_close_plugin

I made an other approach:
If a plugin or a script-fu script is open, the related item cannot be removed. A warning message will be shown.
Comment 7 Hartmut Kuhse 2013-06-22 20:35:36 UTC
Oh, I forgot: This patch works for the newest git version 2.8.
Comment 8 Michael Natterer 2014-03-06 23:55:49 UTC
*** Bug 724447 has been marked as a duplicate of this bug. ***
Comment 9 Jehan 2015-08-23 19:01:23 UTC
Hi,

I had a look. There are a few indentation and spaces issues here and there.

----

Also in gimp_plug_in_proc_frame_dispose():

> g_list_free_full (proc_frame->proc_frame_items,
>                        (GDestroyNotify) g_object_unref);

These are not objects so you get an assertion. You destroy with g_slice_free().

----

gimp_plug_in_proc_frame_have_item() is badly named in my opinion. It should be like gimp_plug_in_proc_frame_destroy_on_removed_item () or something (well this one is a little long. Feel free for another name. Just _have_item() seems not to say what it does at all).

----

Also still in gimp_plug_in_proc_frame_have_item(), I don't understand what you are doing in case of GIMP_EXTENSION. You are freeing the GimpPlugInProcFrameItem, but without removing and freeing the elements from the proc_frame_items list. I feel like it may end up badly.

Also what about this test:

>           if (proc_frame_item->item == item)
>            {
>            }

Why a test with empty code?

----

Actually even when testing a Script-fu script open, your code actually crash (segmentation fault) on a g_slice_free() in gimp_plug_in_proc_frame_have_item(). I have not pushed the diagnosis too far so I can't tell you exactly why. This may be related to the previous issue of freeing list data without freeing the list items themselves (maybe?).

----

In gimp_plug_in_proc_frame_item_removed(), you call gimp_plug_in_proc_frame_have_item() for all open items, where you close every open plugin inconditionnally.
So you end up closing some plugins by mistake.

Reproduction steps:
- open image A and open a plugin P1 on it.
- open image B and open a plugin P2 on it.
- close image A.

Result: both plugins P1 and P2 are ended!
Expected result: you wanted only the plugin P1 linked to A to close.

You want to make your test in the else part of gimp_plug_in_proc_frame_have_item():

>   else
>    {
>      for (list = main_proc_frame.proc_frame_items; list; list = g_list_next (list))
>        {
>          GimpPlugInProcFrameItem *proc_frame_item = list->data;
>
>          if (proc_frame_item->item == item)
>            {
>              gimp_plug_in_close (plug_in, TRUE);
>              break;
>            }
>        }
>    }

Other than these, I think that's a good start but there are definitely details to fix. :-)
Comment 10 Jehan 2015-08-23 19:07:08 UTC
Oh and this was a review of attachment 247529 [details] [review]. I have not reviewed the other approach (forbidding items to be removed when a plugin is reading it) because I feel the first approach is better. I feel that users may feel frustrated if they can't remove a layer or close an image for instance because a plugin is open. They would expect the plugin to just close, and that's it.

This said, I can also understand this other approach has some interest though. A plugin may be doing some work on an image with the goal of modifying it on exit, and maybe the user wants to be alerted if one forgot to commit the plugin work (in order not to lose it) before closing the image.
Maybe someone else would feel this second approach is better somehow?
Comment 11 Jehan 2015-08-24 11:14:32 UTC
By the way, Mitch, are you sure you want this code in GimpPlugInProcFrame code?
Reading at the code, the whole point is to eventually close a GimpPlugIn with gimp_plug_in_close(), not a GimpPlugInProcFrame. For me, the plugin should be the one to monitor all the items it has been called on.

Reading the attachment 247529 [details] [review], I felt porting this code to GimpPlugInProcFrame ended up in weird loops and inconsistencies.
Comment 12 Michael Schumacher 2017-02-19 18:02:35 UTC
So, what do we do about this?
Comment 13 GNOME Infrastructure Team 2018-05-24 13:43:14 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gimp/issues/474.