So it turns out that the "notify::src-drawables" property signal in
particular can happen during gimp_paint_tool_paint_interpolate() called
from gimp_paint_tool_paint_thread(). Though the function
gimp_clone_options_gui_update_src_label() was run in the main thread in
simple cases, being called this way through a paint thread happens when
very quickly changing the source while painting, which is what Aryeom
does (when I see her using the clone tool, she would sometimes change
the source very quickly several times in barely a few seconds).
Yet GTK and GDK calls must not happen in non-main threads. It used to be
acceptable when protected with gdk_threads_enter|leave() calls but doing
this is deprecated. The now sanctioned method is to call the GTK code in
an idle function since these are guaranteed to run in the main thread.
This was most likely explaining why crashes could quite randomly happen,
though I'm not sure why I never had those (even though I could reproduce
the GTK calls happening in non-main threads, but without crashing GIMP)
and why Aryeom gets these much more often suddenly. Maybe some recent
dependency change which would trigger these more easily or other
context-dependant changes (as most non thread-safe code, bugs and crash
often happen in race conditions, so are harder to reproduce reliably)?
We take a step back from the original MR which was proposing the "single
dot" cursor as a new "Pointer mode" option. I was really unsure this was
the best solution, especially reading again the whole original report.
It means that now nearly all of the original patch has been rewritten
another way, but let's leave the contributor commit as a start point to
get to where we are, and as acknowledgement of the contribution.
The reporter was annoyed by the crosshair when none were requested and
probably mostly for painting tools only (at least examples were about
brush or pencil, etc.) while showing outline. It looks to me like the
real issue was maybe when we were showing the big crosshair when using
the 4-arc fallback outline, for instance when using a dynamics changing
size. If so, this main issue is already fixed by my commit 64dc26064b.
No need of a new option for this, especially if the option can be as
confusing as a barely visible dot-cursor (I can already imagine the bug
reports of people tweaking random preferences and unhappy because the
pointer became invisible, while they don't know how they did it).
Instead I would say that when people specifically uncheck both "Show
brush outline" and "Show pointer" options, showing a huge crosshair
feels quite counter-productive. This is where I think that our small
unobtrusive cursor (probably a better name than "Single dot" by the way,
as it's not a single dot anymore) might be of use, the ultimate case
when someone really want a cursor as inconspicuous as possible, while
still having a visible feedback of the pointer position (even with
display-tablets, parallax issues make such a visual feedback important
to target where one paints).
So let's try this first and see how it goes.
If someone explicitly asked not to get a pointer, yet to have the
outline, we should not force a crosshair on them while the fallback
outline is a perfectly visible mark (4 arcs around the pointer's
position, well delimiting the brush size).
It's still the same visually but it will be useful for 2 reasons: first,
it makes nicer code to show/hide only this one box instead of 3 frames;
second it will be used for the release note demo feature so that we can
blink the full line art settings box.
… blinking it.
This will be necessary to demo new features available only in some
situations. E.g. a new option in line art detection mode of bucket fill
would require said mode to be enabled.
… a given order, not at the same time.
This will be used for the release item notifications to show people
where new or changed features are, but in an ordered blink scenario. For
instance: select a tool first, then blink some tool icon, open the tool
options and finally blink the specific new/changed option. I am hoping
it would help awareness of changes (just words on a web news may make
some features a bit foreign when not used to look in details in advanced
options).
It should help with scrolling the dockable to make the widget visible,
as I don't see a generic and simple scrolling API for GTK scrollable
containers.
In my own testing, it did work if the dockable was already opened. But
if it was just created by the blink API, then giving focus immediately
doesn't properly scroll. I am not really going to investigate now, but
this should be fixed eventually.
This function will help us raise attention to various widgets in
dockables by blinking them similarly to how we blink locks or visibility
icons.
I associate this with the fact that property widgets identifier will be
the property name, so we get identifiers for free when creating widgets
through the propwidgets API.
… on the line art source.
Also now the default tooltip itself will be more appropriate rather than
reusing the "fill-transparent" property blurb (maybe I should have made
this its own property in fact).
This is the same thing as setting the max gap length to 0, except that
it would mean constantly having to play with the gap length and possibly
losing settings you carefully tweaked. Instead with this additional
settings, we hide the gap length settings when automatic closure is
disabled.
Also it makes kind of a nice parallel with the "Manual closure" settings
which can also be enabled/disabled similarly.
Since we can't select a source, the source tools would fail stroking
with an error. It was usable when stroking a path or selection as we
can have the source tool started while running these. Since only one
tool can be started at once, this is not possible when running bucket
fill tool (it would require some logics change on tools).
This change has 2 parts: add an "insensitive" column to
GimpContainerComboBox (allowing to mark some cells as insensitive), then
use this new attribute in the GimpBucketFillOptions to show all source
tools as insensitive.
There are clearly 3 types of settings:
1. How the line art is detected.
2. How do we additionally close line art.
3. How to handle fill borders.
Additionally I am rewording some options:
- "Allow closing lines in selected layer" -> "Manual closure in fill
layer"
- Adding the "Maximum gap length" into an "Automatic closure" frame to
hopefully make a parallel with the "manual" closure settings.
- "Allow completely transparent regions to be filled" -> "Detect opacity
rather than grayscale" (only within the context of the line art bucket
fill) because the main idea of this option is really here that we
detect lines as being opaque pixels, rather than detecting lines are
being dark pixels.
Finally don't hide the manual closure frame when in a configuration
where it won't be taken into account. Only make it insensitive. Having
options disappear is not so nice. Usually you want to gray them out to
have people realize they are simply not always usable.
Currently the option is quite simple. What should happen to make it more
usable:
* Right now, it uses the last stroke options (e.g. as used in a
previously run "Stroke Selection" or "Stroke Path"). We should have
some dedicated GUI in the bucket fill options.
* The bucket fill options GUI should really be redesigned. The more we
add options, the less understandable it is.
* There is a question whether we want to just use whatever brush size is
being set or if we want to have it vary and follow the line art width
(since we have proper distance map, we could use this to tweak the
stroke per-coords).
As usual, this feature was suggested by Aryeom who was still very
saddened that despite all the fancy features in this tool, it is not
able to produce nice rendering. So she proposed that the tool could
stroke the fill region borders.
… gimp_prop_widget_set_factor() to libgimpwidgets.
Now that GimpSpinScale is in libgimpwidgets, it's time to move the
associated prop too, to make it a prop widget with such a widget easily
creatable by plug-ins.
While doing so, I update both these functions logic, binding properties
together with the g_object_bind_property*() APIs (as we do already in
some other recent prop functions) rather than connecting to signals
ourselves. It makes for much simpler code.
There is really nothing specific to the core application, it is quite a
generic widget, so it would be nice for plug-ins to be able to use this
widget.
It can be argued that layer groups can't be painted on, and that's
probably the original reason, but it's really just the same as "Lock
pixels". It is interesting to be able to lock alpha channels on a layer
group to simply lock all its contents alpha channels.
Since we are now allowed to move groups (which is the same thing as
multi-selecting all its children and moving them), it makes no sense
that this lock is disabled.
This works the same way as "Lock pixels" in that a locked grouped also
forbid moving children. And there was already some logics so that you
can't move a layer group if one of it's children is locked. So this lock
really works both ways and is a bit special.
Finally I cleaned up a bit the multi-layer selection logics and
messaging, as well as which lock to blink (similar to the previous
commit) for the "Lock position" case.
In particular, if painting on a layer whose parent's pixels are locked,
we were blinking an empty lock spot, which is confusing. Now
gimp_item_is_content_locked() will also return the proper item (when
relevant, i.e. when returning TRUE) which is locked. It may or may not
the same item as passed in (it may also be a parent item in particular).
We were getting a critical when selecting multiple drawables (either
changing layer selection while the tool is ON, or starting with multiple
selection). We should not have assert code here, just handle the case
gracefully with a normal error message when trying to fill on several
layers at once.
The line art algorithm is useful but not always accurate enough and
sometimes it can even be counter-productive to fast painters.
A technique of advanced painters which Aryeom uses and teaches when she
wants to close an area without actually closing the line art (i.e. the
non-closed line is a stylistic choice) is to close with a brush the area
on the color layer. It has also a great advantage over the line art
"smart" closing algorithm: you control the brush style and the exact
shape of the closure (therefore something you'd usually have to redo
with a closure made by an algorithm as you would likely not find it
pretty enough).
This new feature takes this technique into account. Basically rather
than relying on the closure algorithm, you would close yourself and the
tool is able to recognize closure pixels by the color proximity with the
fill color.
Final point is that this additional step is made after line art
computation i.e. in particular the target drawable is not added to the
sources for line art logics. This allows to stay fast otherwise the line
art would have to recompute itself after each fill.
This also shows why the previous commit of moving the line art object
was necessary, because a painter would likely want to move regularly
from bucket fill to a brush tool to create area closures and we want to
avoid recomputation every time.
… fill tool finalizes.
The idea is that you may want to quickly switch tool to do something and
back to the bucket fill for line art selection. If the input drawable(s)
did not change, the previously computed data is still valid, so let's
hang on the line art object a little longer.
Since we are resetting the input when we get back, we would still
recompute anyway *if needed*, and the line art object does follow
changes on the input pickable so we would not get any deprecated data
anyway. Still we move around ownership a tiny bit to optimize for
common case of tool switching.
In order not to keep forever this data (buffer and distmap in
particular) forever just because one tried the line art once, I add a
timer to free it after a few minutes.
Moreover it will be useful for other cases, such as being able to share
the same line art object with the fuzzy select tool (if we end up adding
the line art option there). In a coming commit, the usage will be even
more obvious for use case where you want to edit the filled area with
other paint tools, then back to bucket fill while not touching the line
art source layer.
Actually we had half of the fix already with my recent change to source
tools giving the ability to clone from multiple source drawables where I
moved the source drawables from GimpSourceCore to GimpSourceOptions (see
commit 6ad00cdbba). The problem is that gimp_vectors_stroke() is using
the context paint options, but it is creating a brand new paint core
just to stroke the path. Hence with my recent changes, the drawable
sources were finally available to the path stroke, yet not the source
position. So stroking with clone was always starting on position (0, 0)
on the source.
This is now fixed by moving also the position properties "src-x" and
"src-y" on the GimpSourceOptions. This makes this info finally
accessible to the core when running the source tools this way.
Thanks to Eneko Castresana Vara for their initial contribution.
Unfortunately the code had diverged too much for it to be usable because
of our much too late review so a different fix had to be done.
Fixes#4333
If the checkbox is unchecked: dynamics falls back to "Dynamics Off",
the current dynamics name and its options are hidden in the UI.
If the checkbox is checked: dynamics is set to previously used one
or the default one, all dynamics options are seen in the UI.
Relative to the MR !553 where I could verify that the function
gimp_tool_push_status() does not just push new messages, it also removes
any other message from the same context (and place the new one on
front, unlike gimp_tool_replace_status()).
Therefore calling gimp_tool_pop_status() before a push or replace is
simply wrong with undesirable effect (e.g. too many useless redraws,
which can be pretty bad on some platforms like macOS, but are not ideal
anyway as a general rule).
This change reduces redraws due to spurious status updates.
Before this change, the status would be unset, then set again to the
same value it was previously. Each turn around setting and unsetting
causes a redraw.
On macOS this redraw causes a full screen redraw. With this
optimization, when the status message is unchanged, no redraw occurs.
This is because `gimp_tool_push_status` checks to see if the message
has changed.
I cleaned many remaining places where the concept of linked item still
survived.
On loading an XCF file with linked items, we are now going to create a
named sets for the linked items, allowing people to easily select these
back if the relation was still needed.
We don't remove gimp_item_get_linked() yet and in particular, we don't
save stored items into XCF files. This will come in an upcoming change.
- removing the GIMP_ITEM_SET_LINKED enum value.
- removing gimp_image_item_list_linked(). Now we should directly use
gimp_image_item_list_filter() instead.
- "preview-linked" option for transform tools is no more.
Use this for the alpha lock brought by GimpLayerTreeView.
Also use the new gimp_widget_blink_rect() to only blink the appropriate
cell when a lock is preventing you from doing something.
The "Source" dropdown to choose an image or pattern, and to check
"Sample merged" seem important enough that I moved them up the source
tool options. I also added a label giving information about the image
source being currently set, i.e. in particular which image (when the
source is another image), how many composited layers (or all of them
with "Sample merged" checked), or if each layer is its own source.
For this to happen, I moved src-drawables property from GimpSourceCore
to GimpSourceOptions (though without making it a config property,
because we don't want this option to be saved in config files). It
actually makes sense, it is a kind of "option" of how the tool will
behave, and then it is also visible by the options GUI.
By doing this, I also add the ability to use a composited projection of
the selected drawables as source. This is similar to "Sample merged"
except that instead of using the whole visible image, we use what would
have been visible if only the selected layers existed.
Note that this doesn't work together with the previously added ability
of multi-cloning from each layer to itself. This ability works for
cloning from multiple layers to one.
- Make the various virtual methods of GimpPaintCore use a list of
drawables as argument instead of a single drawable.
- gimp_brush_core_eval_transform_dynamics() can work with an image as
argument rather than a drawable as it doesn't actually depends on
specific drawable data.
- New function gimp_paint_tool_enable_multi_paint() to be used in init()
method of paint tools to announce that this tool can work with
multiple layers selected.
- Use gimp_paint_tool_enable_multi_paint() in the GimpSourceTool base
class only for now.
This is a first step for multi-layer drawing, but we don't want it to be
possible in just any random cases, which is why I add a special function
to advertize this capability. We will use it for special-casing the
clone (as well as heal and perspective tools most likely) tool to work
on several layers at once. At this step, it is still very bugged and not
really working properly. In particular, since we don't process the
drawable offset early anymore (because it makes no sense when we pass a
list of drawables with different offsets), I suspect that all the
offset-related code will be very broken.
Since it appeared with GLib 2.68.0, we could not change this until we
bumped the dependency which has only become possible a few days ago
(since Debian testing is our baseline for dependency bumps). Cf.
previous commit.
As this is a drop-in replacement (just a guint parameter changed to
gsize to avoid integer overflow), search-and-replace with:
> sed -i 's/g_memdup\>/g_memdup2/g' `grep -rIl 'g_memdup\>' *`
… followed by a few manual alignment tweaks when necessary.
This gets rid of the many deprecation warnings which we had lately when
building with a recent GLib version.
