[gimp] app: fix leak of a GdkPoint and improve stable zoom centering logics.



commit f5ea8e9b2aaa9c82692127685d8aafc285d0ff31
Author: Jehan <jehan girinstud io>
Date:   Fri Aug 20 20:09:50 2021 +0200

    app: fix leak of a GdkPoint and improve stable zoom centering logics.
    
    This started as yet another report of leak by Massimo. But really the
    leak of the GdkPoint created by the function
    gimp_display_shell_push_zoom_focus_pointer_pos() is not only when
    delta_y is 0. There are a few code paths in gimp_display_shell_scale()
    when we would not pop this point. One of them is for instance when
    window resizing in multi-window mode is allowed. There might be more
    (but the code is convoluted enough not to be 100% sure if these are
    possible with our specific case).
    
    This specific function was initially created only to be used for unit
    testing code (commit 7e3898da093), but it ended up being also used
    internally (commit 792cd581a21). Since I see that the test for which
    this code was initially created even seem broken right now (the assert
    part for position check is commented out!), I even wonder if we should
    keep it. We could indeed instead just add optional start_x|y arguments
    to gimp_display_shell_scale(), which would be much cleaner. But I leave
    it for now.
    
    Instead I just make sure we clean the created GdkPoint after calling
    gimp_display_shell_scale(). Also I get rid of the GQueue. It is clear in
    the code that we are not expecting queuing interaction of several
    positions. Worse right now, we could end up in weird cases where the
    pushed points are not used when they should, then could end up being
    used later in totally unrelated interactions (this would make the shell
    position jump here and there). So let's just make it a single point.
    Finally adding some appropriate comments in parts which are still a bit
    wrong.

 app/display/gimpdisplayshell-scale.c | 69 ++++++++++++++++++++++++------------
 app/display/gimpdisplayshell.c       |  8 +++--
 app/display/gimpdisplayshell.h       |  2 +-
 3 files changed, 53 insertions(+), 26 deletions(-)
---
diff --git a/app/display/gimpdisplayshell-scale.c b/app/display/gimpdisplayshell-scale.c
index 26afbbbf22..24c7d234f9 100644
--- a/app/display/gimpdisplayshell-scale.c
+++ b/app/display/gimpdisplayshell-scale.c
@@ -539,6 +539,10 @@ gimp_display_shell_scale (GimpDisplayShell *shell,
       gimp_zoom_model_zoom (shell->zoom, GIMP_ZOOM_TO, new_scale);
 
       gimp_display_shell_scale_resize (shell, TRUE, FALSE);
+
+      /* XXX The @zoom_focus policy is clearly not working in this code
+       * path. This should be fixed.
+       */
     }
   else
     {
@@ -805,21 +809,33 @@ gimp_display_shell_scale_drag (GimpDisplayShell *shell,
 
   scale = gimp_zoom_model_get_factor (shell->zoom);
 
-  gimp_display_shell_push_zoom_focus_pointer_pos (shell, start_x, start_y);
-
-  if (delta_y > 0)
-    {
-      gimp_display_shell_scale (shell,
-                                GIMP_ZOOM_TO,
-                                scale * 1.1,
-                                GIMP_ZOOM_FOCUS_POINTER);
-    }
-  else if (delta_y < 0)
+  if (delta_y != 0.0)
     {
-      gimp_display_shell_scale (shell,
-                                GIMP_ZOOM_TO,
-                                scale * 0.9,
-                                GIMP_ZOOM_FOCUS_POINTER);
+      gimp_display_shell_push_zoom_focus_pointer_pos (shell, start_x, start_y);
+
+      if (delta_y > 0.0)
+        {
+          gimp_display_shell_scale (shell,
+                                    GIMP_ZOOM_TO,
+                                    scale * 1.1,
+                                    GIMP_ZOOM_FOCUS_POINTER);
+        }
+      else /* delta_y < 0.0 */
+        {
+          gimp_display_shell_scale (shell,
+                                    GIMP_ZOOM_TO,
+                                    scale * 0.9,
+                                    GIMP_ZOOM_FOCUS_POINTER);
+        }
+
+      if (shell->zoom_focus_point)
+        {
+          /* In case we hit one of the cases when the focus pointer
+           * position was unused.
+           */
+          g_slice_free (GdkPoint, shell->zoom_focus_point);
+          shell->zoom_focus_point = NULL;
+        }
     }
 }
 
@@ -997,6 +1013,13 @@ gimp_display_shell_get_rotated_scale (GimpDisplayShell *shell,
  *
  * When the zoom focus mechanism asks for the pointer the next time,
  * use @x and @y.
+ *
+ * It was primarily created for unit testing (see commit 7e3898da093).
+ * Therefore it should not be used by our code. When it is, we should
+ * make sure that @shell->zoom_focus_point has been properly used and
+ * freed, if we don't want it to leak.
+ * Just calling gimp_display_shell_scale() is not enough as there are
+ * currently some code paths where the values is not used.
  **/
 void
 gimp_display_shell_push_zoom_focus_pointer_pos (GimpDisplayShell *shell,
@@ -1007,8 +1030,8 @@ gimp_display_shell_push_zoom_focus_pointer_pos (GimpDisplayShell *shell,
   point->x = x;
   point->y = y;
 
-  g_queue_push_head (shell->zoom_focus_pointer_queue,
-                     point);
+  g_slice_free (GdkPoint, shell->zoom_focus_point);
+  shell->zoom_focus_point = point;
 }
 
 
@@ -1372,16 +1395,16 @@ gimp_display_shell_scale_get_zoom_focus (GimpDisplayShell *shell,
       gtk_get_event_widget (event) == shell->canvas ||
       gtk_get_event_widget (event) == window)
     {
-      GdkPoint *point = g_queue_pop_head (shell->zoom_focus_pointer_queue);
-      gint      canvas_pointer_x;
-      gint      canvas_pointer_y;
+      gint canvas_pointer_x;
+      gint canvas_pointer_y;
 
-      if (point)
+      if (shell->zoom_focus_point)
         {
-          canvas_pointer_x = point->x;
-          canvas_pointer_y = point->y;
+          canvas_pointer_x = shell->zoom_focus_point->x;
+          canvas_pointer_y = shell->zoom_focus_point->y;
 
-          g_slice_free (GdkPoint, point);
+          g_slice_free (GdkPoint, shell->zoom_focus_point);
+          shell->zoom_focus_point = NULL;
         }
       else
         {
diff --git a/app/display/gimpdisplayshell.c b/app/display/gimpdisplayshell.c
index e0a553c21c..998500e874 100644
--- a/app/display/gimpdisplayshell.c
+++ b/app/display/gimpdisplayshell.c
@@ -374,7 +374,7 @@ gimp_display_shell_init (GimpDisplayShell *shell)
                     G_CALLBACK (gimp_display_shell_buffer_hover),
                     shell);
 
-  shell->zoom_focus_pointer_queue = g_queue_new ();
+  shell->zoom_focus_point = NULL;
 
   gtk_widget_set_events (GTK_WIDGET (shell), (GDK_POINTER_MOTION_MASK    |
                                               GDK_BUTTON_PRESS_MASK      |
@@ -782,7 +782,11 @@ gimp_display_shell_dispose (GObject *object)
 
   g_clear_object (&shell->motion_buffer);
 
-  g_clear_pointer (&shell->zoom_focus_pointer_queue, g_queue_free);
+  if (shell->zoom_focus_point)
+    {
+      g_slice_free (GdkPoint, shell->zoom_focus_point);
+      shell->zoom_focus_point = NULL;
+    }
 
   if (shell->title_idle_id)
     {
diff --git a/app/display/gimpdisplayshell.h b/app/display/gimpdisplayshell.h
index 2f63a88c53..f9bf6360f0 100644
--- a/app/display/gimpdisplayshell.h
+++ b/app/display/gimpdisplayshell.h
@@ -232,7 +232,7 @@ struct _GimpDisplayShell
 
   GimpMotionBuffer  *motion_buffer;
 
-  GQueue            *zoom_focus_pointer_queue;
+  GdkPoint          *zoom_focus_point;
 
   gboolean           blink;
   guint              blink_timeout_id;


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]