[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Handle sharing Cocoa xwidgets more gracefully

From: Po Lu
Subject: Re: [PATCH] Handle sharing Cocoa xwidgets more gracefully
Date: Tue, 04 Oct 2022 08:26:59 +0800
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.91 (gnu/linux)

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]