libgimp: don't keep per-procedure proxies of ID objects.

As discussed in !1725, there is an init order issue, which is that you
cannot obtain any of these ID objects as long as the GimpProcedure is
not created. In particular, now that GimpResource arguments can have
defaults, we will want to query such resource in the create_procedure()
implementation of a plug-in (where the GimpProcedure is not running yet,
as we are defining it!).

Anyway I don't really see the point of these multiple-level hash tables
all storing a reference to the same object. Let's just store in the
GimpPlugIn's hash tables once and give the same reference to any
procedure (anyway we make it clear that these objects belong to libgimp
and must not be freed, so it's a bug all the same if someone frees
them). Then it also fixes the init order issue.
This commit is contained in:
Jehan 2024-09-04 18:06:30 +02:00
parent 7fc7683cae
commit 4dcf141739
7 changed files with 8 additions and 243 deletions

View File

@ -26,7 +26,6 @@
#include "libgimpbase/gimpwire.h" /* FIXME kill this include */ #include "libgimpbase/gimpwire.h" /* FIXME kill this include */
#include "gimpplugin-private.h" #include "gimpplugin-private.h"
#include "gimpprocedure-private.h"
enum enum
@ -168,10 +167,9 @@ gimp_display_get_by_id (gint32 display_id)
{ {
if (display_id > 0) if (display_id > 0)
{ {
GimpPlugIn *plug_in = gimp_get_plug_in (); GimpPlugIn *plug_in = gimp_get_plug_in ();
GimpProcedure *procedure = _gimp_plug_in_get_procedure (plug_in);
return _gimp_procedure_get_display (procedure, display_id); return _gimp_plug_in_get_display (plug_in, display_id);
} }
return NULL; return NULL;

View File

@ -26,7 +26,6 @@
#include "gimppixbuf.h" #include "gimppixbuf.h"
#include "gimpplugin-private.h" #include "gimpplugin-private.h"
#include "gimpprocedure-private.h"
enum enum
@ -157,10 +156,9 @@ gimp_image_get_by_id (gint32 image_id)
{ {
if (image_id > 0) if (image_id > 0)
{ {
GimpPlugIn *plug_in = gimp_get_plug_in (); GimpPlugIn *plug_in = gimp_get_plug_in ();
GimpProcedure *procedure = _gimp_plug_in_get_procedure (plug_in);
return _gimp_procedure_get_image (procedure, image_id); return _gimp_plug_in_get_image (plug_in, image_id);
} }
return NULL; return NULL;

View File

@ -26,7 +26,6 @@
#include "libgimpbase/gimpwire.h" /* FIXME kill this include */ #include "libgimpbase/gimpwire.h" /* FIXME kill this include */
#include "gimpplugin-private.h" #include "gimpplugin-private.h"
#include "gimpprocedure-private.h"
enum enum
@ -180,10 +179,9 @@ gimp_item_get_by_id (gint32 item_id)
{ {
if (item_id > 0) if (item_id > 0)
{ {
GimpPlugIn *plug_in = gimp_get_plug_in (); GimpPlugIn *plug_in = gimp_get_plug_in ();
GimpProcedure *procedure = _gimp_plug_in_get_procedure (plug_in);
return _gimp_procedure_get_item (procedure, item_id); return _gimp_plug_in_get_item (plug_in, item_id);
} }
return NULL; return NULL;

View File

@ -35,7 +35,6 @@
#include "gimpgpparams.h" #include "gimpgpparams.h"
#include "gimpplugin-private.h" #include "gimpplugin-private.h"
#include "gimpplugin_pdb.h" #include "gimpplugin_pdb.h"
#include "gimpprocedure-private.h"
/** /**
@ -1653,12 +1652,6 @@ gimp_plug_in_pop_procedure (GimpPlugIn *plug_in,
priv->procedure_stack = g_list_remove (priv->procedure_stack, procedure); priv->procedure_stack = g_list_remove (priv->procedure_stack, procedure);
/* Make procedure object unref it's proxy references.
* Our hashes still have references to proxies.
* Calling procs may also have retained references to proxies.
*/
_gimp_procedure_destroy_proxies (procedure);
/* Don't destroy proxies now because any proc, especially temporary procs, /* Don't destroy proxies now because any proc, especially temporary procs,
* may have passed a reference to a proc higher in the stack e.g. the main procedure. * may have passed a reference to a proc higher in the stack e.g. the main procedure.
* We don't have separate proxy hashes for each pushed procedure, * We don't have separate proxy hashes for each pushed procedure,
@ -1961,10 +1954,6 @@ gimp_plug_in_destroy_proxies (GimpPlugIn *plug_in,
if (object->ref_count == 1) if (object->ref_count == 1)
{ {
/* this is the normal case for an unused proxy, since we already
* destroyed the only other reference in procedure with
* _gimp_procedure_destroy_proxies().
*/
g_hash_table_iter_remove (&iter); g_hash_table_iter_remove (&iter);
} }
else if (! G_IS_OBJECT (object)) else if (! G_IS_OBJECT (object))

View File

@ -1,46 +0,0 @@
/* GIMP - The GNU Image Manipulation Program
* Copyright (C) 1995 Spencer Kimball and Peter Mattis
*
* gimpprocedure-private.h
* Copyright (C) 2019 Michael Natterer <mitch@gimp.org>
*
* This library is free software: you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library. If not, see
* <https://www.gnu.org/licenses/>.
*/
#if !defined (__GIMP_H_INSIDE__) && !defined (GIMP_COMPILATION)
#error "Only <libgimp/gimp.h> can be included directly."
#endif
#ifndef __GIMP_PROCEDURE_PRIVATE_H__
#define __GIMP_PROCEDURE_PRIVATE_H__
G_BEGIN_DECLS
GimpDisplay * _gimp_procedure_get_display (GimpProcedure *procedure,
gint32 display_id);
GimpImage * _gimp_procedure_get_image (GimpProcedure *procedure,
gint32 image_id);
GimpItem * _gimp_procedure_get_item (GimpProcedure *procedure,
gint32 item_id);
GimpResource * _gimp_procedure_get_resource (GimpProcedure *procedure,
gint32 resource_id);
void _gimp_procedure_destroy_proxies (GimpProcedure *procedure);
G_END_DECLS
#endif /* __GIMP_PROCEDURE_H__ */

View File

@ -36,7 +36,6 @@
#include "gimppdb_pdb.h" #include "gimppdb_pdb.h"
#include "gimpplugin-private.h" #include "gimpplugin-private.h"
#include "gimpplugin_pdb.h" #include "gimpplugin_pdb.h"
#include "gimpprocedure-private.h"
#include "gimpprocedureconfig-private.h" #include "gimpprocedureconfig-private.h"
#include "libgimp-intl.h" #include "libgimp-intl.h"
@ -103,11 +102,6 @@ typedef struct _GimpProcedurePrivate
GDestroyNotify run_data_destroy; GDestroyNotify run_data_destroy;
gboolean installed; gboolean installed;
GHashTable *displays;
GHashTable *images;
GHashTable *items;
GHashTable *resources;
} GimpProcedurePrivate; } GimpProcedurePrivate;
@ -267,8 +261,6 @@ gimp_procedure_finalize (GObject *object)
g_clear_pointer (&priv->values, g_free); g_clear_pointer (&priv->values, g_free);
} }
_gimp_procedure_destroy_proxies (procedure);
G_OBJECT_CLASS (parent_class)->finalize (object); G_OBJECT_CLASS (parent_class)->finalize (object);
} }
@ -2483,165 +2475,3 @@ gimp_procedure_set_icon (GimpProcedure *procedure,
if (priv->installed) if (priv->installed)
gimp_procedure_install_icon (procedure); gimp_procedure_install_icon (procedure);
} }
/* internal functions */
GimpDisplay *
_gimp_procedure_get_display (GimpProcedure *procedure,
gint32 display_id)
{
GimpProcedurePrivate *priv;
GimpDisplay *display = NULL;
g_return_val_if_fail (GIMP_IS_PROCEDURE (procedure), NULL);
g_return_val_if_fail (gimp_display_id_is_valid (display_id), NULL);
priv = gimp_procedure_get_instance_private (procedure);
if (G_UNLIKELY (! priv->displays))
priv->displays =
g_hash_table_new_full (g_direct_hash,
g_direct_equal,
NULL,
(GDestroyNotify) g_object_unref);
display = g_hash_table_lookup (priv->displays,
GINT_TO_POINTER (display_id));
if (! display)
{
display = _gimp_plug_in_get_display (priv->plug_in,
display_id);
if (display)
g_hash_table_insert (priv->displays,
GINT_TO_POINTER (display_id),
g_object_ref (display));
}
return display;
}
GimpImage *
_gimp_procedure_get_image (GimpProcedure *procedure,
gint32 image_id)
{
GimpProcedurePrivate *priv;
GimpImage *image = NULL;
g_return_val_if_fail (GIMP_IS_PROCEDURE (procedure), NULL);
g_return_val_if_fail (gimp_image_id_is_valid (image_id), NULL);
priv = gimp_procedure_get_instance_private (procedure);
if (G_UNLIKELY (! priv->images))
priv->images =
g_hash_table_new_full (g_direct_hash,
g_direct_equal,
NULL,
(GDestroyNotify) g_object_unref);
image = g_hash_table_lookup (priv->images,
GINT_TO_POINTER (image_id));
if (! image)
{
image = _gimp_plug_in_get_image (priv->plug_in,
image_id);
if (image)
g_hash_table_insert (priv->images,
GINT_TO_POINTER (image_id),
g_object_ref (image));
}
return image;
}
GimpItem *
_gimp_procedure_get_item (GimpProcedure *procedure,
gint32 item_id)
{
GimpProcedurePrivate *priv;
GimpItem *item = NULL;
g_return_val_if_fail (GIMP_IS_PROCEDURE (procedure), NULL);
g_return_val_if_fail (gimp_item_id_is_valid (item_id), NULL);
priv = gimp_procedure_get_instance_private (procedure);
if (G_UNLIKELY (! priv->items))
priv->items =
g_hash_table_new_full (g_direct_hash,
g_direct_equal,
NULL,
(GDestroyNotify) g_object_unref);
item = g_hash_table_lookup (priv->items,
GINT_TO_POINTER (item_id));
if (! item)
{
item = _gimp_plug_in_get_item (priv->plug_in,
item_id);
if (item)
g_hash_table_insert (priv->items,
GINT_TO_POINTER (item_id),
g_object_ref (item));
}
return item;
}
GimpResource *
_gimp_procedure_get_resource (GimpProcedure *procedure,
gint32 resource_id)
{
GimpProcedurePrivate *priv;
GimpResource *resource = NULL;
g_return_val_if_fail (GIMP_IS_PROCEDURE (procedure), NULL);
g_return_val_if_fail (gimp_resource_id_is_valid (resource_id), NULL);
priv = gimp_procedure_get_instance_private (procedure);
if (G_UNLIKELY (! priv->resources))
priv->resources =
g_hash_table_new_full (g_direct_hash,
g_direct_equal,
NULL,
(GDestroyNotify) g_object_unref);
resource = g_hash_table_lookup (priv->resources,
GINT_TO_POINTER (resource_id));
if (! resource)
{
resource = _gimp_plug_in_get_resource (priv->plug_in,
resource_id);
if (resource)
g_hash_table_insert (priv->resources,
GINT_TO_POINTER (resource_id),
g_object_ref (resource));
}
return resource;
}
void
_gimp_procedure_destroy_proxies (GimpProcedure *procedure)
{
GimpProcedurePrivate *priv;
g_return_if_fail (GIMP_IS_PROCEDURE (procedure));
priv = gimp_procedure_get_instance_private (procedure);
g_clear_pointer (&priv->displays, g_hash_table_unref);
g_clear_pointer (&priv->images, g_hash_table_unref);
g_clear_pointer (&priv->items, g_hash_table_unref);
g_clear_pointer (&priv->resources, g_hash_table_unref);
}

View File

@ -23,7 +23,6 @@
#include "libgimpbase/gimpwire.h" /* FIXME kill this include */ #include "libgimpbase/gimpwire.h" /* FIXME kill this include */
#include "gimpplugin-private.h" #include "gimpplugin-private.h"
#include "gimpprocedure-private.h"
/* GimpResource: base class for resources. /* GimpResource: base class for resources.
@ -325,10 +324,9 @@ gimp_resource_get_by_id (gint32 resource_id)
{ {
if (resource_id > 0) if (resource_id > 0)
{ {
GimpPlugIn *plug_in = gimp_get_plug_in (); GimpPlugIn *plug_in = gimp_get_plug_in ();
GimpProcedure *procedure = _gimp_plug_in_get_procedure (plug_in);
return _gimp_procedure_get_resource (procedure, resource_id); return _gimp_plug_in_get_resource (plug_in, resource_id);
} }
return NULL; return NULL;