New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Opacity Toolbox for Color Item #5271
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- Direct access to GdkEvent members (see here). Use the getters instead.
- 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.
- 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. |
This commit introduces a new feature: an opacity toolbox that appears as a floating widget next to the color item when clicked. 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.
In older versions, the opacity toolbox was hidden when clicking in the PageView. However, the UI/UX design has evolved to hide it when leaving the toolbox. This commit addresses the outdated code in the PageView, removing the now-obsolete include statements. By doing so, it improves the codebase's clarity and ensures that the commit history remains focused on relevant changes.
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
afd3452
to
4cca060
Compare
Enter/leave handlers are now connected directly to the ColorToolItem instead of using overlaid eventboxes. This makes the code much simpler and clearer. The code has also been rewritten to anticipate a for port to GTK4.
19514e2
to
7593b85
Compare
A condition was missing in FloatingToolbox::handleLeaveFloatingToolbox before checking if the pointer is over the opacity toolbox. If the opacity toolbox is hidden, there's no need to check if the pointer is over it (and it should not, otherwise xoj::util::gtk::isEventOverWidget would complain).
974ac66
to
80974a5
Compare
This commit allows a user to set the opacity for the current selection by using the opacity toolbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
|
||
void OpacityToolbox::updateScaleValue() { | ||
GtkRange* rangeWidget = (GtkRange*)this->theMainWindow->get("opacityToolboxScaleAlpha"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instance should keep a pointer to this range, instead of fetching it everytime. GladeGui::get() is relatively expensive.
cairo_t* cr = cairo.get(); | ||
|
||
cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE); | ||
cairo_set_source_rgb(cr, 255, 255, 255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the background follow either the paper color or the UI background color?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. A color can look quite different depending on its surroundings or the elements around it
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use cairo_fill_preserve/cairo_stroke_preserve if the same path is gonna be used again
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); |
Color borderColor = this->color; | ||
borderColor.alpha = 255; | ||
Util::cairo_set_source_argb(cr, borderColor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use Util::cairo_set_source_rgbi() for this
|
||
xoj::util::GObjectSPtr<GdkPixbuf> pixbuf( | ||
gdk_pixbuf_get_from_surface(surface.get(), 0, 0, PREVIEW_WIDTH, PREVIEW_HEIGHT), xoj::util::adopt); | ||
gtk_image_set_from_pixbuf(GTK_IMAGE(theMainWindow->get("opacityToolboxImg")), pixbuf.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep a pointer to this GtkImage instead of fetching it everytime. See above.
} | ||
|
||
bool ColorToolItem::handleEnter(GtkEventController* eventController, gdouble x, gdouble y, ColorToolItem* self) { | ||
if (self->enterLeaveController.get() == eventController) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
if (win) { | ||
win->getOpacityToolbox()->update(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
Co-authored-by: Benjamin Hennion <17818041+bhennion@users.noreply.github.com>
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