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

Previous Next

To reply to this bug, email your comments to 58271 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

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

From: 조성빈 <goranmoomin <at> daum.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully
Date: Mon, 3 Oct 2022 18:32:20 +0900

[Message part 1 (text/plain, inline)]

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.

[0001-Handle-sharing-Cocoa-xwidgets-more-gracefully.patch (application/octet-stream, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: 조성빈 <goranmoomin <at> daum.net>
Cc: 58271 <at> debbugs.gnu.org, Po Lu <luangruo <at> yahoo.com>
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Mon, 03 Oct 2022 20:00:15 +0200

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

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

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

2022. 10. 4. 오전 3:00, Lars Ingebrigtsen <larsi <at> gnus.org> 작성:
> 
> 조성빈 <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 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.

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

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

goranmoomin <at> 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.)

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

From: Sungbin Jo <goranmoomin <at> daum.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 58271 <at> debbugs.gnu.org, Po Lu <luangruo <at> yahoo.com>
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Tue, 4 Oct 2022 09:01:14 +0900

[Message part 1 (text/plain, inline)]

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

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

[0001-Handle-sharing-Cocoa-xwidgets-more-gracefully.patch (application/octet-stream, attachment)]

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.

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

From: Sungbin Jo <goranmoomin <at> daum.net>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 58271 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Tue, 4 Oct 2022 11:03:28 +0900

[Message part 1 (text/plain, inline)]

2022. 10. 4. 오전 9:34, Po Lu <luangruo <at> 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 <at> 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.

[0001-Handle-sharing-Cocoa-xwidgets-more-gracefully.patch (application/octet-stream, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, goranmoomin <at> daum.net
Cc: 58271 <at> debbugs.gnu.org, Po Lu <luangruo <at> yahoo.com>
Subject: Re: bug#58271: 29.0.50;
 [PATCH] Handle sharing Cocoa xwidgets more gracefully
Date: Tue, 4 Oct 2022 06:42:12 +0200

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

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: goranmoomin <at> daum.net, 58271 <at> debbugs.gnu.org, luangruo <at> yahoo.com
Subject: Re: bug#58271: 29.0.50;
 [PATCH] Handle sharing Cocoa xwidgets more gracefully
Date: Tue, 04 Oct 2022 09:48:16 +0300

> Cc: 58271 <at> debbugs.gnu.org, Po Lu <luangruo <at> yahoo.com>
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Tue, 04 Oct 2022 01:35:38 +0200
> 
> goranmoomin <at> 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 <at> 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

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

From: Sungbin Jo <goranmoomin <at> daum.net>
To: 58271 <at> debbugs.gnu.org
Cc: Po Lu <luangruo <at> yahoo.com>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Eli Zaretskii <eliz <at> gnu.org>, Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Tue, 4 Oct 2022 16:25:11 +0900

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

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

Done.

2022. 10. 4. 오후 1:42, Stefan Kangas <stefankangas <at> 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.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Sungbin Jo <goranmoomin <at> daum.net>
Cc: 58271 <at> debbugs.gnu.org, Po Lu <luangruo <at> yahoo.com>,
 Lars Ingebrigtsen <larsi <at> gnus.org>, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#58271: 29.0.50;
 [PATCH] Handle sharing Cocoa xwidgets more gracefully
Date: Fri, 11 Nov 2022 05:27:48 -0800

Sungbin Jo <goranmoomin <at> daum.net> writes:

> 2022. 10. 4. 오후 3:48, Eli Zaretskii <eliz <at> gnu.org> 작성:
>
>> I agree, but would you please inform copyright-clerk <at> fsf.org of your
>> new email address?
>
> Done.
>
> 2022. 10. 4. 오후 1:42, Stefan Kangas <stefankangas <at> 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.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Sungbin Jo <goranmoomin <at> daum.net>
Cc: Po Lu <luangruo <at> yahoo.com>, 58271 <at> debbugs.gnu.org,
 Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#58271: 29.0.50;
 [PATCH] Handle sharing Cocoa xwidgets more gracefully
Date: Thu, 24 Nov 2022 17:26:20 -0800

Sungbin Jo <goranmoomin <at> daum.net> writes:

> 2022. 10. 4. 오전 9:34, Po Lu <luangruo <at> 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.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Sungbin Jo <goranmoomin <at> daum.net>, 58271 <at> debbugs.gnu.org,
 Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Fri, 25 Nov 2022 10:55:00 +0800

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Sungbin Jo <goranmoomin <at> daum.net> writes:
>
>> 2022. 10. 4. 오전 9:34, Po Lu <luangruo <at> 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.

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

From: 조성빈 <goranmoomin <at> daum.net>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 58271 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Sat, 26 Nov 2022 14:31:54 +0900

> 2022. 11. 25. 오전 11:55, Po Lu <luangruo <at> yahoo.com> 작성:
> 
> Stefan Kangas <stefankangas <at> 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.

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

From: Po Lu <luangruo <at> yahoo.com>
To: 조성빈 <goranmoomin <at> daum.net>
Cc: 58271 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more
 gracefully
Date: Sat, 26 Nov 2022 13:54:12 +0800

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

>> 2022. 11. 25. 오전 11:55, Po Lu <luangruo <at> yahoo.com> 작성:
>> 
>> Stefan Kangas <stefankangas <at> 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.

Previous Next

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