From 422a32a7e4b3df26cfb4a2ac33efb44f62bf5dd6 Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 30 Mar 2024 14:17:35 +0100 Subject: [PATCH 1/6] Fix a use-after-free in OKLab picker rendering Replace an externally managed pixel buffer with one internal to the Cairo surface. --- src/ui/widget/oklab-color-wheel.cpp | 12 ++++++------ src/ui/widget/oklab-color-wheel.h | 1 - 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/ui/widget/oklab-color-wheel.cpp b/src/ui/widget/oklab-color-wheel.cpp index bcdfc00e64..c27d9627e9 100644 --- a/src/ui/widget/oklab-color-wheel.cpp +++ b/src/ui/widget/oklab-color-wheel.cpp @@ -50,6 +50,7 @@ bool OKWheel::setRgb(double r, double g, double b, if (changed_lightness) { _updateChromaBounds(); _redrawDisc(); + queue_drawing_area_draw(); } bool const changed = changed_hue || changed_saturation || changed_lightness; @@ -218,13 +219,15 @@ void OKWheel::on_drawing_area_draw(Cairo::RefPtr const &cr, int, void OKWheel::_redrawDisc() { int const size = std::ceil(2.0 * _disc_radius); - _pixbuf.resize(4 * size * size); - double const radius = 0.5 * size; double const inverse_radius = 1.0 / radius; + _disc = Cairo::ImageSurface::create(Cairo::Surface::Format::RGB24, size, size); + // Fill buffer with (, R, G, B) values. - uint32_t *pos = (uint32_t *)(_pixbuf.data()); + uint32_t *pos = (uint32_t *)(_disc->get_data()); + g_assert(pos); + for (int y = 0; y < size; y++) { // Convert (x, y) to a coordinate system where the // disc is the unit disc and the y-axis points up. @@ -234,9 +237,6 @@ void OKWheel::_redrawDisc() *pos++ = _discColor(pt); } } - - int const stride = Cairo::ImageSurface::format_stride_for_width(Cairo::Surface::Format::RGB24, size); - _disc = Cairo::ImageSurface::create(_pixbuf.data(), Cairo::Surface::Format::RGB24, size, size, stride); } /** @brief Convert widget (event) coordinates to an abstract coordinate system diff --git a/src/ui/widget/oklab-color-wheel.h b/src/ui/widget/oklab-color-wheel.h index ba9c20ee0b..db91196d69 100644 --- a/src/ui/widget/oklab-color-wheel.h +++ b/src/ui/widget/oklab-color-wheel.h @@ -68,7 +68,6 @@ private: double _disc_radius = 1.0; Geom::Point _margin; Cairo::RefPtr _disc; - std::vector _pixbuf; std::array _bounds; }; -- GitLab From 6a9b3f72c4ab097d0cfe343867e80c3acac17922 Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 30 Mar 2024 14:19:15 +0100 Subject: [PATCH 2/6] Simplify a function call --- src/ui/widget/oklab-color-wheel.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ui/widget/oklab-color-wheel.cpp b/src/ui/widget/oklab-color-wheel.cpp index c27d9627e9..e5f7e0b6bf 100644 --- a/src/ui/widget/oklab-color-wheel.cpp +++ b/src/ui/widget/oklab-color-wheel.cpp @@ -233,8 +233,7 @@ void OKWheel::_redrawDisc() // disc is the unit disc and the y-axis points up. double const normalized_y = inverse_radius * (radius - y); for (int x = 0; x < size; x++) { - auto const pt = Geom::Point(inverse_radius * (x - radius), normalized_y); - *pos++ = _discColor(pt); + *pos++ = _discColor({inverse_radius * (x - radius), normalized_y}); } } } -- GitLab From fa4e7f2235d0df50af31887f9fad9f6aeccce412 Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 30 Mar 2024 15:15:20 +0100 Subject: [PATCH 3/6] OKLab picker: refactor color calculation from coords Split off the calculation of an OK Lch colour from coordinates in the unit disc (corresponding to the picker wheel) into a separate function, capable of performing the calculation exactly in addition to the existing optimization of working with precomputed chroma bounds. --- src/ui/widget/oklab-color-wheel.cpp | 71 ++++++++++++++++++++++------- src/ui/widget/oklab-color-wheel.h | 3 ++ 2 files changed, 57 insertions(+), 17 deletions(-) diff --git a/src/ui/widget/oklab-color-wheel.cpp b/src/ui/widget/oklab-color-wheel.cpp index e5f7e0b6bf..1a3a05d537 100644 --- a/src/ui/widget/oklab-color-wheel.cpp +++ b/src/ui/widget/oklab-color-wheel.cpp @@ -115,30 +115,45 @@ bool OKWheel::_updateDimensions() return disc_needs_redraw; } -/** @brief Compute the ARGB32 color for a point inside the picker disc. +/** + * @brief Compute the color in OK Lch space corresponding to a point in the picker disc. * - * The picker disc is viewed as the unit disc in the xy-plane, with - * the y-axis pointing up. If the passed point lies outside of the unit - * disc, the returned color is the same as for a point rescaled to the - * unit circle (outermost possible color in that direction). + * @param point A point in the unit disc (mapped to the picker disc with y-axis facing up). + * @tparam exact Whether to use exact but slower computation. + * @tparam use_radians Whether the hue angle in the Lch should be in radians (otherwise degrees). * - * @param point A point in the normalized disc coordinates. - * @return a Cairo-compatible ARGB32 color. + * @return A triple (L, c, h) of OKLch coordinates of the color corresponding to the given point. */ -uint32_t OKWheel::_discColor(Geom::Point const &point) const +template +Oklab::Triplet OKWheel::_unitDiscPointToOkLch(Geom::Point const &point) const { using namespace Oklab; - using Display::AssembleARGB32; double saturation = point.length(); if (saturation == 0.0) { - auto [r, g, b] = oklab_to_rgb({ _values[L], 0.0, 0.0 }); - return AssembleARGB32(0xFF, (guint)(r * 255.5), (guint)(g * 255.5), (guint)(b * 255.5)); + // Chroma is zero (grayscale) therefore hue is undefined; use 0. + return {_values[L], 0.0, 0.0}; } else if (saturation > 1.0) { saturation = 1.0; } - double const hue_radians = Geom::Angle(Geom::atan2(point)).radians0(); + const auto hue_angle = Geom::Angle(Geom::atan2(point)); + if constexpr (exact) { + // Exact (but more expensive) computation of the OK Lch color + double const absolute_chroma = saturation * Oklab::max_chroma(_values[L], hue_angle.degrees()); + + if constexpr (use_radians) { + return {_values[L], absolute_chroma, hue_angle.radians0()}; + } + + // Convert hue angle to degrees in the range [0, 360) + double hue_degrees = hue_angle.degrees(); + if (hue_degrees < 0.0) hue_degrees += 360.0; + return {_values[L], absolute_chroma, hue_degrees}; + } + + // Faster but less precise computation using cached max chroma bounds. + double const hue_radians = hue_angle.radians0(); // Find the precomputed chroma bounds on both sides of this angle. unsigned previous_sample = std::floor(hue_radians * 0.5 * CHROMA_BOUND_SAMPLES / M_PI); @@ -152,8 +167,30 @@ uint32_t OKWheel::_discColor(Geom::Point const &point) const double const chroma_bound_estimate = Geom::lerp(t, _bounds[previous_sample], _bounds[next_sample]); double const absolute_chroma = chroma_bound_estimate * saturation; - auto [r, g, b] = oklab_to_rgb(oklch_radians_to_oklab({ _values[L], absolute_chroma, hue_radians })); - return AssembleARGB32(0xFF, (guint)(r * 255.5), (guint)(g * 255.5), (guint)(b * 255.5)); + if constexpr (use_radians) { + return {_values[L], absolute_chroma, hue_radians}; + } + + double hue_degrees = hue_angle.degrees(); + if (hue_degrees < 0.0) hue_degrees += 360.0; + return {_values[L], absolute_chroma, hue_degrees}; +} + +/** @brief Compute the ARGB32 color for a point inside the picker disc. + * + * The picker disc is viewed as the unit disc in the xy-plane, with + * the y-axis pointing up. If the passed point lies outside of the unit + * disc, the returned color is the same as for a point rescaled to the + * unit circle (outermost possible color in that direction). + * + * @param point A point in the normalized disc coordinates. + * @return a Cairo-compatible ARGB32 color. + */ +uint32_t OKWheel::_discColor(Geom::Point const &point) const +{ + using namespace Oklab; + auto [r, g, b] = oklab_to_rgb(oklch_radians_to_oklab(_unitDiscPointToOkLch(point))); + return Display::AssembleARGB32(0xFF, (guint)(r * 255.5), (guint)(g * 255.5), (guint)(b * 255.5)); } /** @brief Returns the position of the current color in the coordinates @@ -173,7 +210,7 @@ Geom::Point OKWheel::_curColorWheelCoords() const /** @brief Draw the widget into the Cairo context. */ void OKWheel::on_drawing_area_draw(Cairo::RefPtr const &cr, int, int) { - if(_updateDimensions()) { + if (_updateDimensions()) { _redrawDisc(); } @@ -253,11 +290,11 @@ Geom::Point OKWheel::_event2abstract(Geom::Point const &event_pt) const * @param pt A point in the abstract coordinate system in which the picker * disc is the unit disc and the y-axis points up. */ -bool OKWheel::_setColor(Geom::Point const &pt, bool const emit) +bool OKWheel::_setColor(Geom::Point const &pt, bool emit) { auto const s = std::clamp(pt.length(), 0.0, 1.0); - Geom::Angle clicked_hue = _values[S] ? Geom::atan2(pt) : 0.0; + Geom::Angle const clicked_hue = _values[S] ? Geom::atan2(pt) : 0.0; auto const h = clicked_hue.radians0(); bool const changed = _values[S] != s || _values[H] != h; diff --git a/src/ui/widget/oklab-color-wheel.h b/src/ui/widget/oklab-color-wheel.h index db91196d69..35f0a4f07f 100644 --- a/src/ui/widget/oklab-color-wheel.h +++ b/src/ui/widget/oklab-color-wheel.h @@ -15,6 +15,7 @@ #define SEEN_OKLAB_COLOR_WHEEL_H #include "ui/widget/ink-color-wheel.h" +#include "oklab.h" #include // GtkEventControllerMotion #include // Gtk::EventSequenceState @@ -50,6 +51,8 @@ private: static double constexpr HALO_STROKE = 1.5; ///< Width of the halo's stroke. Geom::Point _curColorWheelCoords() const; + template + Oklab::Triplet _unitDiscPointToOkLch(Geom::Point const &point) const; uint32_t _discColor(Geom::Point const &point) const; Geom::Point _event2abstract(Geom::Point const &point) const; void _redrawDisc(); -- GitLab From 9eb5764189b50e51f393b9446f3526ba95ff3638 Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 30 Mar 2024 15:35:31 +0100 Subject: [PATCH 4/6] Handle right mouse clicks on OKLab color wheel A right click sets the color just like a left click, but it does not initiate a drag. --- src/ui/widget/oklab-color-wheel.cpp | 30 ++++++++++++++++------------- src/ui/widget/oklab-color-wheel.h | 2 +- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/ui/widget/oklab-color-wheel.cpp b/src/ui/widget/oklab-color-wheel.cpp index 1a3a05d537..086f8f8db7 100644 --- a/src/ui/widget/oklab-color-wheel.cpp +++ b/src/ui/widget/oklab-color-wheel.cpp @@ -306,20 +306,25 @@ bool OKWheel::_setColor(Geom::Point const &pt, bool emit) return changed; } -/** @brief Handle a left mouse click on the widget. +/** @brief Handle a mouse click on the widget. * - * @param pt The clicked point expressed in the coordinate system in which - * the picker disc is the unit disc and the y-axis points up. + * @param pt The clicked point expressed in the coordinate system in which + * the picker disc is the unit disc and the y-axis points up. + * @param is_right_click Whether the right mouse button was clicked (context menu). * @return Whether the click has been handled. */ -bool OKWheel::_onClick(Geom::Point const &pt) +bool OKWheel::_onClick(Geom::Point const &pt, bool is_right_click) { auto r = pt.length(); if (r > 1.0) { // Clicked outside the disc, no cookie. return false; } - _adjusting = true; + _adjusting = !is_right_click; _setColor(pt); + + if (is_right_click) { + // TODO: add a context menu to copy out the CSS4 color values. + } return true; } @@ -327,15 +332,14 @@ bool OKWheel::_onClick(Geom::Point const &pt) Gtk::EventSequenceState OKWheel::on_click_pressed(Gtk::GestureClick const &click, int /*n_press*/, double const x, double const y) { - if (click.get_current_button() == 1) { - // Convert the click coordinates to the abstract coords in which - // the picker disc is the unit disc in the xy-plane. - if (_onClick(_event2abstract({x, y}))) { - return Gtk::EventSequenceState::CLAIMED; - } + const unsigned button_number = click.get_current_button(); + if (button_number != 1 && button_number != 3) { + return Gtk::EventSequenceState::NONE; } - // TODO: add a context menu to copy out the CSS4 color values. - return Gtk::EventSequenceState::NONE; + // Convert the click coordinates to the abstract coords in which + // the picker disc is the unit disc in the xy-plane. + return _onClick(_event2abstract({x, y}), button_number == 3) ? Gtk::EventSequenceState::CLAIMED + : Gtk::EventSequenceState::NONE; } /** @brief Handle a button release event. */ diff --git a/src/ui/widget/oklab-color-wheel.h b/src/ui/widget/oklab-color-wheel.h index 35f0a4f07f..40814c61e9 100644 --- a/src/ui/widget/oklab-color-wheel.h +++ b/src/ui/widget/oklab-color-wheel.h @@ -61,7 +61,7 @@ private: bool _updateDimensions(); // Event handlers - bool _onClick(Geom::Point const &unit_pos); + bool _onClick(Geom::Point const &unit_pos, bool is_right_click); Gtk::EventSequenceState on_click_pressed (Gtk::GestureClick const &click, int n_press, double x, double y) final; Gtk::EventSequenceState on_click_released(Gtk::GestureClick const &click, -- GitLab From dfb7c82d2e87b3ac541e090e1e23b09535982eca Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 30 Mar 2024 19:24:53 +0100 Subject: [PATCH 5/6] Compute color wheel offset The color wheel is automatically centered inside a widget called AspectFrame which occupies a rectangular area but exposes a square area (1:1 aspect ratio) to the picker. The newly added function computes the coordinates of the top left corner of the picker inside the complete rectangular area. --- src/ui/widget/ink-color-wheel.cpp | 8 ++++++++ src/ui/widget/ink-color-wheel.h | 1 + 2 files changed, 9 insertions(+) diff --git a/src/ui/widget/ink-color-wheel.cpp b/src/ui/widget/ink-color-wheel.cpp index 9c73b17ead..6068888985 100644 --- a/src/ui/widget/ink-color-wheel.cpp +++ b/src/ui/widget/ink-color-wheel.cpp @@ -170,6 +170,14 @@ void ColorWheel::focus_drawing_area() _drawing_area->grab_focus(); } +Geom::IntPoint ColorWheel::getDrawingAreaOffset() const +{ + auto const drawing_allocation = get_drawing_area_allocation(); + auto const total_allocation = get_allocation(); + return Geom::Point(get_xalign() * (total_allocation.get_width() - drawing_allocation.get_width()), + get_yalign() * (total_allocation.get_height() - drawing_allocation.get_height())).round(); +} + bool ColorWheel::on_key_released(GtkEventControllerKey const * /*controller*/, unsigned /*keyval*/, unsigned const keycode, GdkModifierType const state) diff --git a/src/ui/widget/ink-color-wheel.h b/src/ui/widget/ink-color-wheel.h index 57aa0135ed..47a3ecb392 100644 --- a/src/ui/widget/ink-color-wheel.h +++ b/src/ui/widget/ink-color-wheel.h @@ -113,6 +113,7 @@ protected: [[nodiscard]] Gtk::Allocation get_drawing_area_allocation() const; [[nodiscard]] bool drawing_area_has_focus() const; void focus_drawing_area(); + Geom::IntPoint getDrawingAreaOffset() const; private: sigc::signal _signal_color_changed; -- GitLab From b6bafa53013429b5bae41e60504113b4e2212ad8 Mon Sep 17 00:00:00 2001 From: Rafael Siejakowski Date: Sat, 30 Mar 2024 19:45:50 +0100 Subject: [PATCH 6/6] Add a context menu to copy OKLab picker colors The menu works with the "OKHSL" color wheel, allowing the user to copy RGB hex and CSS4 oklch() color values to the clipboard. Hopefully, this functionality can be generalized to cover other color wheels as well. Early prototype for https://gitlab.com/inkscape/inbox/-/issues/7862 --- po/POTFILES.src.in | 1 + src/ui/widget/oklab-color-wheel.cpp | 128 +++++++++++++++++++++++++++- src/ui/widget/oklab-color-wheel.h | 2 + 3 files changed, 127 insertions(+), 4 deletions(-) diff --git a/po/POTFILES.src.in b/po/POTFILES.src.in index 22fd4c8022..dccaf8ac59 100644 --- a/po/POTFILES.src.in +++ b/po/POTFILES.src.in @@ -413,6 +413,7 @@ ${_build_dir}/share/templates/templates.h ../src/ui/widget/licensor.cpp ../src/ui/widget/marker-combo-box.cpp ../src/ui/widget/object-composite-settings.cpp +../src/ui/widget/oklab-color-wheel.cpp ../src/ui/widget/page-properties.cpp ../src/ui/widget/page-selector.cpp ../src/ui/widget/paint-selector.cpp diff --git a/src/ui/widget/oklab-color-wheel.cpp b/src/ui/widget/oklab-color-wheel.cpp index 086f8f8db7..ed08965f57 100644 --- a/src/ui/widget/oklab-color-wheel.cpp +++ b/src/ui/widget/oklab-color-wheel.cpp @@ -15,10 +15,61 @@ #include #include +#include + +#include +#include +#include +#include +#include +#include +#include #include +#include #include "display/cairo-utils.h" #include "oklab.h" +#include "svg/svg-color.h" +#include "ui/menuize.h" +#include "ui/popup-menu.h" + +namespace { + +class OKWheelContextMenu : public Gtk::PopoverMenu +{ +public: + OKWheelContextMenu(Gtk::Widget &parent, + Gdk::Rectangle const &location, + Glib::RefPtr actions, + Glib::RefPtr model) + : _action_group{std::move(actions)} + { + set_visible(false); + set_name("OKLabPickerContextMenu"); + set_parent(parent); + set_menu_model(std::move(model)); + + set_has_arrow(false); + set_position(Gtk::PositionType::BOTTOM); + set_pointing_to(location); + Inkscape::UI::menuize_popover(*this); + + popup(); + } +private: + Glib::RefPtr _action_group; +}; + +class StringCopier +{ +public: + StringCopier(Glib::ustring to_copy) : _to_copy{std::move(to_copy)} {} + void operator()() { Gdk::Display::get_default()->get_clipboard()->set_text(_to_copy); } +private: + Glib::ustring _to_copy; +}; + +} // anonymous namespace namespace Inkscape::UI::Widget { @@ -285,6 +336,15 @@ Geom::Point OKWheel::_event2abstract(Geom::Point const &event_pt) const return result * Geom::Scale(scale, -scale); } +/** @brief Convert abstract coordinates in a unit disc (with the y-axis pointing up) + * to the coordinate system of the widget. + */ +Geom::Point OKWheel::_abstract2event(Geom::Point const &abstract_pt) const +{ + auto real_sized = abstract_pt * Geom::Scale(_disc_radius, -_disc_radius); + return real_sized + _margin + Geom::Point(_disc_radius, _disc_radius); +} + /** @brief Set the current color based on a point on the wheel. * * @param pt A point in the abstract coordinate system in which the picker @@ -315,19 +375,79 @@ bool OKWheel::_setColor(Geom::Point const &pt, bool emit) */ bool OKWheel::_onClick(Geom::Point const &pt, bool is_right_click) { + auto color_point{pt}; auto r = pt.length(); - if (r > 1.0) { // Clicked outside the disc, no cookie. - return false; + if (r > 1.0) { + color_point = pt.normalized(); } _adjusting = !is_right_click; - _setColor(pt); + _setColor(color_point); if (is_right_click) { - // TODO: add a context menu to copy out the CSS4 color values. + _showContextMenu(color_point); } return true; } +/// \todo: Replace these functions after the color space refactoring (Merge Request 6150). +namespace temporary { +Glib::ustring format_color(uint32_t rgb) +{ + constexpr unsigned bufsize = 32; + char buffer[bufsize]; + sp_svg_write_color(buffer, bufsize, rgb << 8); + return buffer; +} + +Glib::ustring format_color(Oklab::Triplet const &oklch_color) +{ + auto &[L, c, h] = oklch_color; + if (L < 0.001) { //black + return "oklch(0 none none)"; + } else if (L > 0.999) { // white + return "oklch(1 none none)"; + } + + std::stringstream formatter; + formatter.imbue(std::locale("C")); + formatter.precision(3); + formatter << "oklch(" << L << ' ' << c << ' ' << h << ')'; + return formatter.str(); +} +} + +/** + * @brief Show the context menu for copying color values. + * @param pt Position in abstract coordinates (within the unit disc). + */ +void OKWheel::_showContextMenu(Geom::Point const &pt) +{ + using namespace Gio; + auto const popup_position = _abstract2event(pt).round() + getDrawingAreaOffset(); + int constexpr target_size = static_cast(0.5 + HALO_RADIUS + 2.0 * HALO_STROKE); + Gdk::Rectangle popup_location{popup_position.x(), popup_position.y(), target_size, target_size}; + + auto actions = SimpleActionGroup::create(); + const Glib::ustring action_prefix{"ctx"}; + insert_action_group(action_prefix, actions); + auto model = Menu::create(); + + auto const add_copy_color_menuitem = [&actions, &model, &action_prefix](const char *action_name, + Glib::ustring color_to_copy) + { + auto menuItem = MenuItem::create(Glib::ustring::compose(_("Copy \"%1\""), color_to_copy), + action_prefix + '.' + action_name); + actions->add_action(action_name, StringCopier(std::move(color_to_copy))); + model->append_item(menuItem); + }; + + // Create "Copy color" items + add_copy_color_menuitem("copy-hex-color", temporary::format_color(getRgb())); + add_copy_color_menuitem("copy-oklch-color", temporary::format_color(_unitDiscPointToOkLch(pt))); + + UI::on_hide_reset(std::make_shared(*this, popup_location, actions, model)); +} + /** @brief Handle a button press event. */ Gtk::EventSequenceState OKWheel::on_click_pressed(Gtk::GestureClick const &click, int /*n_press*/, double const x, double const y) diff --git a/src/ui/widget/oklab-color-wheel.h b/src/ui/widget/oklab-color-wheel.h index 40814c61e9..2b98a73802 100644 --- a/src/ui/widget/oklab-color-wheel.h +++ b/src/ui/widget/oklab-color-wheel.h @@ -55,6 +55,8 @@ private: Oklab::Triplet _unitDiscPointToOkLch(Geom::Point const &point) const; uint32_t _discColor(Geom::Point const &point) const; Geom::Point _event2abstract(Geom::Point const &point) const; + Geom::Point _abstract2event(Geom::Point const &point) const; + void _showContextMenu(Geom::Point const &point); void _redrawDisc(); bool _setColor(Geom::Point const &pt, bool emit = true); void _updateChromaBounds(); -- GitLab