Technically not problematic but when inspecting using Plugin Browser,
the resulting hierarchy is confusing because it sometimes shows actions
to be inside "empty" directories if there are doubled path separators or
trailing ones. Let's not depend on the user to do this correctly when we
can do it for them.
Whatever a plug-in does, it should not be able to trigger WARNINGs or
CRITICALs on the core process. Here this was possible when requesting an
array of param specs with gimp_drawable_filter_operation_get_pspecs() on
a GEGL operation, while GIMP doesn't support all types for this filter's
arguments. Trying to send unsupported type specs through the wire would
fail.
Unfortunately just saying that we must add support for these types is
not enough because we simply cannot support every possible types. First
because even in current GEGL core filters, there are types we might
never support (e.g. audio fragments?). But even more because third-party
filters could have custom types too (just like our own filters have
custom GIMP types).
So instead, acknowledge that some types cannot be sent through the wire,
verify when it's one such argument and simply output an informational
message on stderr (because the info of a non-supported type is still
interesting in case this is something we should in fact add support for;
it's much better than silently ignoring the argument).
… gimp_procedure_is_internal().
I realized we were already naming these "internal procedures" in the
Procedure Browser and this is in fact a better naming than "core
procedure".
Note that I don't touch the GimpFileProcedure and children because they
are special-cased anyway. As for GimpBatchProcedure, ALWAYS is a good
default too.
After discussing with NikcDC and Wormnest on IRC, we agreed that maybe a
default of being sensitive when one or more drawables are selected seems
better, since the run() signature now has an array of drawables. So most
plug-in developers will consider the case when there are several
selected drawables as an obvious possible case.
And if someone really doesn't want to handle this case, they can always
set the sensitivity mask explicitly.
Core procedures are all the procedures created for libgimp basically. In
opposition, procedures created by plug-ins are not core procedures.
GimpProcedure class in libgimp now has a gimp_procedure_is_core() which
will tell you if a procedure is core or not.
Private procedures already existed, except that they were only marked as
"private" in libgimp (e.g. _gimp_font_get_lookup_name()) starting with
an underscore and marked as G_GNUC_INTERNAL. Now we also store this
information in the procedure object itself for reuse.
… to GIMP_PDB_PROC_TYPE_PERSISTENT, let's rename some procedures.
s/gimp_plug_in_extension_enable/gimp_plug_in_persistent_enable/
s/gimp_plug_in_extension_process/gimp_plug_in_persistent_process/
s/gimp_procedure_extension_ready/gimp_procedure_persistent_ready/
Even though it's not public yet (and won't really be for GIMP 3.0), I
created a new concept of "GIMP Extension" (.gex files) which bundles
various types of data for GIMP, such as plug-ins but also brushes and
other resources, themes, icons, etc.
Having 2 different concepts named the same is confusing, especially
since one of them is not really self-explaining IMO (why are "always-ON"
plug-ins called "extensions"?). So even though this is the older
concept, and since we are anyway massively breaking the API for GIMP
3.0, let's rename this older concept. "Persistent Plug-Ins" is much more
self-defining.
Preferences > Image Import and Export tab has various Export Policies,
which are mostly for safety reasons. One may want to default at never
exporting specific metadata. This got broken and anyway the logic was
not good enough.
Now these policies are followed in interactive mode, unless an export
has already happened for this specific export plug-in on this specific
file (in this case, we reuse the last values). We don't want settings to
unexpectedly change in such a case.
In last-vals and non-interactive run-mode though, we don't follow the
Preferences policies, since in the former case, we want to reuse exactly
the same settings (e.g. we don't want an Export discarding metadata
which explicitly checked in the Export As) and in the latter case, it is
the developer's responsibility to set up expected options.
This abstract spec type is basically a GParamSpecObject with a default
value. It will be used by various object spec with default values, such
as GimpParamSpecColor, GimpParamSpecUnit and all GimpParamSpecResource
subtypes. Also it has a duplicate() class method so that every spec type
can implement the proper way to duplicate itself.
This fixes the fact that in gimp_config_param_spec_duplicate(), all
unknown object spec types (because they are implemented in libgimp,
which is invisible to libgimpconfig) are just copied as
GParamSpecObject, hence losing default values and other parameters.
As a second enhancement, it also makes it easier to detect the object
spec types for which we have default value support in
gimp_config_reset_properties().
As a side fix, gimp_param_spec_color() now just always duplicates the
passed default color, making it hence much easier to avoid bugs when
reusing a GeglColor.
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.
Though I am not fond of these macros for our core code (it makes the
code more cumbersome especially for using private data in derivable
types), this definitely makes sense for public API, since it would allow
easier modifications with less chances of messing API/ABI stability.
Same for gimp_procedure_add_aux_argument() and gimp_procedure_add_return_value().
We now have specific public functions for every supported type and it's
in fact much better to use them. The generic functions gave the feeling
that we could use any GParamSpec as procedure argument, whereas we in
fact depend on what the PDB support, and only these subtypes.
These were created because of some limitation/bug in pygobject, which is
now much better worked around by having specific functions for every
argument type supported by the PDB.
Previous gimp_procedure_run_config() was in fact only good for private usage
inside the various run() methods for the different GimpProcedure subtypes. The
problem with this implementation is that the returned config object is not
complete. For instance, for a GimpLoadProcedure, the "run-mode" and "file"
properties are not part of the config object so you cannot call a
GimpLoadProcedure with any of the gimp_procedure_run*() functions.
Note: we had some working usage, e.g. in file-openraster, but only because it
was running the load procedure as a GimpPDBProcedure whose returned config
object was indeed always complete!
As a consequence, I rename gimp_procedure_run_config() as an internal-only
function _gimp_procedure_create_run_config() and I create a new
gimp_procedure_run_config() which always return a full config with all
arguments.
- Fix a few broken references and an inconsistent argument name.
- Add the new headers in the introspectable header list.
- Add a few missing class descriptions for GimpProcedure and subclasses.
gimp_procedure_new_return_values() takes ownership of the passed GError (it
allows, among other things, to call it directly as return value). So we must not
try and free it afterwards.
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.
This partially revert some of the changes in commit 652a1b4388 because the
Windows CI suddenly failed because of this (my local build on Linux didn't have
any problem though) with:
> /usr/bin/x86_64-w64-mingw32-ld: libgimp/libgimpui-3.0-0.dll.p/gimpproceduredialog.c.obj: in function `gimp_procedure_dialog_save_defaults':
> /builds/GNOME/gimp/_build/../libgimp/gimpproceduredialog.c:2570:(.text+0x633): undefined reference to `_gimp_procedure_config_save_default'
> /usr/bin/x86_64-w64-mingw32-ld: /builds/GNOME/gimp/_build/../libgimp/gimpproceduredialog.c:2576:(.text+0x644): undefined reference to `_gimp_procedure_config_has_default'
> /usr/bin/x86_64-w64-mingw32-ld: libgimp/libgimpui-3.0-0.dll.p/gimpproceduredialog.c.obj: in function `gimp_procedure_dialog_load_defaults':
> /builds/GNOME/gimp/_build/../libgimp/gimpproceduredialog.c:2549:(.text+0xa2f): undefined reference to `_gimp_procedure_config_load_default'
> /usr/bin/x86_64-w64-mingw32-ld: libgimp/libgimpui-3.0-0.dll.p/gimpproceduredialog.c.obj: in function `gimp_procedure_dialog_constructed':
> /builds/GNOME/gimp/_build/../libgimp/gimpproceduredialog.c:368:(.text+0x11b1): undefined reference to `_gimp_procedure_config_has_default'
This is because these functions are used not only inside libgimp but also
across inside libgimpui. As a consequence, the build fails when linking
libgimpui.
Some of these should not even be visible by libgimp and were just fine as static
as well! For the rest, I make them really private (not only with a private
header).
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.
This will be useful for plug-in developers but also for us. Seeing we leak the
config object is often a good indication that something is wrong in our handling
of internal references (since everything relies on the config object in plug-ins
now, in particular all the GUI).
Otherwise we will always try to reuse previous values or use the default,
bypassing the actual passed values.
I encountered this issue while porting file-glob and realizing that the
"pattern" argument was always passed to NULL, ignoring the explicitly set
pattern.
When a procedure has no run-mode argument, we should simply not assume anything
and use the passed arguments (which is what the non-interactive mode does).
This new function is meant to replace gimp_procedure_new() when all plug-in
usage will have been switched.
This function creates the GimpProcedureConfig object on behalf of the plug-in
and calls gimp_procedure_config_begin_run() and gimp_procedure_config_end_run().
This way we ensure that all plug-in calls with successful result are properly
stored without asking the developer not to forget to call these (if a "good
practice" is in fact something we request to do every time, especially for good
user experience, we might as well make it rather a core process).
Advantages:
* Better interactive experience: using any plug-in will result in saved
previously used settings.
* for developers, working on config objects is also much more comfortable than
working on GValueArray;
* step forward for the future macro infrastructure: if we can ensure that all
plug-in calls are properly logged, then we can replay plug-in actions, in
NON_INTERACTIVE with the same settings.
Looking further, the @help is only used in gimp_proc_view_new() so far (for the
Procedure Browser) where the blurb and argument descriptions are already
localized. It makes no sense to only keep this in English. So let's ask to have
both arguments translated.
Now clearly we should not ask for @help to be mandatory. Very often, it makes no
sense to have a longer help string (the small blurb and the few arguments may be
very self-explanatory). So I make this argument nullable.
There is only the @help_id which I wonder if we could not have a simpler
function gimp_procedure_set_documentation_uri(). Indeed while having a unified
infrastructure with a XML summary and help IDs and whatelse makes sense for GIMP
as a whole, I think that many third-party plug-ins would work much better with a
very simple direct URL. Or it could even be a GFile to a local file (for
plug-ins which want to embed their documentation in the plug-in folder for
instance). To be continued…
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
GLib has a specific type for byte arrays: `GBytes` (and it's underlying
GType `G_TYPE_BYTES`).
By using this type, we can avoid having a `GimpUint8Array` which is a
bit cumbersome to use for both the C API, as well as bindings. By using
`GBytes`, we allow other languages to pass on byte arrays as they are
used to, while the bindings will make sure to do the right thing.
In the end, it makes the API a little bit simpler for everyone, and
reduces confusion for people who are used to working with byte arrays
in other C/GLib based code (and not having 2 different types to denote
the same thing).
Related: https://gitlab.gnome.org/GNOME/gimp/-/issues/5919
This was the last remaining bit in #8124. Basically I needed to check how
localization of menu paths worked. I was thinking of maybe have 2 arguments to
gimp_procedure_add_menu_path(), one non-localized (for default menu paths) and
one localized by the plug-in (for custom menus). That would break all plug-ins,
but also looking at our code, it's complicated to do right.
Instead let's just keep current API and add an example in function docs. We'll
see how we can improve the API if the very hypothetical problem I am foreseeing
actually happens some day: say a word in English translates to e.g. "Filters" in
some other language, whereas English "Filters" translates to yet another term;
in such case, this new menu would still merge with the default /Filters/ menu
when localized in this language, so we'd have the weird situation where the
custom menu label would have passed through 2 translations somehow.
But let's see how it goes. If we really need, in the future, we can deprecate
gimp_procedure_add_menu_path() and add a gimp_procedure_add_menu_paths() with a
base_path and a custom_path, while the custom_path would be expected to be
already translated.
GLib has a specific type of NULL-terminated string arrays:
`G_TYPE_STRV`, which is the `GType` of `char**` aka `GStrv`.
By using this type, we can avoid having a `GimpStringArray` which is a
bit cumbersome to use for both the C API, as well as bindings. By using
`GStrv`, we allow other languages to pass on string lists as they are
used to, while the bindings will make sure to do the right thing.
In the end, it makes the API a little bit simpler for everyone, and
reduces confusion for people who are used to working with string arrays
in other C/GLib based code (and not having 2 different types to denote
the same thing).
Related: https://gitlab.gnome.org/GNOME/gimp/-/issues/5919
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
Fixes the patch from !470 which is mostly right, except that
g_param_spec_sink() may possibly lead to finalizing the GParamSpec
(typically if it was a just-created floating spec). We don't want to
return pointer to freed data. Let's return NULL instead.
Also looking closer at the memory handling here, it looks the right
annotation for @pspec is (transfer floating). Basically we are sinking a
floating object into a full object and taking ownership of this sunk
object. But if the object was already sunk, we are reffing it and
keeping this additional reference, not the passed argument's. Hopefully
it's right since the annotation and handling of floating object with
GObject Introspection seems very unclear to me (even in core GObject
code, I see what looks like contradictory annotations).
In the normal flow, pspec is persisted in the arguments array, and is
g_param_spec_ref_sink()'d in order to sink a possible floating ref. To
avoid a leak in the error case, we need to add some g_param_spec_sink().
