GimpPlugin improperly destroys proxies after each run of a temporary procedure.
A temporary procedure may pass reference to proxy to main procedure.
Proxies should live as long as the main procedure of a plugin,
or for an extension plugin, until only the extension main procedure
is on the procedure stack. More discussion in the issue.
Extract method gimp_plug_in_proc_run.
Call it from two new methods: main_proc_run and temp_proc_run,
which do more e.g. cleanup.
Extract methods for cleanup, main_proc_cleanup and temp_proc_cleanup
Add method is_proc_stack_empty
This commit renames the GimpVectors
object to GimpPath in both app/core and
in libgimp. It also renames the files
to gimppath.[ch] and updates the relevant
build and translation files.
There are still outstanding gimp_vectors_* ()
functions on the app side that need to be renamed
in a subsequent commit.
...to paths
Follow-up to d0bdbdfd. Changes all
gimp_vectors_* () PDB to gimp_path_* ()
and renames relevant PDB files from
vectors to path.
The next step will be to rename
GimpVectors in libgimp to GimpPath,
removing the last (public) trace of it.
Also:
- renaming gimp_layer_group_new() to gimp_group_layer_new() in order to keep the
same name as in core code (i.e. GimpGroupLayer, not GimpLayerGroup).
- renaming gimp_image_merge_layer_group() to gimp_group_layer_merge()
- new functions: gimp_procedure_add_group_layer_argument(),
gimp_procedure_add_group_layer_aux_argument() and
gimp_procedure_add_group_layer_return_value().
This can be tested, e.g. in Python with these calls:
```py
i = Gimp.get_images()[0]
g = Gimp.GroupLayer.new(i, "hello")
i.insert_layer(g, None, 1)
g2 = Gimp.GroupLayer.new(i, "world")
i.insert_layer(g2, g, 1)
g.merge()
```
This was work started long ago, stored in an old stash which I finally
finish now! :-)
The gimp_procedure_run() already existed, though it was with an ordered
GimpValueArray array of arguments. Its usage feels redundant to the series of
gimp_pdb_run_procedure*() functions (which is confusing), but
gimp_procedure_run() was actually a bit more generic, because it does not
necessarily calls GimpProcedure-s through the PDB! For instance, it can runs a
local GimpProcedure, such as the case of one procedure which would want to call
another procedure in the same plug-in, but without having to go through PDB. Of
course, for local code, you may as well run relevant functions directly, yet it
makes sense that if one of the redundant-looking function is removed, it should
be the more specific one. Also gimp_procedure_run() feels a lot simpler and
logical, API wise.
A main difference in usage is that now, plug-in developers have to first
explicitly look up the GimpPdbProcedure with gimp_pdb_lookup_procedure() when
they wish to call PDB procedures on the wire. This was done anyway in the
gimp_pdb_run_procedure*() code, now it's explicit (rather than calling by name
directly).
Concretely:
* gimp_pdb_run_procedure(), gimp_pdb_run_procedure_config() and
gimp_pdb_run_procedure_valist() are removed.
* gimp_procedure_run() API is modified to use a variable args list instead of a
GimpValueArray.
* gimp_procedure_run_config() and gimp_procedure_run_valist() are added.
* gimp_procedure_run_config() in particular will be the one used in bindings
which don't have variable args support through a (rename-to
gimp_procedure_run) annotation.
We cannot be 100% sure generically (i.e. for all possible bindings available
with GObject Introspection) if bindings add their own reference to objects or
not. Clearly we have cases when they always do (Lua, Javascript), cases when
they do only in certain conditions (global Python variables) and cases when they
don't (Vala). What we know for sure is that in these script languages,
developers don't manually manage memory anyway. So the additional reference is
not their fact.
So let's just maintain a list of automatic memory managed binding languages,
among the few we officially support (i.e. the ones for which we have working
test plug-ins) and verify by executable extension if the plug-in is written in
one of these.
Both keeping a manually-updated list and verifying by extension are not so
pretty solution, but for now it will do.
As explained in the comment above, the reference might actually be owned by the
binding code (not by the plug-in code) and therefore can still be released
afterwards. Freeing it now while we don't own the reference exposes us to
double-free crashes.
The expectation of 2 references per object in gimp_plug_in_destroy_proxies() was
wrong. It is true during most of the plug-in life, because both the
GimpProcedure and the GimpPlugIn have a hash-table keeping their own reference
to it, except that in gimp_plug_in_pop_procedure(), we release the reference
owned by the procedure with _gimp_procedure_destroy_proxies() first. So at this
point of the object life, its reference count is supposed to be 1.
The source of the bug was in fact in _gimp_plug_in_get_*() (where * can be
display, image, item or resource) which was behaving differently the first time
it is called for an object with the successive calls. In the first call only, it
was creating then refing into the table (so the object started directly with 2
references) whereas on successive calls, it just returned the hashtable-looked
up reference. In other words, it behaved as a (transfer full) on the first call
and (transfer none) on successive calls. And so did all public API which were
making use of this infrastructure (in particular gimp_*_get_by_id() functions).
Much like for images and items. Change the PDB to transmit IDs
instead of names for brush, pattern etc. and refactor a whole
lot of libgimp code to deal with it.
modified: libgimp/gimpplugin-private.h
Nothing was really clearly specified until now, which was kinda equivalent to
the string being in the OS encoding as used by GLib. Since this string will
usually be statically hardcoded in code (and not extracted from system), it's
just much easier to request UTF-8 for this specific case.
Now text layers are proper types, which means that the binding API will also be
nicer (e.g. `txt_layer.set_text('hello world')` in Python).
This commit also adds the param specs allowing to create plug-in procedures with
text layer parameters.
Finally it fixes the few calls in file-pdf-save (apparently the only plug-in
using specific text layer API right now) with explicit type conversion.
Plug-in localization was always partially plug-in side, especially for
things like custom GUI. But labels or blurb in GIMP (such as in menus or
action search) were localizing GIMP side.
It had many drawbacks:
- To get menu localization, a plug-in had to set up gettext, even though
they might want to use something else for their GUI (after all, giving
facilities for gettext is a good idea, but there is no reason to force
using this system).
- There was a complex internal system passing the localization domain
name, as well as the catalog file system path to core, then through
various classes which we can now get rid of.
- There could be domain name clashes, if 2 plug-ins were to use the same
i18n domain name. This was handled in now removed functions
gimp_plug_in_manager_get_locale_domains() by simply keeping a unique
one (and gimp_plug_in_manager_bind_text_domains() would just bind the
domain to the kept directory). In other words, one of the duplicate
plug-ins would use the wrong catalog. We could try to make the whole
thing more complicated or try to forbid plug-ins to use any random
name (in particular made easier with the new extension wrapper). But
anyway this whole issue doesn't happen anymore if localization is
fully made plug-in side, so why bother?
I tried to evaluate the advantages of the core-side localization of
plug-in labels/blurbs and could only find one theoretical: if we wanted
to keep access to the original English text. This could be useful
(theoretically) if we wanted to search (e.g. in the action search) in
both localized and English text; or if we wanted to be able to swap
easily en/l10n text in a UI without reload. But even if we were to ever
do this, it would only be possible for plug-ins (GEGL operations in
particular are localized GEGL-side), so it lacks consistency. And it's
unsure why special-casing English should really make sense for other
language natives who want text in their lang, and search in their lang.
They don't necessarily care about original.
So in the end, I decided to simplify the whole thing, make localization
of plug-ins a plug-in side thing. Core will only receive translated text
and that's it. It cuts a lot of code out of the core, simplify runtime
processing and make plug-in creation simpler to understand.
The only think I still want to look at is how exactly menu paths are
translated right now. Note that it still works, but it's possible that
some things may be worth improving/simplifying on this side too.
I hesitated a lot whether we should just drop the whole localization of
plug-ins' label and description (blurb) within the core. Actually the
commit messages I wrote a few days ago were moving towards this logic.
It really looks to me like plug-in localization can happen fully within
plug-in themselves. As far as I can see, the only advantage which the
current logic has theoretically is that if we needed, we have access to
both the original strings and their translations (e.g. it could be
useful for text search). Nevertheless I am not sure if we will ever make
use of this, and this is limited cases as all filters turned GEGL ops
don't have such ability anyway.
Nevertheless since previous contributors clearly put quite a lot of work
on this code of localizing the plug-in's label and description within
the main binary, I want to give myself a little more time to think and
study the whole thing because doing anything rash.
In the meantime, what changes is that by default now, a plug-in without
a local gettext catalog is simply not localized. In particular, the core
process doesn't try to localize it using the default catalog, a.k.a.
GETTEXT_PACKAGE"-std-plug-ins" ("gimp30-std-plug-ins"). It just doesn't
make sense and the worst which could happen would be to get unexpected
and wrong translations.
Now by default, plug-ins will try to find a catalog in their main
folder, named as this folder. If it fails to find it, a message is
printed to stderr and localization is disabled (rather than falling back
to a default catalog). It is up to plug-in developers to either install
a catalog, or implement set_i18n() to give the right catalog, folder, or
disable localization with gettext, as handled by libgimp.
Clearly the old logic for localizing plug-ins was "core plug-ins first":
* In particular, we were defaulting to the "gimp*-std-plugins" domain,
asking plug-ins to override it with libgimp function
gimp_plug_in_set_translation_domain(). Obviously any third-party
plug-in would have to call this function since their strings are most
likely not the same as GIMP core plug-ins.
* Moreover this was only for the localization of menu items, which means
we had to duplicate work anyway (simplified for core C plug-ins only
with the INIT_I18N macro).
* Also plug-ins had to manage the location of the Gettext catalogs,
which is simpler for core plug-ins with gimp_locale_directory(), but
more annoying for third-party plug-ins which can be installed
basically anywhere (assuming their message catalogs are in their
directory, not centrally installed on the system, especially for
non-UNIX-like packages, with relocatable GIMP, no central package
system and so on).
* Finally in this logic of centrally installed catalogs, we were
requesting that the domain name is unique, which makes sense in a
Linux world with well maintained packages to avoid name clashes, not
in a world with people making plug-ins without knowing too much what's
done by their neighbour.
So now the new logic is:
- By default, GIMP will search for a folder called locale/ on the same
level as the plug-in executable. The Gettext domain will be the name
of the executable folder (and it doesn't matter too much if it's not
unique in such configuration).
- This can be disabled by overriding set_i18n() to NULL or
reimplementing it. All our core plug-ins will do this since they will
continue to use the centrally installed "gimp*-std-plugins" domain (or
"gimp*-python" for Python plug-ins).
- When not disabled while the folder is not available, warning messages
will be outputted to stderr, so that plug-in developers can easily
detect what is missing and how to handle it (and how to easily support
internationalization of their plug-in).
- gimp_plug_in_set_translation_domain() is removed and a future commit
is going to get rid of _gimp_plug_in_domain_register() because we are
going to get rid of the core-side localization of menu items (with
logic explained in the upcoming commit).
Fixing these 2 warnings in the CI which end up fatal:
WARNING: Invalid fragment for 'Gimp.Parasite': it should be struct
Serializes the object properties of @config to a [class@Parasite].
^~~~~~~~~~~~~~~~
WARNING: Invalid fragment for 'GLib.MainLoop': it should be struct
it has a GUI and is hanging around in a [class@GLib.MainLoop], it must call
^~~~~~~~~~~~~~~~~~~~~
gtk-doc has been slowly dying for the past few years; with gi-docgen we
have a nice successor.
This also makes sure the C documentation also uses the GIR file, which
in turn means faster build times (since all the C code doesn't have to
be parsed and recompiled again), and has a clear dependency graph.
See the [gi-docgen tutorial] for more info on how the system works.
[gi-docgen tutorial]: https://gnome.pages.gitlab.gnome.org/gi-docgen/tutorial.html
We heavily rely on GError in libgimp to retrieve plug-in error messages.
In a lot of our code, we just use domain=0 for g_set_error*() functions
and alike, but this is actually forbidden and results in GLib warnings.
Some plug-ins instead create their own domain, other use G_FILE_ERROR
nearly everywhere, even in some cases where the choice is really
questionable. Since anyway this is mostly useful for passing the error
message through, it is much nicer to provide a generic domain
GIMP_PLUG_IN_ERROR, which can be used by all plug-ins when they don't
want to bother with the error domain.
Don't drop references we do not own. Turns out bindings can have
things referenced even after all procedure code has returned. Keep the
old code there in #if 0 and keep the debug warning for now, maybe we
can do something generic about this.
Mention and guarantee the order of procedure registration (see
previous thumbnail loader commit). Remove duplicate docs from the
GimpPlugIn class comments that are needed for introspection, it's too
cumbersome to keep two identical texts in sync, and the removed text
is used nowhere.
Break reference cycles between the objects and the procedures they
keep by moving procedure destruction to dispose() and calling
g_object_run_dispose() before unrefing PLUG_IN and PDB in gimp.c.
Also some formatting and "Since: 3.0" annotation .
which frees exactly what _gimp_value_array_to_gp_params() has
allocated, honors its "full_copy" parameter, and plugs the last
libgimp refactoring leaks I'm currently aware of.
because we were simply destroying the proxy hashes when the last
procedure is done. Now we run gimp_plug_in_destroy_proxies() with the
right flag from gimp_plug_in_pop_procedure() before destroying the
hashes and get the proper debug warnings.
Need to push the current procedure around the entire body of
gimp_plug_in_proc_run_internal() because
_gimp_gp_params_to_value_array() needs access to proxy objects now.
Turn all ID param specs into object param specs (e.g. GimpParamImageID
becomes GimpParamImage) and convert between IDs and objects in
gimpgpparams.c directly above the the wire protocol, so all of app/,
libgimp/ and plug-ins/ can deal directly with objects down to the
lowest level and not care about IDs.
Use the actual object param specs for procedure arguments and return
values again instead of a plain g_param_spec_object() and bring back
the none_ok parameter.
This implies changing the PDB type checking functions to work on pure
integers instead of IDs (one can't check whether object creation is
possible if performing that check requires the object to already
exist).
For example gimp_foo_is_valid() becomes gimp_foo_id_is_valid() and is
not involved in automatic object creation magic at the protocol
level. Added wrappers which still say gimp_foo_is_valid() and take the
respective objects.
Adapted all code, and it all becomes nicer and less convoluted, even
the generated PDB wrappers in app/ and libgimp/.
Add an additional ref count to all proxies handed out by
GimpPlugIn, and when destroying the proxies, warn about any
proxy that has been unrefed, or additionally refed.
This catches and warns about only one broken unref, any more will
simply crash like before.
Turn GimpPlugIn into the main factory for all proxies and keep the
main hash tables there. The hash tables keep the initial reference.
For each GimpProcedure::run(), have s "sub-factory" which hands out
proxies to the actual procedure code. Each run() has hash tables of
its own which hold additional references. When run() is done, get rid
of its hash tables and their references, *and* drop the main plug-in
reference counts from the global hashes if the proxies' refcount has
dropped to one.
which are GimpProcedure subclasses with API to register as load/save
handlers and their own kind of run functions that get their standard
arguments passed directly instead of packed into a GimpValueArray.
They also register their standard arguments themselves, which removes
quite some boilerplate from load/save plug-ins.
Remove gimpprocedure-private.[ch] because install() and uninstall()
are now virtual functions of GimpProcedure.
They only contain private functions and don't need to be installed or
included by gimp_pdb_headers.h.
The PDB generation part is done by adding a "lib_private" variable
that can be set on PDB groups which should not be public API; the rest
is manual Makefile fiddling.
gpointer is not introspectable, and we should always prefer boxed types
over generic pointer types (thanks nielsdg!).
This also fixes gjs (JavaScript) binding of our API which was dropping
our properties even though they didn't need to be handled in JS.
See https://gitlab.gnome.org/GNOME/gjs/issues/266
bootchk