GNU bug report logs - #58271
29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully

Previous Next

Full log

Message #20 received at 58271 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 조성빈 <goranmoomin <at> daum.net>, 58271 <at> debbugs.gnu.org
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Tue, 04 Oct 2022 08:34:53 +0800

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> 조성빈 <goranmoomin <at> 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 <at> 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 <at> 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 <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.