[PATCH] Handle sharing Cocoa xwidgets more gracefully

[조성빈 <goranmoomin@daum.net>]

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

Hello, 

Attached is a patch that improves the handling of sharing Cocoa xwidgets. 

The previous implementation of Cocoa xwidgets poorly handled cases where the
user tried to share xwidgets between multiple views (by spamming messages to
*Messages* on every redisplay, and not drawing anything on the second xwidget).
The attached patch fixes this issue by showing a warning label in the place of
 the xwidget view. 

I originally tried to create a bug report with the patch by mailing to
bug-gnu-emacs@gnu.org, but it seems that the report wasn’t created. As such,
I’m sending the patch to the emacs-devel list. How should I proceed with this?

Thanks.

Sungbin Jo


[-- Attachment #2: 0001-Handle-sharing-Cocoa-xwidgets-more-gracefully.patch --]
[-- Type: application/octet-stream, Size: 12525 bytes --]

From ea24ca5d49bb0c1d09be98ab032bbc50668ae627 Mon Sep 17 00:00:00 2001
From: VirtualBuddy <virtualbuddy@monterey-vm.local>
Date: Mon, 3 Oct 2022 17:32:45 +0900
Subject: [PATCH] Handle sharing Cocoa xwidgets more gracefully

* etc/NEWS: Mention fix.
* src/nsxwidget.h: Remove now-unused NSView subclasses and functions.
* src/nsxwidget.m:
([XwWebView mouseDown:], [XwWebView mouseUp:], [XwWebView keyDown:])
([XwWebView userContentController:didReceiveScriptMessage:]): Rename field of
xwidget_view from emacswindow to emacsFrame to better match emacs terminology.
(nsxwidget_init, nsxwidget_resize_view, nsxwidget_move_widget_in_view):
Simplify logic by removing field xwWindow and using the xvWindow as the
container.
(nsxwidget_resize, XwWindow, XvWindow): Remove now-unused code.
(nsxwidget_init_view, nsxwidget_delete_view): Handle creating non-primary
xwidget views.
(nsxwidget_show_view, nsxwidget_hide_view): Remove poor hack to hide views.
* src/xwidget.c (xwidget_init_view): Update formatting.
(x_draw_xwidget_glyph_string): Handle displaying non-primary xwidget views and
remove previous message warning.
(Fxwidget_resize): Remove useless call to nsxwidget_resize, as the subsequent
redisplay handles them via nsxwidget_resize_view.
* src/xwidget.h (struct xwidget): Remove field xwWindow and update comments
to be more accurate.
(struct xwidget_view): Add field xvWidget and rename field emacswindow to
emacsFrame to better match emacs terminology.
---
 etc/NEWS        |  8 +++++
 src/nsxwidget.h | 14 --------
 src/nsxwidget.m | 87 ++++++++++++++++++++-----------------------------
 src/xwidget.c   | 11 ++-----
 src/xwidget.h   | 15 ++++-----
 5 files changed, 53 insertions(+), 82 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index ec23f10b1f..ec85d0ba41 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4016,6 +4016,14 @@ has built-in support for displaying BMP images.
 ---
 *** 'process-attributes' is now implemented.
 
+** macOS
+---
+*** Attempts to share an xwidget is now handled more gracefully.
+Attempts to share an xwidget between multiple views now results in
+views other than the primary view displaying warnings.  If the primary
+view disappears, one of the left views becomes the primary view that
+displays the widget.
+
 \f
 ----------------------------------------------------------------------
 This file is part of GNU Emacs.
diff --git a/src/nsxwidget.h b/src/nsxwidget.h
index 666509744a..f7c744d405 100644
--- a/src/nsxwidget.h
+++ b/src/nsxwidget.h
@@ -42,26 +42,12 @@ #define NSXWIDGET_H_INCLUDED
 
 /* Functions for xwidget model.  */
 
-#ifdef __OBJC__
-@interface XwWindow : NSView
-@property struct xwidget *xw;
-@end
-#endif
-
 void nsxwidget_init (struct xwidget *xw);
 void nsxwidget_kill (struct xwidget *xw);
-void nsxwidget_resize (struct xwidget *xw);
 Lisp_Object nsxwidget_get_size (struct xwidget *xw);
 
 /* Functions for xwidget view.  */
 
-#ifdef __OBJC__
-@interface XvWindow : NSView
-@property struct xwidget *xw;
-@property struct xwidget_view *xv;
-@end
-#endif
-
 void nsxwidget_init_view (struct xwidget_view *xv,
                           struct xwidget *xww,
                           struct glyph_string *s,
diff --git a/src/nsxwidget.m b/src/nsxwidget.m
index be0eba0bcb..da4069e2f1 100644
--- a/src/nsxwidget.m
+++ b/src/nsxwidget.m
@@ -199,13 +199,13 @@ - (void)webView:(WKWebView *)webView
 
 - (void)mouseDown:(NSEvent *)event
 {
-  [self.xw->xv->emacswindow mouseDown:event];
+  [self.xw->xv->emacsFrame mouseDown:event];
   [super mouseDown:event];
 }
 
 - (void)mouseUp:(NSEvent *)event
 {
-  [self.xw->xv->emacswindow mouseUp:event];
+  [self.xw->xv->emacsFrame mouseUp:event];
   [super mouseUp:event];
 }
 
@@ -220,15 +220,15 @@ - (void)keyDown:(NSEvent *)event
   Lisp_Object val = buffer_local_value (var, Fcurrent_buffer ());
   if (!EQ (val, Qunbound) && !EQ (val, Qnil))
     {
-      [self.window makeFirstResponder:self.xw->xv->emacswindow];
-      [self.xw->xv->emacswindow keyDown:event];
+      [self.window makeFirstResponder:self.xw->xv->emacsFrame];
+      [self.xw->xv->emacsFrame keyDown:event];
       return;
     }
 
   /* Emacs handles keyboard events when javascript is blocked.  */
   if ([self.urlScriptBlocked[self.URL] boolValue])
     {
-      [self.xw->xv->emacswindow keyDown:event];
+      [self.xw->xv->emacsFrame keyDown:event];
       return;
     }
 
@@ -237,13 +237,13 @@ - (void)keyDown:(NSEvent *)event
       if (error)
         {
           NSLog (@"xwHasFocus: %@", error);
-          [self.xw->xv->emacswindow keyDown:event];
+          [self.xw->xv->emacsFrame keyDown:event];
         }
       else if (result)
         {
           NSNumber *hasFocus = result; /* __NSCFBoolean */
           if (!hasFocus.boolValue)
-            [self.xw->xv->emacswindow keyDown:event];
+            [self.xw->xv->emacsFrame keyDown:event];
           else
             [super keyDown:event];
         }
@@ -291,7 +291,7 @@ - (void)userContentController:(WKUserContentController *)userContentController
     {
       /* Just give up focus, no relay "C-g" to emacs, another "C-g"
          follows will be handled by emacs.  */
-      [self.window makeFirstResponder:self.xw->xv->emacswindow];
+      [self.window makeFirstResponder:self.xw->xv->emacsFrame];
     }
 }
 
@@ -437,16 +437,10 @@ - (void)userContentController:(WKUserContentController *)userContentController
     }];
 }
 
-/* Window containing an xwidget.  */
-
-@implementation XwWindow
-- (BOOL)isFlipped { return YES; }
-@end
-
 /* Xwidget model, macOS Cocoa part.  */
 
 void
-nsxwidget_init(struct xwidget *xw)
+nsxwidget_init (struct xwidget *xw)
 {
   block_input ();
   NSRect rect = NSMakeRect (0, 0, xw->width, xw->height);
@@ -454,9 +448,6 @@ - (BOOL)isFlipped { return YES; }
                    initWithFrame:rect
                    configuration:[[WKWebViewConfiguration alloc] init]
                          xwidget:xw];
-  xw->xwWindow = [[XwWindow alloc]
-                   initWithFrame:rect];
-  [xw->xwWindow addSubview:xw->xwWidget];
   xw->xv = NULL; /* for 1 to 1 relationship of webkit2.  */
   unblock_input ();
 }
@@ -481,22 +472,10 @@ - (BOOL)isFlipped { return YES; }
       [((XwWebView *) xw->xwWidget).urlScriptBlocked release];
       [xw->xwWidget removeFromSuperviewWithoutNeedingDisplay];
       [xw->xwWidget release];
-      [xw->xwWindow removeFromSuperviewWithoutNeedingDisplay];
-      [xw->xwWindow release];
       xw->xwWidget = nil;
     }
 }
 
-void
-nsxwidget_resize (struct xwidget *xw)
-{
-  if (xw->xwWidget)
-    {
-      [xw->xwWindow setFrameSize:NSMakeSize(xw->width, xw->height)];
-      [xw->xwWidget setFrameSize:NSMakeSize(xw->width, xw->height)];
-    }
-}
-
 Lisp_Object
 nsxwidget_get_size (struct xwidget *xw)
 {
@@ -504,12 +483,6 @@ - (BOOL)isFlipped { return YES; }
                  xw->xwWidget.frame.size.height);
 }
 
-/* Xwidget view, macOS Cocoa part.  */
-
-@implementation XvWindow : NSView
-- (BOOL)isFlipped { return YES; }
-@end
-
 void
 nsxwidget_init_view (struct xwidget_view *xv,
                      struct xwidget *xw,
@@ -526,16 +499,27 @@ - (BOOL)isFlipped { return YES; }
   xv->clip_top = 0;
   xv->clip_bottom = xw->height;
 
-  xv->xvWindow = [[XvWindow alloc]
+  xv->xvWindow = [[NSView alloc]
                    initWithFrame:NSMakeRect (x, y, xw->width, xw->height)];
-  xv->xvWindow.xw = xw;
-  xv->xvWindow.xv = xv;
 
-  xw->xv = xv; /* For 1 to 1 relationship of webkit2.  */
-  [xv->xvWindow addSubview:xw->xwWindow];
+  if (!xw->xv)
+    {
+      xw->xv = xv; /* For 1 to 1 relationship of webkit2.  */
+      /* This seems like it should be retained, but tests show otherwise.  */
+      xv->xvWidget = xw->xwWidget;
+    }
+  else
+    {
+      NSTextField *warningLabel = [NSTextField labelWithString:@"Cocoa Xwidgets do not support sharing widgets."];
+      [warningLabel setFrameSize:NSMakeSize (xw->width, xw->height)];
+      warningLabel.drawsBackground = YES;
+      warningLabel.backgroundColor = [NSColor textBackgroundColor];
+      xv->xvWidget = warningLabel;
+    }
 
-  xv->emacswindow = FRAME_NS_VIEW (s->f);
-  [xv->emacswindow addSubview:xv->xvWindow];
+  [xv->xvWindow addSubview:xv->xvWidget];
+  xv->emacsFrame = FRAME_NS_VIEW (s->f);
+  [xv->emacsFrame addSubview:xv->xvWindow];
 }
 
 void
@@ -544,8 +528,10 @@ - (BOOL)isFlipped { return YES; }
   if (!EQ (xv->model, Qnil))
     {
       struct xwidget *xw = XXWIDGET (xv->model);
-      [xw->xwWindow removeFromSuperviewWithoutNeedingDisplay];
-      xw->xv = NULL; /* Now model has no view.  */
+      if (xw->xv == xv)
+        {
+          xw->xv = NULL; /* Now model has no view.  */
+        }
     }
   [xv->xvWindow removeFromSuperviewWithoutNeedingDisplay];
   [xv->xvWindow release];
@@ -555,21 +541,21 @@ - (BOOL)isFlipped { return YES; }
 nsxwidget_show_view (struct xwidget_view *xv)
 {
   xv->hidden = NO;
-  [xv->xvWindow setFrameOrigin:NSMakePoint(xv->x + xv->clip_left,
-                                           xv->y + xv->clip_top)];
+  [xv->xvWindow setHidden:NO];
 }
 
 void
 nsxwidget_hide_view (struct xwidget_view *xv)
 {
   xv->hidden = YES;
-  [xv->xvWindow setFrameOrigin:NSMakePoint(10000, 10000)];
+  [xv->xvWindow setHidden:YES];
 }
 
 void
 nsxwidget_resize_view (struct xwidget_view *xv, int width, int height)
 {
-  [xv->xvWindow setFrameSize:NSMakeSize(width, height)];
+  [xv->xvWindow setFrameSize:NSMakeSize (width, height)];
+  [xv->xvWidget setFrameSize:NSMakeSize (width, height)];
 }
 
 void
@@ -582,8 +568,7 @@ - (BOOL)isFlipped { return YES; }
 void
 nsxwidget_move_widget_in_view (struct xwidget_view *xv, int x, int y)
 {
-  struct xwidget *xww = xv->xvWindow.xw;
-  [xww->xwWindow setFrameOrigin:NSMakePoint (x, y)];
+  [xv->xvWidget setFrameOrigin:NSMakePoint (x, y)];
 }
 
 void
diff --git a/src/xwidget.c b/src/xwidget.c
index 8bdfab02fd..013b773a90 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -2762,7 +2762,7 @@ xwidget_init_view (struct xwidget *xww,
   xv->just_resized = false;
 #elif defined NS_IMPL_COCOA
   nsxwidget_init_view (xv, xww, s, x, y);
-  nsxwidget_resize_view(xv, xww->width, xww->height);
+  nsxwidget_resize_view (xv, xww->width, xww->height);
 #endif
 
   return xv;
@@ -2794,7 +2794,7 @@ x_draw_xwidget_glyph_string (struct glyph_string *s)
 
   xv->just_resized = false;
 #elif defined NS_IMPL_COCOA
-  if (!xv)
+  if (!xv || xww->xv != xv)
     {
       /* Enforce 1 to 1, model and view for macOS Cocoa webkit2.  */
       if (xww->xv)
@@ -2805,11 +2805,6 @@ x_draw_xwidget_glyph_string (struct glyph_string *s)
               XSETXWIDGET_VIEW (xvl, xww->xv);
               Fdelete_xwidget_view (xvl);
             }
-          else
-            {
-              message ("You can't share an xwidget (webkit2) among windows.");
-              return;
-            }
         }
       xv = xwidget_init_view (xww, s, x, y);
     }
@@ -3243,8 +3238,6 @@ DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
 
       gtk_widget_queue_allocate (GTK_WIDGET (xw->widget_osr));
     }
-#elif defined NS_IMPL_COCOA
-  nsxwidget_resize (xw);
 #endif
   unblock_input ();
 
diff --git a/src/xwidget.h b/src/xwidget.h
index 502beb6765..3943148e3f 100644
--- a/src/xwidget.h
+++ b/src/xwidget.h
@@ -81,16 +81,13 @@ #define XWIDGET_H_INCLUDED
   guint hit_result;
 #elif defined (NS_IMPL_COCOA)
 # ifdef __OBJC__
-  /* For offscreen widgets, unused if not osr.  */
+  /* The primary widget.  */
   NSView *xwWidget;
-  XwWindow *xwWindow;
 
-  /* Used only for xwidget types (such as webkit2) enforcing 1 to 1
-     relationship between model and view.  */
+  /* The current view that displays the primary widget, if exists.  */
   struct xwidget_view *xv;
 # else
   void *xwWidget;
-  void *xwWindow;
   struct xwidget_view *xv;
 # endif
 #endif
@@ -136,11 +133,13 @@ #define XWIDGET_H_INCLUDED
   int just_resized;
 #elif defined (NS_IMPL_COCOA)
 # ifdef __OBJC__
-  XvWindow *xvWindow;
-  NSView *emacswindow;
+  NSView *xvWindow;
+  NSView *xvWidget;
+  NSView *emacsFrame;
 # else
   void *xvWindow;
-  void *emacswindow;
+  void *xvWidget;
+  void *emacsFrame;
 # endif
 #endif
 
-- 
2.37.0 (Apple Git-136)