bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[조성빈]

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

Severity: normal
Tags: patch

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.


[-- 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)

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Lars Ingebrigtsen]

조성빈 <goranmoomin@daum.net> writes:

> The attached patch fixes this issue by showing a warning label in the place of
> the xwidget view.

Makes sense to me -- perhaps Po Lu has some comments; added to the CCs.

In any case, it looks like this patch is too large to apply without
getting a copyright assignment for the FSF first.  Would you be willing
to sign such paperwork?

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[goranmoomin]

2022. 10. 4. 오전 3:00, Lars Ingebrigtsen <larsi@gnus.org> 작성:
> 
> 조성빈 <goranmoomin@daum.net> writes:
> 
>> The attached patch fixes this issue by showing a warning label in the place of
>> the xwidget view.
> 
> Makes sense to me -- perhaps Po Lu has some comments; added to the CCs.
> 
> In any case, it looks like this patch is too large to apply without
> getting a copyright assignment for the FSF first.  Would you be willing
> to sign such paper?

I’ve signed the paper, though I’ve used a different email address before. (I’m
Sungbin Jo <pcr910303 <at> icloud.com> who contributed the original Cocoa
xwidget patch in bug#29565.)

Would changing my email address require going through the process again?

Thanks.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Lars Ingebrigtsen]

goranmoomin@daum.net writes:

> I’ve signed the paper, though I’ve used a different email address before. (I’m
> Sungbin Jo <pcr910303 <at> icloud.com> who contributed the original Cocoa
> xwidget patch in bug#29565.)
>
> Would changing my email address require going through the process again?

No, that's not necessary, but it would be convenient if the patch
carried the name "Sungbin Jo" somewhere.  (That's not really necessary,
either, so long as we know who's who, but it would be convenient.)

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Sungbin Jo]

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

2022. 10. 4. 오전 8:35, Lars Ingebrigtsen <larsi@gnus.org> 작성:

> goranmoomin@daum.net writes:
> 
>> I’ve signed the paper, though I’ve used a different email address
>>  before. (I’m
>> Sungbin Jo <pcr910303 <at> icloud.com> who contributed the original
>>  Cocoa
>> xwidget patch in bug#29565.)
>> 
>> Would changing my email address require going through the process again?
> 
> No, that's not necessary, but it would be convenient if the patch
> carried the name "Sungbin Jo" somewhere.  (That's not really necessary,
> either, so long as we know who's who, but it would be convenient.)

Ah, that’s definitely a mistake (was testing and committing inside an
 unconfigured virtual machine).

Attached is an updated patch with proper author details, rebased onto
commit 6a5169e747.

Thanks.


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

From 1ac9922a1720ac4e3899b8f245056f9a287e87c3 Mon Sep 17 00:00:00 2001
From: Sungbin Jo <goranmoomin@daum.net>
Date: Tue, 4 Oct 2022 08:51:33 +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 f21acaf207..a6cd7da341 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4059,6 +4059,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)

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> 조성빈 <goranmoomin@daum.net> writes:
>
>> The attached patch fixes this issue by showing a warning label in the place of
>> the xwidget view.
>
> Makes sense to me -- perhaps Po Lu has some comments; added to the CCs.
>
> In any case, it looks like this patch is too large to apply without
> getting a copyright assignment for the FSF first.  Would you be willing
> to sign such paperwork?

Thanks.  I saw the post to emacs-devel first, and replied without
checking the bug tracker.  These were my comments:

조성빈 <goranmoomin@daum.net> writes:

> 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.

Thanks; since you seem to be the original author of the xwidget code on
Mac OS, could you please fix the crash there when an xwidget is deleted
but remains on-screen?

Some minor formatting comments on the patch below:

> From: VirtualBuddy <virtualbuddy@monterey-vm.local>

Is this a real email?

> * etc/NEWS: Mention fix.

Since this is a bug fix, I see no reason to mention it in NEWS.  In
addition, even if it warranted a mention, it ought to be placed under
"Changes in Emacs 29.1 on Non-Free Operating Systems".

> * 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.

Please make sure that each line of the commit message is no longer than
64 characters in length.

>  {
> -  [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];
>  }

The "emacswindow" field should actually be named "frame", IMHO.

> +    {
> +      NSTextField *warningLabel = [NSTextField labelWithString:@"Cocoa Xwidgets do not support sharing widgets."];

This line is too long.  Please find a way to make it fit in 80 columns.

> +      if (xw->xv == xv)
> +        {
> +          xw->xv = NULL; /* Now model has no view.  */
> +        }

Please remove the unnecessary braces here.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Sungbin Jo]

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

2022. 10. 4. 오전 9:34, Po Lu <luangruo@yahoo.com> 작성:

> Thanks; since you seem to be the original author of the xwidget code on
> Mac OS, could you please fix the crash there when an xwidget is deleted
> but remains on-screen?

I’ve continued on this (with some questions) on emacs-devel.

> Some minor formatting comments on the patch below:

Updated patch attached.

>> From: VirtualBuddy <virtualbuddy@monterey-vm.local>
> 
> Is this a real email?

No, it was a mistake. Updated.

>> * etc/NEWS: Mention fix.
> 
> Since this is a bug fix, I see no reason to mention it in NEWS.  In
> addition, even if it warranted a mention, it ought to be placed under
> "Changes in Emacs 29.1 on Non-Free Operating Systems".

I did place it under the “Non-Free Operating Systems” part, but I removed them.

>> * 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.
> 
> Please make sure that each line of the commit message is no longer than
> 64 characters in length.

Done.

>> {
>> -  [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];
>> }
> 
> The "emacswindow" field should actually be named "frame", IMHO.

I’m not sure I follow; that’s exactly the rename I’ve done. Or are you
suggesting that I should use ‘frame’ instead of ‘emacsFrame’? In any
case, I’m fine with either.

>> +    {
>> +      NSTextField *warningLabel = [NSTextField labelWithString:@"Cocoa Xwidgets do not support sharing widgets."];
> 
> This line is too long.  Please find a way to make it fit in 80 columns.

Done.

>> +      if (xw->xv == xv)
>> +        {
>> +          xw->xv = NULL; /* Now model has no view.  */
>> +        }
> 
> Please remove the unnecessary braces here.

Done.


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

From 2f90ceb0d2e556cad2802cdca5ef8324a8dc5523 Mon Sep 17 00:00:00 2001
From: Sungbin Jo <goranmoomin@daum.net>
Date: Tue, 4 Oct 2022 08:51:33 +0900
Subject: [PATCH] Handle sharing Cocoa xwidgets more gracefully

* src/nsxwidget.h (nsxwidget_resize, XwWindow, XvWindow): Remove
unused 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 unused subclasses
and functions.
(nsxwidget_init_view, nsxwidget_delete_view): Handle creating
non-primary xwidget views.
(nsxwidget_show_view, nsxwidget_hide_view): Remove 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.
---
 src/nsxwidget.h | 14 --------
 src/nsxwidget.m | 86 ++++++++++++++++++++-----------------------------
 src/xwidget.c   | 11 ++-----
 src/xwidget.h   | 15 ++++-----
 4 files changed, 44 insertions(+), 82 deletions(-)

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..2a6d5cdb54 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,28 @@ - (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
+    {
+      NSString *warningString = @"Cocoa Xwidgets do not support sharing widgets.";
+      NSTextField *warningLabel = [NSTextField labelWithString:warningString];
+      [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 +529,8 @@ - (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 +540,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 +567,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)

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> goranmoomin@daum.net writes:
>
>> I’ve signed the paper, though I’ve used a different email address before. (I’m
>> Sungbin Jo <pcr910303 <at> icloud.com> who contributed the original Cocoa
>> xwidget patch in bug#29565.)
>>
>> Would changing my email address require going through the process again?
>
> No, that's not necessary, but it would be convenient if the patch
> carried the name "Sungbin Jo" somewhere.  (That's not really necessary,
> either, so long as we know who's who, but it would be convenient.)

You could also consider adding an entry to the .mailmap file.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Eli Zaretskii]

> Cc: 58271@debbugs.gnu.org, Po Lu <luangruo@yahoo.com>
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 04 Oct 2022 01:35:38 +0200
> 
> goranmoomin@daum.net writes:
> 
> > I’ve signed the paper, though I’ve used a different email address before. (I’m
> > Sungbin Jo <pcr910303 <at> icloud.com> who contributed the original Cocoa
> > xwidget patch in bug#29565.)
> >
> > Would changing my email address require going through the process again?
> 
> No, that's not necessary, but it would be convenient if the patch
> carried the name "Sungbin Jo" somewhere.  (That's not really necessary,
> either, so long as we know who's who, but it would be convenient.)

I agree, but would you please inform copyright-clerk@fsf.org of your
new email address?  That doesn't require anything else from you, but
they will update their records, and people who look you up in the list
will have fewer doubts.

TIA

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Sungbin Jo]

2022. 10. 4. 오후 3:48, Eli Zaretskii <eliz@gnu.org> 작성:

> I agree, but would you please inform copyright-clerk@fsf.org of your
> new email address?

Done.

2022. 10. 4. 오후 1:42, Stefan Kangas <stefankangas@gmail.com> 작성:

> You could also consider adding an entry to the .mailmap file.


Would it be acceptable for me to add the entry on this patch? If that’s
the case, I’d like to do on my next patch.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Stefan Kangas]

Sungbin Jo <goranmoomin@daum.net> writes:

> 2022. 10. 4. 오후 3:48, Eli Zaretskii <eliz@gnu.org> 작성:
>
>> I agree, but would you please inform copyright-clerk@fsf.org of your
>> new email address?
>
> Done.
>
> 2022. 10. 4. 오후 1:42, Stefan Kangas <stefankangas@gmail.com> 작성:
>
>> You could also consider adding an entry to the .mailmap file.
>
> Would it be acceptable for me to add the entry on this patch? If that’s
> the case, I’d like to do on my next patch.

(Sorry; it seems that I missed replying to this part.)

I think it's better to separate out the .mailmap change, so feel free to
send a patch for that.  Thanks.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Stefan Kangas]

Sungbin Jo <goranmoomin@daum.net> writes:

> 2022. 10. 4. 오전 9:34, Po Lu <luangruo@yahoo.com> 작성:
>
>> Thanks; since you seem to be the original author of the xwidget code on
>> Mac OS, could you please fix the crash there when an xwidget is deleted
>> but remains on-screen?
>
> I’ve continued on this (with some questions) on emacs-devel.
>
>> Some minor formatting comments on the patch below:
>
> Updated patch attached.

Po Lu, is this patch good to go?  It seems like your comments were all
addressed?  Thanks in advance.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

> Sungbin Jo <goranmoomin@daum.net> writes:
>
>> 2022. 10. 4. 오전 9:34, Po Lu <luangruo@yahoo.com> 작성:
>>
>>> Thanks; since you seem to be the original author of the xwidget code on
>>> Mac OS, could you please fix the crash there when an xwidget is deleted
>>> but remains on-screen?
>>
>> I’ve continued on this (with some questions) on emacs-devel.
>>
>>> Some minor formatting comments on the patch below:
>>
>> Updated patch attached.
>
> Po Lu, is this patch good to go?  It seems like your comments were all
> addressed?  Thanks in advance.

IIRC we agreed that it is rather pointless to install this change when
there are still easily encountered fatal crashes in the NS xwidget code,
and that Sunbin Jo was working on fixing those crashes.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[조성빈]

> 2022. 11. 25. 오전 11:55, Po Lu <luangruo@yahoo.com> 작성:
> 
> Stefan Kangas <stefankangas@gmail.com> writes:
> 
>> Po Lu, is this patch good to go?  It seems like your comments were all
>> addressed?  Thanks in advance.
> 
> IIRC we agreed that it is rather pointless to install this change when
> there are still easily encountered fatal crashes in the NS xwidget code,
> and that Sungbin Jo was working on fixing those crashes.

Sorry for the silence, having a bit busy time right now.

I do have a WIP patch that fixes some more of the mentioned crashes, but it’s
not patch-quality; I couldn’t find the time to clean it up it.
I’ll open a new bug for it if I get to finish that (hopefully sometime soon);
for now, would it be fine if this gets applied first?

Thanks.

bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

조성빈 <goranmoomin@daum.net> writes:

>> 2022. 11. 25. 오전 11:55, Po Lu <luangruo@yahoo.com> 작성:
>> 
>> Stefan Kangas <stefankangas@gmail.com> writes:
>> 
>>> Po Lu, is this patch good to go?  It seems like your comments were all
>>> addressed?  Thanks in advance.
>> 
>> IIRC we agreed that it is rather pointless to install this change when
>> there are still easily encountered fatal crashes in the NS xwidget code,
>> and that Sungbin Jo was working on fixing those crashes.
>
> Sorry for the silence, having a bit busy time right now.
>
> I do have a WIP patch that fixes some more of the mentioned crashes, but it’s
> not patch-quality; I couldn’t find the time to clean it up it.
> I’ll open a new bug for it if I get to finish that (hopefully sometime soon);
> for now, would it be fine if this gets applied first?

I'd rather not see Emacs 29 be released with half of the necessary
changes to some niche code included, so no, but if you really insist I
won't object.