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

[Po Lu]

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.