As usual, these got forgotten!
Also colorxhtml is actually deeply broken by commit 89c359ce47 which removed
gimp_drawable_get_pixel() (Niels thought it was not used, whereas it was simply
harder to spot with bindings!).
This will have to be fixed eventually.
I was hoping to redesign further but the whole logic of animated brush creation
is made so complicated that I think it would require a whole more complex GUI
with visual hints. So anyway I stopped at basic redesign and port to the new
dialog generation code.
It still makes the code much simpler and a bit more powerful (also less bugs
hopefully). I have also reviewed the procedure arguments, removing redundant
ones (display-cols and display-rows are just computed from cell-width and
cell-height) and adding some aux arguments instead to simplify dynamic GUI code.
The assert tests were not taking well into account the case where upper == lower
or where it's an integer spin which is just separated by 1 (both cases seem
silly, but it makes sense in the case of generic — or even dynamic! — spin
widgets where we want to adjust the min and max, e.g. depending on the property
of the image, or on other settings.
Now gimp_procedure_dialog_get_label() can work both with an existing property ID
or a new property ID. In the former case, it will simply sync the label with the
procedure argument, which will make it easy to update the label contents. In the
latter case, it just initialize with the provided text.
It's the basic stupid port, though this dialog is so bad that I think I'm going
for a second stage with a basic redesign of this GUI while also moving to
GimpSaveProcedureDialog. To be continued.
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).
The arg "num-images" is clearly redundant as the "images" array already has this
information. Moreover it is bug-prone, for instance running 'Repeat "Filmstrip"'
would crash the plug-in which was defaulting to NULL "images" and 1 "num-images"
(without any GimpItem serialization ability yet).
Of course, we could just fix this, but better remove the redundant arg and all
possibilities of future similar bugs.
Also I now display a proper error message when running without input images
which says: "\"Filmstrip\" cannot be run without any input images"
The widget_creator_func() given to gimp_prop_chooser_factory() will create an
object which will take its own reference. We must release the one we got with
g_object_get().
Metadata handling is also integrated in this API:
* while giving a possibility to disable metadata saving if you want to do it
yourself (e.g. in file-heif), by setting a NULL MimeType;
* and the GimpMetadata object is added as run() argument, allowing one to edit
the metadata during the run, while still letting the infrastucture handle the
save (e.g. in file-jpeg);
* or to save intermediate metadata with gimp_procedure_config_save_metadata()
(e.g. in file-tiff).
While we definitely should not use this inside app/, because having the private
structure easily accessible as a member is very convenient, it is clear that it
makes for a much nicer public signature. Also the priv member is of no help to
third-party developers using this API to make plug-ins and is better hidden in
such a case.
The dialog itself is ported to GimpProcedureDialog, but the original
plug-in only had one config parameter (Number of Tiles).
Some of the other options might make sense as config parameters
such as the horizontal/vertical toggles and opacity.
I needed a few more GimpProcedureDialog widgets, which I now have. The only part
still not generated in this plug-in is the image selection. This is hundreds of
lines less!
It now has arguments for every usable setting (except "keep-height" which is an
aux arg because it is simulated with a 0 "film-height" in non-interactive mode).
Also the film-height/keep-height default in interactive mode is now much better.
I override the film-height default and set it to be the image's height. This
makes for a much better default at all time.
The "Reset" button in Advanced section is not useful anymore since we now have
the generic "Reset" (to Initial/Factory) of GimpProcedureDialog, though it
actually doesn't work right for the time being, as it fails serializing
GimpFont.
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).
I was clearly confused when I wrote this. The sinking part matters to take
ownership of a reference in the widgets table, but we don't need to ref widgets
again before inserting them in containers. We were leaking widgets and as a
consequence the config object (and as a second consequence, some objects such as
resources for resource-selection widgets).
It would be worth porting when GimpDrawableComboBox widgets can be used as
GimpProcedureDialog generated widgets. Or even better, if the GimpPickableButton
were ported to libgimp to be used by plug-ins (since it's actually a lot more
usable).
I didn't port to GimpProcedureDialog because we'd need first a GimpSizeEntry
generatable widget, which can map to several properties (here for instance 3
width properties).
… working with a GimpProcedureConfig.
These are the easiest ones to port as most of the work was already done (mostly
just some now useless calls to remove).
Same as with gimp_procedure_new2(), I will end up renaming the function to
gimp_image_procedure_new() once all usage of this function will have been ported
to the new function using GimpProcedureConfig instead of GimpValueArray
arguments.
The one in GimpResourceSelect is a very nice example of why using config objects
is much nicer, getting arguments by their name instead of an index (which in
this case had to be tracked down by a private function to handle different
cases).
Also in gimp_procedure_config_begin_run(), make sure we sync the arguments with
the config object first thing, even in interactive and with-last-vals case
(where the args may be further overridden). This was especially important for
Script-fu scripts as the image and drawable were not provided separately, so we
need to make sure that the config file has the right values.
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).
A lot less lines for the same functionalities!
This includes improvements too:
* "include-pointer" as new PDB argument;
* settings storage thanks to GimpProcedureDialog;
* it should hopefully work better in non-interactive (though it's untested).
The macOS and Windows screenshots are untested after this change, though they
should still work the same. Please report if there is any problem!
This function allows to change the sensitivity of a widget depending on the
value of another property.
We already had gimp_procedure_dialog_set_sensitive() except it was only syncing
with a boolean property, whereas the new function can compare with any property
type.
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.
