Add Opacity Toolbox for Color Item

[jerdoe]

This commit introduces a new feature: an opacity toolbox that appears as a non-movable floating widget next to the clicked color item.

The toolbox provides a slider to adjust the opacity of the tool and offers real-time preview functionality, making it easier to fine-tune the tool's transparency.

This enhancement improves the user experience by providing a more seamless control over the tool opacity.

Opacity.Toolbox.demo.mp4

[bhennion]

Thanks a lot for the nice feature!
I have one main concern about the code (which is otherwise very clean): in order to make an eventual port possible, we have been trying to move away from GTK 3 features that have been removed from GTK 4.
You seem to be using quite a few gtk-3-only things, which is a small step back for the porting effort:

1. Direct access to GdkEvent members (see here). Use the getters instead.
2. Parts of the GtkOverlay have been retired/replaced. It would be nice to add porting-help wrappers, like the ones in util/gtk4-helper.{h,cpp}, so that we never use in the main code functions that will no longer exist.
3. The use of the retired class GtkEventBox (see here). Probably not possible to stop using it. Maybe a good old #if GTK_MAJOR_VERSION == 3 #else #endif would help with the future porting.

[jerdoe]

Thanks a lot for the nice feature! I have one main concern about the code (which is otherwise very clean): in order to make an eventual port possible, we have been trying to move away from GTK 3 features that have been removed from GTK 4. You seem to be using quite a few gtk-3-only things, which is a small step back for the porting effort:

1. Direct access to GdkEvent members (see here). Use the getters instead.
2. Parts of the GtkOverlay have been retired/replaced. It would be nice to add porting-help wrappers, like the ones in util/gtk4-helper.{h,cpp}, so that we never use in the main code functions that will no longer exist.
3. The use of the retired class GtkEventBox (see here). Probably not possible to stop using it. Maybe a good old #if GTK_MAJOR_VERSION == 3 #else #endif would help with the future porting.

Thank you very much for your prompt feedback and your deep involvement in this project. It's a real pleasure to contribute to Xournal++ and to learn from you every time you review my code. I have a background in Java and worked as a developer until 2010. This is my first involvement in a C/C++ project, so there are still some aspects I'm not completely comfortable with. It's truly exciting to learn programming in C/C++ and GTK, although I must admit that I'm facing challenges in understanding some essential concepts, especially those related to memory management.

I'm currently a bit short on time, but I'll gladly fix it as soon as I have more free time.
I can't give you an exact time frame right now. But if you're eager for a quick fix, feel free to tackle it yourself, or let other contributors give it a shot. I'll still be here to explain my code to anyone who needs it.

[bhennion]

I gave this a quick look. It looks great and works very well!

I have one main request about the OpacityToolbox::update() use in Control. See below. The rest is nitpicks.

[bhennion]

The instance should keep a pointer to this range, instead of fetching it everytime. GladeGui::get() is relatively expensive.

[bhennion]

Should the background follow either the paper color or the UI background color?

[jerdoe]

Yes, I agree. A color can look quite different depending on its surroundings or the elements around it

[bhennion]

Use cairo_fill_preserve/cairo_stroke_preserve if the same path is gonna be used again

Suggested change

	cairo_fill(cr);
	if (addBorder) {
	cairo_set_line_width(cr, 5);
	cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
	Color borderColor = this->color;
	borderColor.alpha = 255;
	Util::cairo_set_source_argb(cr, borderColor);
	cairo_rectangle(cr, PREVIEW_BORDER, PREVIEW_BORDER, PREVIEW_WIDTH - PREVIEW_BORDER * 2,
	PREVIEW_HEIGHT - PREVIEW_BORDER * 2);
	cairo_stroke(cr);
	if (!addBorder) {
	cairo_fill(cr);
	} else {
	cairo_fill_preserve(cr);
	cairo_set_line_width(cr, 5);
	cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
	Color borderColor = this->color;
	borderColor.alpha = 255;
	Util::cairo_set_source_argb(cr, borderColor);
	cairo_stroke(cr);

[bhennion]

You can use Util::cairo_set_source_rgbi() for this

[bhennion]

Keep a pointer to this GtkImage instead of fetching it everytime. See above.

[bhennion]

Is this test really necessary?

[jerdoe]

Since the eventController is already attached to the widget, this test is not necessary.

Not directly related to this, I have another question. According to the gtk document, the caller of the function gtk_event_controller_motion_new takes ownership of the data, and is responsible for freeing it. Do I have to do something to free the memory allocated by the EventController in the destructor, or is this already automatically handled by GObjectSPtr?

[bhennion]

I think it'd be cleaner if the OpacityToolbox connected to some GAction state's (via g_connect_signal(action, "notify::state", ...), there are a couple of examples of this in our codebase).
The idea is that Control should not really know of UI details such as "there is a popover doing this".

You may need to add an action (see control/actions/ActionProperties.h) whose uint8_t state is the current tool's (filling) opacity value, similar to Action::TOOL_COLOR. (Renaming the existing TOOL_OPACITY_FILL is not off the table :)

[jerdoe]

@bhennion,
Wow, I felt a bit concerned by putting so many opacityToolbox->update() through all over the place and It looked quite unmaintenable and messy.

I thought these GActions were only connected to menu and buttons and did not know you could get notified anywhere from the code when they are triggered: this perfectly fits to my case.

I'm really amazed how things get always cleaner and optimized with you. Thank you very much.

Right now, I have a little problem.

given the following code :

ActionRef gActionSelectTool = actionDB->getAction(Action::SELECT_TOOL);

g_signal_connect(gActionSelectTool.get(), "notify::state", G_CALLBACK(+[](GObject* a, GParamSpec*, OpacityToolbox* self) {
		xoj::util::GVariantSPtr state(g_action_get_state(G_ACTION(a)), xoj::util::adopt);
		auto toolType = getGVariantValue<ToolType>(state.get());
		g_log("OpacityToolbox::gActionSelectTool", G_LOG_LEVEL_DEBUG, "toolType_from_state=%s ; toolType_from_toolhandler=%s", toolTypeToString(toolType).c_str(), toolTypeToString(self->toolHandler->getToolType()).c_str());
	}),
	this);

When I click on a button to select a new tool, I get notified before the new tool is selected via ctrl->selectTool() in the callback.

/** Tool menu **/
template <>
struct ActionProperties<Action::SELECT_TOOL> {
    using state_type = ToolType;
    using parameter_type = state_type;
    static state_type initialState(Control* ctrl) { return ctrl->getToolHandler()->getActiveTool()->getToolType(); }
    static void callback(GSimpleAction* ga, GVariant* p, Control* ctrl) {
    	g_log("ActionProperties::SELECT_TOOL callback()", G_LOG_LEVEL_DEBUG, "--> entered");
        g_simple_action_set_state(ga, p);
        ToolType tt = getGVariantValue<ToolType>(p);
        xoj_assert(tt < TOOL_END_ENTRY);
        if (requiresClearedSelection(tt)) {
            ctrl->clearSelection();
        }
        g_log("ActionProperties::SELECT_TOOL callback()", G_LOG_LEVEL_DEBUG, "--> ctrl->selectTool");
        ctrl->selectTool(tt);
    }
};

So at this time, toolHandler is not yet in the updated state that I need to update the opacityToolbox accordingly.
However, since ctrl->selectTool() is invoked, and thus, also Control::toolChanged() ...

void Control::toolChanged() {
    g_log("Control::toolChanged", G_LOG_LEVEL_DEBUG, "--> entered");
    ToolType type = toolHandler->getToolType();

    this->actionDB->setActionState(Action::SELECT_TOOL, type);

Do you have an idea why I don't get notified from this->actionDB->setActionState(Action::SELECT_TOOL, type);? Is there a way to be notiified after the tool handler has already set the new tool, rather than just before?

[bhennion]

So, GSimpleAction instances basically have two methods to the state: g_simple_action_set_state() and g_action_change_state():

• g_action_change_state() essentially only fires the signal change-state (without actually modifying the state). The state is then changed via a call to g_simple_action_set_state() in the callback.
• g_simple_action_set_state() actually changes the state, without firing the signal change-state. The signal notify::state gets fired however, because the property state got modified.

The idea is: any UI component just connects to the notify::state signal so that it is displayed according to the actual value of the state property. Only the ActionProperties::callback is connected to the change-state signal.

Now if you want, e.g., the tool type to be sest before the notify::state signals are emitted, just move down the line

g_simple_action_set_state(ga, p);

in ActionPropertiesAction::SELECT_TOOL::callback().
If the rest of the interactions with the action state is coded as it should be, there should be no side effects.

[jerdoe]

Thank you for your further explanations.

Now I understand why by inlining ctrl->selectTool(tt); in the callback, the infered g_simple_action_set_state(ga, p) will not notify my handler. This is because the state did not change between the two calls of g_simple_action_set_state().
I mistakenly thought that a simple call to g_simple_action_set_state would notify the handler, but it actually requires a change in the state for the notification to occur.