Re: MPS: scroll-bars

[Helmut Eller <eller.helmut@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

On Sun, May 05 2024, Gerd Möllmann wrote:

> I believe the frame::condemned_scroll_bars is okay on NS as it is now.
> With all caveats as usual.

I have a hunch that those would need weak references to work as
intended.  But I'm happy to let them leak for now.

Below is my patch for the scroll-bars.  I went for the indirection layer
solution.  Though, the mark-and-sweep pool idea might be more broadly
usable; with so many different GUI toolkits it's hard to say.

I also had a problem with the tool-bar, triggered by this test:
(dotimes (i 40)
  (igc--collect)
  (message "%S" (make-list i nil))
  (redisplay))

The third patch stops allocating glyph matrices as ambiguous roots.
Seems to work for me.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-scroll-bars-for-GTK.patch --]
[-- Type: text/x-diff, Size: 5363 bytes --]

From 0e0cc32798f6dc8947e411d0e58ae4b8ea2aaced Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 5 May 2024 06:36:24 +0200
Subject: [PATCH] Fix scroll bars for GTK

Use a layer of indirection when struct scroll_bar pointers are
used in GTK callbacks.

* src/gtkutil.c (xg_finish_scroll_bar_creation): Create a cell
with a pointer to the scroll_bar and pass it to callbacks.  Also
Register a "destroy" callback to free it.
(xg_gtk_scroll_free_scroll_bar_cell): New.

* src/xterm.c (xg_scroll_callback, xg_end_scroll_callback): Load
the scroll_bar* out the cell.
(syms_of_xterm): Staticpro window_being_scrolled.
(x_unprotect_window_for_callback): Clear unused
elements in the protected_windows array to allow windows to be
collected.
(x_cache_xi_devices, xi_disable_devices, x_term_init): Add FIXMEs.
---
 src/gtkutil.c | 37 +++++++++++++++++++++++++++++++++++--
 src/xterm.c   | 21 +++++++++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index a01b8a41d5d..342dd35da13 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -44,6 +44,9 @@ #define xp pgtk
 #include "termhooks.h"
 #include "keyboard.h"
 #include "coding.h"
+#ifdef HAVE_MPS
+#include "igc.h"
+#endif
 
 #include <gdk/gdkkeysyms.h>
 
@@ -4523,6 +4526,13 @@ xg_scroll_bar_size_allocate_cb (GtkWidget *widget,
 }
 #endif
 
+static void
+xg_gtk_scroll_free_scroll_bar_cell (GtkWidget *widget, gpointer data)
+{
+  struct scroll_bar **bar_cell = data;
+  igc_xfree (bar_cell);
+}
+
 static void
 xg_finish_scroll_bar_creation (struct frame *f,
                                GtkWidget *wscroll,
@@ -4558,14 +4568,37 @@ xg_finish_scroll_bar_creation (struct frame *f,
                     (gpointer) scroll_id);
 #endif
 
+#ifdef HAVE_MPS
+  // FIXME: can use exact reference
+  struct scroll_bar **bar_cell =
+    igc_xzalloc_ambig (sizeof (struct scroll_bar*));
+  bar_cell[0] = bar;
+#endif
   g_signal_connect (G_OBJECT (wscroll),
                     "change-value",
                     scroll_callback,
-                    (gpointer) bar);
+#ifdef HAVE_MPS
+                    (gpointer) bar_cell
+#else
+		    (gpointer) bar
+#endif
+		    );
   g_signal_connect (G_OBJECT (wscroll),
                     "button-release-event",
                     end_callback,
-                    (gpointer) bar);
+#ifdef HAVE_MPS
+                    (gpointer) bar_cell
+#else
+		    (gpointer) bar
+#endif
+		    );
+
+#ifdef HAVE_MPS
+    g_signal_connect (G_OBJECT (wscroll),
+		      "destroy",
+		      G_CALLBACK (xg_gtk_scroll_free_scroll_bar_cell),
+		      (gpointer) bar_cell);
+#endif
 
   /* The scroll bar widget does not draw on a window of its own.  Instead
      it draws on the parent window, in this case the edit widget.  So
diff --git a/src/xterm.c b/src/xterm.c
index c47c7b0534d..7acd7f9c2a5 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -5748,6 +5748,7 @@ x_cache_xi_devices (struct x_display_info *dpyinfo)
     }
 
 #ifdef HAVE_MPS
+  // FIXME: use exact references
   dpyinfo->devices = igc_xzalloc_ambig (sizeof *dpyinfo->devices * ndevices);
 #else
   dpyinfo->devices = xzalloc (sizeof *dpyinfo->devices * ndevices);
@@ -13782,6 +13783,7 @@ xi_disable_devices (struct x_display_info *dpyinfo,
 
   ndevices = 0;
 #ifdef HAVE_MPS
+  // FIXME: use exact references
   devices = igc_xzalloc_ambig (sizeof *devices * dpyinfo->num_devices);
 #else
   devices = xzalloc (sizeof *devices * dpyinfo->num_devices);
@@ -15480,6 +15482,10 @@ x_unprotect_window_for_callback (struct x_display_info *dpyinfo)
     memmove (dpyinfo->protected_windows, &dpyinfo->protected_windows[1],
 	     sizeof (Lisp_Object) * dpyinfo->n_protected_windows);
 
+#ifdef HAVE_MPS
+  dpyinfo->protected_windows[dpyinfo->n_protected_windows] = Qnil;
+#endif
+
   return window;
 }
 
@@ -15718,7 +15724,12 @@ xg_scroll_callback (GtkRange *range, GtkScrollType scroll,
 
   whole = 0;
   portion = 0;
+#ifdef HAVE_MPS
+  struct scroll_bar** bar_cell = user_data;
+  bar = *bar_cell;
+#else
   bar = user_data;
+#endif
   part = scroll_bar_nowhere;
   adj = GTK_ADJUSTMENT (gtk_range_get_adjustment (range));
   f = g_object_get_data (G_OBJECT (range), XG_FRAME_DATA);
@@ -15795,9 +15806,14 @@ xg_end_scroll_callback (GtkWidget *widget,
                         GdkEventButton *event,
                         gpointer user_data)
 {
+#ifdef HAVE_MPS
+  struct scroll_bar **bar_cell = user_data;
+  struct scroll_bar *bar = *bar_cell;
+#else
   struct scroll_bar *bar;
 
   bar = user_data;
+#endif
   bar->dragging = -1;
 
   if (WINDOWP (window_being_scrolled))
@@ -16936,6 +16952,7 @@ XTset_vertical_scroll_bar (struct window *w, int portion, int whole, int positio
 	 Redraw the scroll bar manually.  */
       x_scroll_bar_redraw (bar);
 #endif
+
     }
   else
     {
@@ -30693,6 +30710,7 @@ #define NUM_ARGV 10
   /* We have definitely succeeded.  Record the new connection.  */
 
 #ifdef HAVE_MPS
+  // FIXME: use exact references
   dpyinfo = igc_xzalloc_ambig (sizeof *dpyinfo);
 #else
   dpyinfo = xzalloc (sizeof *dpyinfo);
@@ -32498,6 +32516,9 @@ syms_of_xterm (void)
   x_dnd_unsupported_drop_data = Qnil;
   staticpro (&x_dnd_unsupported_drop_data);
 
+  window_being_scrolled = Qnil;
+  staticpro (&window_being_scrolled);
+
   /* Used by x_cr_export_frames.  */
   DEFSYM (Qconcat, "concat");
 
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Make-xg_frame_tb_info-an-ambiguous-root.patch --]
[-- Type: text/x-diff, Size: 1357 bytes --]

From fd98e52e0efdb5e7bbf37919e3e95f8e440313b0 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 5 May 2024 09:01:06 +0200
Subject: [PATCH] Make xg_frame_tb_info an ambiguous root

We could use exact references in the future.

* src/gtkutil.c (xg_create_tool_bar): Use igc_xzalloc_ambig for
allocation.
(free_frame_tool_bar): Use igc_xfree to free it.
---
 src/gtkutil.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index 342dd35da13..31e0b915000 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -5496,7 +5496,12 @@ xg_create_tool_bar (struct frame *f)
                          TB_INFO_KEY);
   if (! tbinfo)
     {
+#ifdef HAVE_MPS
+      // FIXME: use exact references
+      tbinfo = igc_xzalloc_ambig (sizeof (*tbinfo));
+#else
       tbinfo = xmalloc (sizeof (*tbinfo));
+#endif
       tbinfo->last_tool_bar = Qnil;
       tbinfo->style = Qnil;
       tbinfo->hmargin = tbinfo->vmargin = 0;
@@ -6162,7 +6167,11 @@ free_frame_tool_bar (struct frame *f)
                                   TB_INFO_KEY);
       if (tbinfo)
         {
+#ifdef HAVE_MPS
+	  igc_xfree (tbinfo);
+#else
           xfree (tbinfo);
+#endif
           g_object_set_data (G_OBJECT (FRAME_GTK_OUTER_WIDGET (f)),
                              TB_INFO_KEY,
                              NULL);
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Glyph-matrices-don-t-need-to-be-ambiguous-roots.patch --]
[-- Type: text/x-diff, Size: 3120 bytes --]

From 3d3187e5c55a770cd68b1144479a70032b3c3558 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Sun, 5 May 2024 09:38:48 +0200
Subject: [PATCH] Glyph matrices don't need to be ambiguous roots

We scan them with fix_glyph_matrix as part of fix_windows.  So avoid
scanning them twice.  save_current_matrix and
restore_current_matrix create temporary matrices that aren't
seen by fix_glyph_matrix; these are still ambiguous roots.

* src/dispnew.c (adjust_glyph_matrix, realloc_glyph_pool)
(free_glyph_matrix): Use plain malloc instead of igc_xzalloc_ambig.
(restore_current_matrix): Use igc_free where needed.
---
 src/dispnew.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/src/dispnew.c b/src/dispnew.c
index 7bb78c3b214..4d7586bdc1e 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -304,11 +304,7 @@ free_glyph_matrix (struct glyph_matrix *matrix)
       /* Free glyph memory if MATRIX owns it.  */
       if (matrix->pool == NULL)
 	for (i = 0; i < matrix->rows_allocated; ++i)
-#ifdef HAVE_MPS
-	  igc_xfree (matrix->rows[i].glyphs[LEFT_MARGIN_AREA]);
-#else
 	  xfree (matrix->rows[i].glyphs[LEFT_MARGIN_AREA]);
-#endif
       /* Free row structures and the matrix itself.  */
       xfree (matrix->rows);
       xfree (matrix);
@@ -512,13 +508,8 @@ adjust_glyph_matrix (struct window *w, struct glyph_matrix *matrix, int x, int y
 	  while (row < end)
 	    {
 	      row->glyphs[LEFT_MARGIN_AREA] =
-#ifdef HAVE_MPS
-		igc_xnrealloc_ambig (row->glyphs[LEFT_MARGIN_AREA],
-			       dim.width, sizeof (struct glyph));
-#else
 		xnrealloc (row->glyphs[LEFT_MARGIN_AREA],
 			   dim.width, sizeof (struct glyph));
-#endif
 	      /* The mode line, if displayed, never has marginal areas.  */
 	      if ((row == matrix->rows + dim.height - 1
 		   && !(w && window_wants_mode_line (w)))
@@ -1374,11 +1365,7 @@ free_glyph_pool (struct glyph_pool *pool)
       --glyph_pool_count;
       eassert (glyph_pool_count >= 0);
 #endif
-#ifdef HAVE_MPS
-      igc_xfree (pool->glyphs);
-#else
       xfree (pool->glyphs);
-#endif
       xfree (pool);
     }
 }
@@ -1409,15 +1396,9 @@ realloc_glyph_pool (struct glyph_pool *pool, struct dim matrix_dim)
   if (needed > pool->nglyphs)
     {
       ptrdiff_t old_nglyphs = pool->nglyphs;
-#ifdef HAVE_MPS
-      pool->glyphs
-	= igc_xpalloc_ambig (pool->glyphs, &pool->nglyphs, needed - old_nglyphs,
-		       -1, sizeof *pool->glyphs);
-#else
       pool->glyphs
 	= xpalloc (pool->glyphs, &pool->nglyphs, needed - old_nglyphs,
 		   -1, sizeof *pool->glyphs);
-#endif
 
       memclear (pool->glyphs + old_nglyphs,
 		(pool->nglyphs - old_nglyphs) * sizeof *pool->glyphs);
@@ -2055,8 +2036,9 @@ restore_current_matrix (struct frame *f, struct glyph_matrix *saved)
 	  memcpy (to->glyphs[RIGHT_MARGIN_AREA],
 		  from->glyphs[RIGHT_MARGIN_AREA], nbytes);
 	  to->used[RIGHT_MARGIN_AREA] = from->used[RIGHT_MARGIN_AREA];
+
 #ifdef HAVE_MPS
-	  xfree (from->glyphs[RIGHT_MARGIN_AREA]);
+	  igc_xfree (from->glyphs[RIGHT_MARGIN_AREA]);
 #else
 	  xfree (from->glyphs[RIGHT_MARGIN_AREA]);
 #endif
-- 
2.39.2