...by adding parameters and porting to
GimpProcedureConfig/GimpProcedureDialog.
Code was also rearranged to match the ordering
of other plug-ins.
As a note, the help message is "Special effects that nobody understands".
It would be nice if someone with more familiarity with this plug-in could
update the help messages to explain the parameters.
I had the case when "Sphere" script crashed, bringing down the whole script-fu
plug-in (while trying to reproduce #10214). Then after being run, we get a
dangling pointer to a finalized action object.
Even in successful use cases, we will want to give the ability to unregister
normal plug-ins/procedures wrapped as GIMP extensions, and there is also the use
case of temporary procedures, so I'm sure this bug could be reproducible even in
normal non-problematic runs.
I am pretty sure that this should be in single selection mode because we don't
even really have code to handle cases with multiple brushes or font selected.
Right now, we assume in many places that there is only one font or brush (or
other data) active at a given time.
Yet this code (or older versions of it) is old apparently and I realize that
even in 2.10, I can ctrl/shift click to select several data objects. This is the
weird part.
Anyway let's put this in single selection mode and see how it goes. If there
were actually use cases which I didn't know about, I'm sure we'll soon have
reports.
I could still see annoying scrolling up/down happening when we are deselecting
an item (typically with ctrl-click). In such a case, the cursor is on a
deselected item. Just make it bump to a closest item, preferably a visible one.
Note that "save-type" and "mipmaps" were left as ints rather than GimpChoice,
as some options are conditionally disabled and I don't know if that
is available in GimpChoice just yet.
A DDS loading bug related to 6ad54ca3 was also resolved.
The correct header value to check for the pixel size was not "depth" but "bpp".
My first versions were commits 98f0c448 then 1d8782915e but the more I go, the
better I understand the implications of the selection vs. the cursor. In
particular, when setting a cursor, which also initializes the selection to this
item only, the tree view would also scroll to this item. The current
implementation, which sets the top item as cursor, is therefore particularly bad
for multi-selection which doesn't fit fully in the view, because we also end up
scrolling up. Say you have a long list of layers, you first select the top
layer, then scroll down to the bottom layer and ctrl-click it: the selection
(now 2 items) works but you end up scrolled back all the way up.
This alternate version is much better, by ensuring that your cursor is at least
within the selection (hence avoiding the discrepancy between keyboard navigation
and pointer navigation, and which was fixed with commit 98f0c448), so that we
don't try to change the cursor when possible.
This was broken in commit 3e101922. Setting a cursor basically resets to a
single selection, invalidating pointer-made multi-selection.
But then we got back the bug it fixes, which is that we must grab focus after
the selection is actually made. So we now grab at the end.
This also had a bad consequence for multi-selection (again): if the focus was
not already on the tree view, gimp_container_tree_view_selection_changed() was
not called. This function was where the actual selection-changing is meant to
happen. So we had to shift-click (or ctrl-click) twice. The first time, nothing
would happen (but focus was given to the tree view). The second time, we could
finally update the selection.
This is why we add 2 different cases of focus grab, which should hopefully
handle all cases correctly, though this code is really extra-complex. This
replaces MR !1128.
I also remove (without replacement) 2 usages of gimp_get_data() without
associated gimp_set_data(). According to the comment, it looks like the
associated data was likely set by the GAP plug-in instead. Let's drop this.
Finally I see a lot of arguments, several of them being float arrays, so I
wondered if they were related to the curve and point arrays we are storing as
aux arg now. Apparently these arguments are not even used in the plug-in, right
now, which is explained in commit e318651c99. Maybe if these were actually
used/set and if we implement float array (de)serialization, would this aux
argument become useless?
Lastly, I deleted some code paths which were never used. For instance, stored
bval.total_steps was never set to anything else than 0 anywhere (so I removed
both the variable and the other code-path, which actually ended up being the
GAP-related data get). Additionally in p_bender_calculate_iter_curve(), there
was a big else block which was only happening when the GAP-related variables
were set. It got deleted too (as is a function only used in this block of code).
The argument is the serialized description of the generated fractal. Actually it
may even be useful as a non-aux argument. A GFile argument could eventually be
interesting too, though one arg per setting would be better of course (it allows
more easily to generate animated frames for instance). In such a case, the GFile
would be used in priority, otherwise the other args.
Notes:
* I didn't port to GimpProcedureDialog though it looks like it would not be too
hard (most of the GUI would still be custom GTK code, yet we could have the
generic "reset to initial|factory values" buttons and load/save).
* The custom "Reset" button (identical to what "reset to factory values" would
do) works fine anyway.
* The "run with last vals" works fine where there were indeed previous runs
(which may be in previous sessions), but crashes when it's actually the first
run ever. Some of the base structure data were not initialized. It should not
be too hard yet would require a bit of code reorganization to fix this.
- Argument "parameter-uri" becomes "parameter-file" (GFile instead of URI
string).
- Adding a "settings-data" auxiliary bytes argument to handle all settings for
the time being, instead of using gimp_get_data() and gimp_set_data().
- "last_file_name" was also removed from the ValueType struct, using the
"settings-data" arg instead.
- Fixing the non-interactive usage which was missing a gegl_init() call.
- Use GIO for various reading and writing to a CML parameter file.
- Better bubbling up of error messages.
- Fix the loading of the last section of CML parameters file.
- Also fixing deactived file dialog when clicking the open button a second time.
We use a GBytes auxiliary argument (i.e. not visible in the public API) which
perfectly replace gimp_[gs]et_data() API, in even better (since it also works
across sessions).
The "Reset to initial values|factory defaults" still don't work. We could make
these work even with the AUX args, but I feel like this would be far too much
work for a non-optimal solution anyway. The real solution can be when we get
more public arguments to this PDB procedure to handle every setting
individually. This can happen after GIMP 3 release (especially now that
arguments number and order don't break the PDB API anymore).
I added a bunch of arguments, in particular nearly all the settings in the main
dialog, except for "Colormap" as I'm unsure yet how to handle it. It looks like
a mix of GimpChoice and GimpDrawable. I guess it could just be both (2 args,
using values from the GimpChoice if GimpDrawable arg is NULL).
And I didn't make the settings in the "Edit Flame" dialog into arguments as I'm
not sure if the "Speed" and "Variation" in particular should be args, or are
just temporary args only meant to construct a flame (in particular, it looks
like it builds data defining the flame?). These data should be their own args
probably?
Maybe we could also have the "file" as alternative way to init the flame data
(the "Open" button)?
For now, I'm just storing the whole settings struct as an aux argument, so that
"Load/Save Settings" work, as well as the initial values from previous run.
Also the whole code is still a bit of a mess. I feel like we could really
simplify a lot of the code, all these values stored globally or duplicated now
in both the GimpProcedureConfig and the control_point structure.
Resolves#9989
In 2.10/GTK2, clicking an already selected toolbutton caused the
background color to change to indicate this action.
This patch restores that behavior through CSS updates.
It also slightly rearranges the CSS to group related button styles.
Since we were not chaining up with parent's finalize(), we were not
removing the instance from the gimp-debug infrastructure which assumed
the object was leaked and would try to read its reference count for
debugging purpose, when GIMP_DEBUG=all was set.
In fact, the object was not leaked, therefore we got into a segfault
when dereferencing already freed memory.
I added a few specific validations for range types (int, double…), and a
generic validation at the end, meant to catch all yet uncaught invalid
argument cases (yet with less details on the what and why).
In Preferences > Toolbox, clicking on an item below the
initial scroll window view causes it to jump to the top
automatically. This patch prevents this by setting the
clicked index in the GtkTreeView before grabbing focus.
I had this one crash upon exit, inside gimp_align_options_update_area() as a
consequence of gimp_align_options_image_changed() being called on an image
change. I could not reproduce after this one time and it's very likely a race
condition when everything is getting finalized, and the tool options object is
getting finalized earlier than the user context.
Anyway this should fix the potential crash.
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.
Alx Sa