From 9f75c2b075b7d9c2bd68bef20c5da67b4833eb34 Mon Sep 17 00:00:00 2001
From: Tim Ledbetter <timledbetter@gmail.com>
Date: Thu, 23 Feb 2023 18:14:59 +0000
Subject: [PATCH] PixelPaint: Minimize clone tool sample marker repaints

This removes 2 FIXMEs :^)
---
 .../PixelPaint/Tools/CloneTool.cpp            | 46 +++++++++++++------
 .../Applications/PixelPaint/Tools/CloneTool.h |  2 +
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/Userland/Applications/PixelPaint/Tools/CloneTool.cpp b/Userland/Applications/PixelPaint/Tools/CloneTool.cpp
index 4e64896bba3..8b73bf1a8fb 100644
--- a/Userland/Applications/PixelPaint/Tools/CloneTool.cpp
+++ b/Userland/Applications/PixelPaint/Tools/CloneTool.cpp
@@ -66,9 +66,9 @@ void CloneTool::on_mousemove(Layer* layer, MouseEvent& event)
         return;
 
     if (m_cursor_offset.has_value()) {
+        auto old_sample_marker_rect = sample_marker_rect();
         m_sample_location = image_event.position() - m_cursor_offset.value();
-        // FIXME: This is a really inefficient way to update the marker's location
-        m_editor->update();
+        update_sample_marker(old_sample_marker_rect);
     }
 
     BrushTool::on_mousemove(layer, event);
@@ -78,10 +78,10 @@ void CloneTool::on_mousedown(Layer* layer, MouseEvent& event)
 {
     auto& image_event = event.image_event();
     if (image_event.alt()) {
+        auto old_sample_marker_rect = sample_marker_rect();
         m_sample_location = image_event.position();
         m_cursor_offset = {};
-        // FIXME: This is a really dumb way to get the marker to show up
-        m_editor->update();
+        update_sample_marker(old_sample_marker_rect);
         return;
     }
 
@@ -102,16 +102,8 @@ void CloneTool::on_second_paint(Layer const*, GUI::PaintEvent& event)
     GUI::Painter painter(*m_editor);
     painter.add_clip_rect(event.rect());
 
-    auto sample_pos = m_editor->content_to_frame_position(m_sample_location.value());
-    // We don't want the marker to be a single pixel and hide the color.
-    auto offset = AK::max(2, size() / 2);
-    Gfx::IntRect rect = {
-        (int)sample_pos.x() - offset,
-        (int)sample_pos.y() - offset,
-        offset * 2,
-        offset * 2
-    };
-    painter.draw_ellipse_intersecting(rect, m_marker_color, 1);
+    auto rect = sample_marker_rect();
+    painter.draw_ellipse_intersecting(rect.value(), m_marker_color, 1);
 }
 
 bool CloneTool::on_keydown(GUI::KeyEvent& event)
@@ -177,4 +169,30 @@ ErrorOr<GUI::Widget*> CloneTool::get_properties_widget()
     return m_properties_widget.ptr();
 }
 
+Optional<Gfx::IntRect> CloneTool::sample_marker_rect()
+{
+    if (!m_sample_location.has_value())
+        return {};
+
+    auto sample_pos = m_editor->content_to_frame_position(m_sample_location.value());
+    // We don't want the marker to be a single pixel and hide the color.
+    auto offset = AK::max(2, size() / 2);
+    return Gfx::IntRect {
+        (int)sample_pos.x() - offset,
+        (int)sample_pos.y() - offset,
+        offset * 2,
+        offset * 2
+    };
+}
+
+void CloneTool::update_sample_marker(Optional<Gfx::IntRect> old_rect)
+{
+    if (old_rect.has_value())
+        m_editor->update(old_rect.value().inflated(2, 2));
+
+    auto current_rect = sample_marker_rect();
+    if (current_rect.has_value())
+        m_editor->update(current_rect.value().inflated(2, 2));
+}
+
 }
diff --git a/Userland/Applications/PixelPaint/Tools/CloneTool.h b/Userland/Applications/PixelPaint/Tools/CloneTool.h
index 5a8af4bf778..053378b3811 100644
--- a/Userland/Applications/PixelPaint/Tools/CloneTool.h
+++ b/Userland/Applications/PixelPaint/Tools/CloneTool.h
@@ -32,6 +32,8 @@ protected:
 
 private:
     virtual StringView tool_name() const override { return "Clone Tool"sv; }
+    Optional<Gfx::IntRect> sample_marker_rect();
+    void update_sample_marker(Optional<Gfx::IntRect> old_rect);
 
     RefPtr<GUI::Widget> m_properties_widget;