These are not usable by plug-ins anymore which should store their data between
runs as arguments or aux arguments (in case of values which should be stored
from one run to another but are not really usable for non-interactive scripts).
These are per-plug-in (not polluting the whole process space with just random
strings as identifiers which could be used by other plug-ins) and even survive
restarts of GIMP.
I still keep these functions, but only internally, as they are used to store
settings of GimpAspectPreview, GimpDrawablePreview and GimpZoomPreview across
plug-in runs. Still I changed their API to set and return a GBytes directly
(mimicking the private PDB functions' API).
Also I remove gimp_pdb_get_data_size() which is useless when exchanging GBytes
directly.
Note that the 2 functions are still exported in the library, and only not
advertized through headers (so they are not really internal, just hidden), on
purpose, because we need to call them in libgimpui. So it is still relatively
easy for a plug-in to use them. Nevertheless I made clear in the function
documentation that these must not be considered public and could end up deleted
at any time. Any plug-in still trying to call these takes the risk of having
their code relying on unreliable API.
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.
Passing (name, type, value) triplets is actually useless because we can get the
type information from the procedure/config anyway. That only adds one more
verification to do. Let's just change the function so that we pass (name, value)
couples instead, pretty much like in `g_object_set()`.
As far as plug-in API is concerned, at least the calling API, order of arguments
when calling PDB procedures doesn't matter anymore.
Order still matters for creating procedures with standard arguments (for
instance, "run-mode" is first, then image, or file, drawables or whatnot,
depending on the subtype of procedure), but not for calling with libgimp.
Concretely in this commit:
- gimp_pdb_run_procedure_argv() was removed as it's intrinsically order-based.
- gimp_pdb_run_procedure() and gimp_pdb_run_procedure_valist() stay but their
semantic changes. Instead of an ordered list of (type, value) couple, it's now
an unordered list of (name, type, value) triplets. This way, you can also
ignore as many args as you want if you intend to keep them default. For
instance, say you have a procedure with 20 args and you only want to change
the last one and keep the 19 first with default values: while you used to have
to write down all 20 args annoyingly, now you can just list the only arg you
care about.
There are 2 important consequences here:
1. Calling PDB procedures becomes much more semantic, which means scripts with
PDB calls are simpler (smaller list of arguments) and easier to read (when
you had 5 int arguments in a row, you couldn't know what they refer to,
except by always checking the PDB source; now you'll have associated names,
such as "width", "height" and so on) hence maintain.
2. We will have the ability to add arguments and even order the new arguments in
middle of existing arguments without breaking compatibility. The only thing
which will matter will be that default values of new arguments will have to
behave like when the arg didn't exist. This way, existing scripts will not be
broken. This will avoid us having to always create variants of PDB procedure
(like original "file-bla-save", then variant "file-bla-save-2" and so on)
each time we add arguments.
Note: gimp_pdb_run_procedure_array() was not removed yet because it's currently
used by the PDB. To be followed.
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.
This goes with our planned change of not making GimpProcedure arguments order
relevant anymore regarding the PDB API. In particular, it means we don't want to
use GimpValueArray for various procedure arguments API, but directly
GimpProcedureConfig objects.
This change will allow to add or reorder arguments in the future, so that we
won't have to create new PDB procedures when adding new arguments, while still
keeping PDB API stability.
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.
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.
Until now, it was not really possible to delete a colormap color, but since we
now use GimpPalette, people would definitely try to do so. It just makes sense
to allow doing this, but only if the color is unused.
Additionally when we do this, all the pixels refering to bigger indexes will be
edited so that they continue to refer to the same color (bigger indexes are
shifted by -1). Therefore removing an unused color does not change the image
render.
I wondered if we might want more options, e.g. the ability to delete a color
without fixing indexes (i.e. that colors over the deleted color index would
shift to the next color). This would even allow to delete used colors (though
now the last index would have to be unused one, unless we cycle colors).
Yet I don't think this should belong to this basic API. The most expected
behavior when deleting a color from an image colormap is to fix all indexes
stored in pixels so that the image still shows the same. So that's what this
function will do in this generic usage.
This is meant to replace gimp_image_get_colormap() (see also #9477).
We likely won't need a gimp_image_set_palette() because we can simply edit the
image's colormap/palette with GimpPalette API now and it is directly updated.
For instance, the following code changes the first entry in the image palette to
red, immediately:
```python
i = Gimp.list_images()[0]
p = i.get_palette()
c = Gimp.RGB()
c.r = 1.0
p.entry_set_color(0, c)
```
For this to work fine, I added a new concept to GimpData, which is that they can
be tied to a GimpImage (instead of a GFile). Image palettes are not considered
internals, they are just tied to their image, therefore they can be edited by
scripts/plug-ins.
Additionally with this commit, editing an image's colormap from libgimp API also
generates undo steps now.
bootchk had the case in commit 6781a35668. I again had it with gfig. I think it
just makes sense to init GEGL, especially as the errors are not that explicit
and that the plug-in code may not even call GEGL code directly (so it makes it
harder to guess).
It returns all the fonts (possibly more than 1) with a given name. I left the
function gimp_font_get_by_name() as a utility when one don't want to choose (or
is not able anyway, e.g. a script with minimal information), though I wondered
if we should not simplify with a single function (the new one, which is the
correct one now that it is possible to have several fonts with a given name).
It is easy to test with fonts named the same. For instance I could find 2
different fonts, both named 'Holiday'. This call in the Python console returns
both:
> Gimp.fonts_get_by_name('Holiday')
As part of this commit, I also implemented resource arrays (or subtype arrays)
as PDB arguments and return types.
… description.
- The returned value as width/height/etc. of the glyph extents (or bounding
box), not "of the font" (which doesn't mean much).
- Adding some definition for ascent and descent. This text is straight out
copied from Pango documentation comments in pango/pango-types.h.
- I don't see why we were negating the descent value. Let's keep the value sign
as defined in Pango.
- Fonctions were renamed: s/gimp_text_fontname/gimp_text_font/ and
s/gimp_text_get_extents_fontname/gimp_text_get_extents_font/
- The size_type arguments were removed. Even in 2.10, this argument was marked
as "dead" and ignored. It was only kept for API compatibility.
- The font name (string) was replaced by a GimpFont argument.
gimp_text_font() is easily tested in the Python console with:
> Gimp.text_font(Gimp.list_images()[0], None, 10, 40, "Hello World!", 1.0, True, 100, Gimp.context_get_font())
And gimp_text_get_extents_font() with:
> Gimp.text_get_extents_font("Hello World!", 100, Gimp.context_get_font())
Fixing:
> [809/2421] Generating libgimp/GimpUi-3.0.gir with a custom command (wrapped by meson to set env)
> libgimpwidgets/gimppropwidgets.c:37: Warning: GimpUi: multiple comment blocks documenting 'SECTION:gimppropwidgets:' identifier (already seen at gimppropwidgets.c:23).
This function is not perfect and in particular doesn't seem usable with binding
because of GimpUnit being some weird mix between an enum and some kind of class.
So this will have to be fixed too. See #8900.
… function gimp_font_get_pango_font_description().
Also updating file-pdf-save which is the only plug-in using these right now.
Note that I am not fully happy with the new function
gimp_font_get_pango_font_description() because I experienced some weird behavior
in file-pdf-save which is that some fonts were wrong if this is called after
pango_cairo_font_map_set_resolution().
But let's say this is a first step looking for improvements.
I am using the same GimpDrawableChooser with an additional drawable_type
argument to only show the appropriate tab if we want to limit what can be
chosen.
None of our plug-ins actually use a GimpLayer or GimpChannel only arg so far,
but if we have some day, or if some third-party plug-ins want to have such arg,
now they quite easily can!
After testing, setting a window as transient to another from another process is
still broken on Windows and it's hard to diagnose without using Windows
directly. Since it's not just broken, but it even hangs the whole process, which
is quite a blocker issue, let's disable again the whole code on Windows.
This was not working properly and needed some external build script as well as
the stamp/bogus header trick like for other similar in-source generated code.
In the same time, I get rid of old meson code which was meant for when using
meson < 0.57.0 (since our requirement is now meson >= 0.59.0).
I believe it should not happen in normal GUI case (which is when you create a
GimpProcedureDialog). I had the issue while moving around some plug-in code and
moved dialog creation before gimp_ui_init() by mistake. The issue was not
obvious until I followed the trace inside libgimp. This would be even more
frustrating for plug-in developers so let's have a clear warning message giving
the possible plug-in crash reason.
Similarly to the various GimpResource select PDB calls, this allows to call a
core dialog in order to choose a drawable which will be returned back to the
calling plug-in.
This new GimpPickableSelect dialog is a subclass of GimpPdbDialog and uses the
same GimpPickableChooser widget as GimpPickablePopup, except that since it's
inter-process window management, it is harder to make a popup positioned
accurately relatively to a parent (especially on Wayland). This is why it's a
separate widget as a simpler dialog (which we will still try to make transient
as much as possible across platforms).
This name was really irking me because it's not a button (anymore? Maybe it used
to be just a button). Depending on the specific widget, it will have several
sub-widgets, including a label. And it can theoretically even be something else
than a button.
So let's just rename these widgets with the more generic "chooser" name.
Similar to the latest commits for GimpBrush:
- gimp_pattern_get_buffer() returns a GeglBuffer and allow getting a scaled
version of the pattern.
- Old gimp_pattern_get_pixels() is made private.
- Moved GimpPattern into its own file and store the buffer to avoid re-querying
it through PDB continuously.
No as for the widget to select a pattern:
- Preview frame ensured to be square.
- Default size increased.
- Drawing code using the new gimp_pattern_get_buffer().
- Cleaned up code.
So what I realized was that the core was sending contents without transparency.
Actually the mask was our transparency channel here. Since in most use cases,
what you want to do when you request a brush buffer is to be able to draw it
somewhere, having a buffer already with alpha is much better, even more because
by default, it looks like background color is black which is possibly not what
you expect usually from a brush preview.
If someone wants absolutely no-alpha, it's easy to get rid of the channel. It's
simply better that the default behavior is the most expected use case.
- Increase the default size to 40x40 and multiply it by the current window scale
factor to have decent preview size.
- Make the brush preview always square with a GtkAspectFrame: even though
brushes are not necessarily square, this is a much more obvious size rather
than letting GTK choose a random allocation size which ends up very weird
looking.
- Scale down the brush to the biggest possible dimensions which fit the square
preview area (if the brush native size is already smaller, I don't scale up
though) while keeping aspect ratio: previous implementation was really weird,
as we were only seeing a tiny corner of much brushes as we weren't scaling
them down. Obviously I use new gimp_brush_get_buffer|mask() functions for
this as it supports scaling.
- Implement drawing color brushes too: the previous implementation was only
drawing the brush mask, which was absolutely not what would be expected for
such brushes.
- Add a white background behind color brushes with transparency.
- Simplify and clean up the code.
One of the consequences of this new implementation is obviously that it's
mandatory to call gegl_init() when using this widget.
… and gimp_brush_get_mask().
gimp_brush_get_pixels() was a bit crappy, returning raw data with only
dimensions and bpp to go with (no color model/space, no bit depth…). So the
assumption is that we work with 8-bit per channel data, possibly with alpha
depending of number of channels as deduced from bpp, and very likely in sRGB
color space. It might be globally ok with many of the brush formats (and
historical brushes) but won't fare well as we improve brush capabilities.
- gimp_brush_get_pixels() is in fact made private.
- The 2 new functions are using this old PDB call _gimp_brush_get_pixels() to
construct buffers. This has some limitations, in particular that it returns
only 8-bit per channel sRGB data, but at least the signature won't change when
we will improve things in the future (so if some day, we pass fancy brushes in
high-bit depth, the method will stay the same).
- This new implementation also allows scaling down the brush (keeping aspect
ratio) which is useful when you need to fit a brush preview into a drawing
widget.
- Current implementation stores the buffers at native size in the libgimp's
GimpBrush object, hence save re-querying the core every time you need an
update. This can be improved as current implementation also means that you
don't get updates if the brush changed. This should handle most common use
cases for now, though.
- Also with this change, I move GimpBrush class implementation into its own
dedicated file.
- Move the property widget functions for GimpResource properties into a new
libgimp/gimppropwidgets.[ch] file. This mirrors the files
libgimpwidgets/gimppropwidgets.[ch] which are for more generic property types.
- Rename the functions gimp_prop_chooser_*_new() to gimp_prop_*_chooser_new().
- gimp_prop_chooser_factory() doesn't need to be public.
- Add a label to GimpResourceSelectButton, make so that the
gimp_prop_chooser_*_new() functions set the property nick to this label and
add this label to the size group in GimpProcedureDialog.
- Removing useless or redundant code.
- Simplifying various logics.
- Using GimpResource directly in temporary PDB procedures, not resource names.
- Better cleanup of the core resource chooser when the plug-in dialog quits (we
need it to ask core to close also any visible resource chooser dialog).
- Replace the "Close" button by more common OK/Cancel. In particular, the
GimpPdbDialog now properly keeps track of the initial object and when hitting
"Cancel" (or Escape key), this initial object is set back.
- Clean up some of the comments, especially when the code is self explanatory.
There is still much more to clean and improve, but it's a first welcome step.
Found by the definitely useful libgimp warnings:
> gimp_plug_in_destroy_proxies: ERROR: GimpPattern proxy with ID 13 was refed by plug-in, it MUST NOT do that!
Brush, font, gradient, palette and pattern choices are currently chosen through
a dialog created by the core, which then returns the user choice to the calling
plug-in. This has the unfortunate consequence of having a pile of likely at
least 3 windows (main GIMP window by core process, plug-in window by plug-in
process, then the choice popup by the core process) shared in 2 processes, which
often end up under each other and that's messy. Even more as the choice popup is
kinda expected to be like a sub-part of the plug-in dialog.
So anyway, now the plug-in can send its window handle to the core so that the
resource choice dialog ends up always above the plug-in dialog.
Of course, it will always work only on platforms where we have working
inter-process transient support.
Instead of passing a guint32, pass the proper type, since our the HANDLE type
can be 64-bit on Windows (according to links I found).
I was hoping it might be the reason for the breakage under Windows, though I
also found Microsoft documentation saying that the 64-bit handle can be safely
truncated: https://learn.microsoft.com/en-us/windows/win32/winprog64/interprocess-communication?redirectedfrom=MSDN
Nevertheless I'd appreciate testing again from NikcDC or anyone else, as I
reactivated setting transient between processes on Windows.
Note that I also pass the proper types on X11 now (Window), even though guint32
worked fine. Better be thorough.
Having windows ID as guint32 is a mistake. Different systems have
different protocols. In Wayland in particular, Windows handles are
exchanged as strings. What this commit does is the following:
In core:
- get_window_id() virtual function in core GimpProgress is changed to
return a GBytes, as a generic "data" to represent a window differently
on different systems.
- All implementations of get_window_id() in various classes implementing
this interface are updated accordingly:
* GimpSubProgress
* GimpDisplay returns the handle of its shell.
* GimpDisplayShell now creates its window handle at construction with
libgimpwidget's gimp_widget_set_native_handle() and simply return
this handle every time it's requested.
* GimpFileDialog also creates its window handle at construction with
gimp_widget_set_native_handle().
- gimp_window_set_transient_for() in core is changed to take a
GimpProgress as argument (instead of a guint32 ID), requests and
process the ID itself, according to the running platform. In
particular, the following were improved:
* Unlike old code, it will work even if the window is not visible yet.
In such a case, the function simply adds a signal handler to set
transient at mapping. It makes it easier to use it at construction
in a reliable way.
* It now works for Wayland too, additionally to X11.
- GimpPdbProgress now exchanges a GBytes too with the command
GIMP_PROGRESS_COMMAND_GET_WINDOW.
- display_get_window_id() in gimp-gui.h also returns a GBytes now.
PDB/libgimp:
- gimp_display_get_window_handle() and gimp_progress_get_window_handle()
now return a GBytes to represent a window handle in an opaque way
(depending on the running platform).
In libgimp:
- GimpProgress's get_window() virtual function changed to return a
GBytes and renamed get_window_handle().
- In particular GimpProgressBar is the only implementation of
get_window_handle(). It creates its handle at object construction with
libgimpwidget's gimp_widget_set_native_handle() and the virtual
method's implementation simply returns the GBytes.
In libgimpUi:
- gimp_ui_get_display_window() and gimp_ui_get_progress_window() were
removed. We should not assume anymore that it is possible to create a
GdkWindow to be used. For instance this is not possible with Wayland
which has its own way to set a window transient with a string handle.
- gimp_window_set_transient_for_display() and
gimp_window_set_transient() now use an internal implementation similar
to core gimp_window_set_transient_for(), with the same improvements
(works even at construction when the window is not visible yet + works
for Wayland too).
In libgimpwidgets:
- New gimp_widget_set_native_handle() is a helper function used both in
core and libgimp* libraries for widgets which we want to be usable as
possible parents. It takes care of getting the relevant window handle
(depending on the running platform) and stores it in a given pointer,
either immediately or after a callback once the widget is mapped. So
it can be used at construction. Also it sets a handle for X11 or
Wayland.
In plug-ins:
- Screenshot uses the new gimp_progress_get_window_handle() directly now
in its X11 code path and creates out of it a GdkWindows itself with
gdk_x11_window_foreign_new_for_display().
Our inter-process transient implementation only worked for X11, and with
this commit, it works for Wayland too.
There is code for Windows but it is currently disabled as it apparently
hangs (there is a comment in-code which links to this old report:
https://bugzilla.gnome.org/show_bug.cgi?id=359538). NikcDC tested
yesterday with re-enabling the code and said they experienced a freeze.
;-(
Finally there is no infrastructure yet to make this work on macOS and
apparently there is no implementation of window handle in GDK for macOS
that I could find. I'm not sure if macOS doesn't have this concept of
setting transient on another processus's window or GDK is simply lacking
the implementation.
Jehan