GNU bug report logs - #60703
Patches to xwidget code

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60703 in the body.
You can then email your comments to 60703 AT debbugs.gnu.org in the normal way.

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

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: emacs-devel <at> gnu.org, bug-gnu-emacs <at> gnu.org
Subject: Patches to xwidget code
Date: Tue, 10 Jan 2023 00:14:28 -0500

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

Hello everyone and thanks for all your work!

I have some fixes to the xwidget code that I'd like to contribute.

These should achieve three things
1) fix `xwidget-webkit-current-url' in xwidget.el so that it actually
displays the current URL
2) fix the Objective-C code in nsxwidget.m to eliminate memory leaks
3) implement the function `xwidget-webkit-estimated-load-progress' in
Objective-C, making this functionality available for macOS as well

Regarding 2), I have tested my changes with the Instruments app within the
XCode dev tools. I was able to test them on two different machines: a
MacBook Air M2 running macOS Ventura 13, and an Intel processor running
macOS Big Sur 11.6.7.
When testing with Instruments, it appears that Emacs isn't leaking memory
anymore. I am still seeing some leaks, but according to Instruments, the
responsible libraries are system libraries, and not Emacs itself.
If possible, I would appreciate it if other people can test/profile these
changes as well, and share their feedback.

Assuming the changes are correct and accepted, would it be possible to
include them in the upcoming Emacs 29.1 release?
Technically, none of these are new features; it's just fixes to existing
features.

I intend to keep working on xwidget support for macOS (fixing remaining
issues and implementing missing functionalities), but I'm not sure when
I'll be able to get to it; for now I'd like to start by contributing these
first major fixes.

Thanks in advance for your feedback; let me know if you need anything else
from me.

Best,
Andrew

[Message part 2 (text/html, inline)]

[xwidget-patches.patch (application/octet-stream, attachment)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: 60703 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Tue, 10 Jan 2023 17:59:04 +0800

Andrew De Angelis <bobodeangelis <at> gmail.com> writes:

> Hello everyone and thanks for all your work!
>
> I have some fixes to the xwidget code that I'd like to contribute.
>
> These should achieve three things 1) fix `xwidget-webkit-current-url'
> in xwidget.el so that it actually displays the current URL 2) fix the
> Objective-C code in nsxwidget.m to eliminate memory leaks 3) implement
> the function `xwidget-webkit-estimated-load-progress' in Objective-C,
> making this functionality available for macOS as well
>
> Regarding 2), I have tested my changes with the Instruments app within
> the XCode dev tools. I was able to test them on two different
> machines: a MacBook Air M2 running macOS Ventura 13, and an Intel
> processor running macOS Big Sur 11.6.7.  When testing with
> Instruments, it appears that Emacs isn't leaking memory anymore. I am
> still seeing some leaks, but according to Instruments, the responsible
> libraries are system libraries, and not Emacs itself.  If possible, I
> would appreciate it if other people can test/profile these changes as
> well, and share their feedback.
>
> Assuming the changes are correct and accepted, would it be possible to
> include them in the upcoming Emacs 29.1 release?  Technically, none of
> these are new features; it's just fixes to existing features.
>
> I intend to keep working on xwidget support for macOS (fixing
> remaining issues and implementing missing functionalities), but I'm
> not sure when I'll be able to get to it; for now I'd like to start by
> contributing these first major fixes.
>
> Thanks in advance for your feedback; let me know if you need anything
> else from me.

Andrew, thanks for working on Emacs.

>
> diff --git a/lisp/xwidget.el b/lisp/xwidget.el
> index abbda29081..8095fa9db5 100644
> --- a/lisp/xwidget.el
> +++ b/lisp/xwidget.el
> @@ -924,8 +924,9 @@ xwidget-webkit-reload
>  (defun xwidget-webkit-current-url ()
>    "Display the current xwidget webkit URL and place it on the `kill-ring'."
>    (interactive nil xwidget-webkit-mode)
> -  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
> -    (message "URL: %s" (kill-new (or url "")))))
> +  (let ((url (or (xwidget-webkit-uri (xwidget-webkit-current-session)) "")))
> +    (kill-new url)
> +    (message "URL: %s" url)))
>  
>  (defun xwidget-webkit-browse-history ()
>    "Display a buffer containing the history of page loads."

This change is fine for Emacs 29.1.  Eli, WDYT?

>  @implementation XwWebView : WKWebView
>  
> +- (void)dealloc
> +{
> +  [super dealloc];
> +}

Thanks.  Please put a space between the part of the Objective-C method
that looks like a cast (I don't know what it's called, sorry) and the
name of the method.

But, is this really necessary?  Without this being defined, the runtime
will just call the super method, correct?

>  - (id)initWithFrame:(CGRect)frame
>        configuration:(WKWebViewConfiguration *)configuration
>              xwidget:(struct xwidget *)xw
>  {
>    /* Script controller to add script message handler and user script.  */
> -  WKUserContentController *scriptor = [[WKUserContentController alloc] init];
> +  WKUserContentController *scriptor = [[[WKUserContentController alloc] init]
> +                                        autorelease];
>    configuration.userContentController = scriptor;
>  
>    /* Enable inspect element context menu item for debugging.  */
> @@ -81,7 +87,8 @@ - (id)initWithFrame:(CGRect)frame
>    if (self)
>      {
>        self.xw = xw;
> -      self.urlScriptBlocked = [[NSMutableDictionary alloc] init];
> +      self.urlScriptBlocked = [[[NSMutableDictionary alloc] init]
> +                                autorelease];
>        self.navigationDelegate = self;
>        self.UIDelegate = self;
>        self.customUserAgent =
> @@ -89,11 +96,13 @@ - (id)initWithFrame:(CGRect)frame
>          @" AppleWebKit/603.3.8 (KHTML, like Gecko)"
>          @" Version/11.0.1 Safari/603.3.8";
>        [scriptor addScriptMessageHandler:self name:@"keyDown"];
> -      [scriptor addUserScript:[[WKUserScript alloc]
> -                                initWithSource:xwScript
> -                                 injectionTime:
> -                                  WKUserScriptInjectionTimeAtDocumentStart
> -                                forMainFrameOnly:NO]];
> +      WKUserScript *userScript = [[[WKUserScript alloc]
> +                                    initWithSource:xwScript
> +                                     injectionTime:
> +                                      WKUserScriptInjectionTimeAtDocumentStart
> +                                    forMainFrameOnly:NO]
> +                                   autorelease];
> +      [scriptor addUserScript:userScript];
>      }
>    return self;
>  }
> @@ -102,7 +111,27 @@ - (void)webView:(WKWebView *)webView
>  didFinishNavigation:(WKNavigation *)navigation
>  {
>    if (EQ (Fbuffer_live_p (self.xw->buffer), Qt))
> -    store_xwidget_event_string (self.xw, "load-changed", "");
> +    store_xwidget_event_string (self.xw, "load-changed", "load-finished");
> +}
> +
> +- (void)webView:(WKWebView *)webView didStartProvisionalNavigation:(WKNavigation *)navigation
> +{
> +  if (EQ (Fbuffer_live_p (self.xw->buffer), Qt))
> +    store_xwidget_event_string (self.xw, "load-changed", "load-started");
> +}
> +
> +- (void)webView:(WKWebView *)webView
> +didReceiveServerRedirectForProvisionalNavigation:(WKNavigation *)navigation
> +{
> +  if (EQ (Fbuffer_live_p (self.xw->buffer), Qt))
> +    store_xwidget_event_string (self.xw, "load-changed", "load-redirected");
> +}
> +
> +// Start loading WKWebView
> +- (void)webView:(WKWebView *)webView didCommitNavigation:(WKNavigation *)navigation
> +{
> +  if (EQ (Fbuffer_live_p (self.xw->buffer), Qt)) // what exactly is this test for
> +    store_xwidget_event_string (self.xw, "load-changed", "load-committed");
>  }

These are also fine for Emacs 29.  However, please replace the C++-style
comments with C style ones, add spaces after the cast-like parts in the
method definition, and fill everything to column 80.  I believe the test
is that the xwidget's buffer has not been killed, and could easily be
written:

  !NILP (Fbuffer_live_p (self.xw->buffer))

as well.

>  - (void)webView:(WKWebView *)webView
> @@ -343,6 +372,20 @@ - (void)userContentController:(WKUserContentController *)userContentController
>    }
>  }
>  
> +double
> +nsxwidget_webkit_estimated_load_progress(struct xwidget *xw)
> +{
> +  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
> +  return xwWebView.estimatedProgress;
> +}

Please place a space between "nsxwidget_webkit_estimated_load_progress"
and the parameter list.

> +void
> +nsxwidget_webkit_stop_loading (struct xwidget *xw)
> +{
> +  XwWebView *xwWebView = (XwWebView *) xw->xwWidget;
> +  [xwWebView stopLoading];
> +}
> +
>  void
>  nsxwidget_webkit_zoom (struct xwidget *xw, double zoom_change)
>  {
> @@ -452,7 +495,8 @@ - (BOOL)isFlipped { return YES; }
>    NSRect rect = NSMakeRect (0, 0, xw->width, xw->height);
>    xw->xwWidget = [[XwWebView alloc]
>                     initWithFrame:rect
> -                   configuration:[[WKWebViewConfiguration alloc] init]
> +                   configuration:[[[WKWebViewConfiguration alloc] init]
> +                                   autorelease]
>                           xwidget:xw];
>    xw->xwWindow = [[XwWindow alloc]
>                     initWithFrame:rect];
> @@ -470,16 +514,18 @@ - (BOOL)isFlipped { return YES; }
>          ((XwWebView *) xw->xwWidget).configuration.userContentController;
>        [scriptor removeAllUserScripts];
>        [scriptor removeScriptMessageHandlerForName:@"keyDown"];
> -      [scriptor release];
> +
>        if (xw->xv)
>          xw->xv->model = Qnil; /* Make sure related view stale.  */
>  
>        /* This stops playing audio when a xwidget-webkit buffer is
> -         killed.  I could not find other solution.  */
> +         killed.  I could not find other solution.
> +         TODO: improve this */
>        nsxwidget_webkit_goto_uri (xw, "about:blank");
>  
>        [((XwWebView *) xw->xwWidget).urlScriptBlocked release];
>        [xw->xwWidget removeFromSuperviewWithoutNeedingDisplay];
> +
>        [xw->xwWidget release];
>        [xw->xwWindow removeFromSuperviewWithoutNeedingDisplay];
>        [xw->xwWindow release];

These changes are also fine for Emacs 29, but the TODO seems excessive.
Eli, any comments here?

> diff --git a/src/xwidget.c b/src/xwidget.c
> index efe2705562..b9550da460 100644
> --- a/src/xwidget.c
> +++ b/src/xwidget.c
> @@ -54,6 +54,7 @@ Copyright (C) 2011-2023 Free Software Foundation, Inc.
>  
>  #include <math.h>
>  
> +

Please undo this extraneous whitespace change.

>  
> +DEFUN ("xwidget-webkit-estimated-load-progress",
> +       Fxwidget_webkit_estimated_load_progress, Sxwidget_webkit_estimated_load_progress,
> +       1, 1, 0, doc: /* Get the estimated load progress of XWIDGET, a WebKit widget.
> +Return a value ranging from 0.0 to 1.0, based on how close XWIDGET
> +is to completely loading its page.  */)
> +  (Lisp_Object xwidget)
> +{
> +  struct xwidget *xw;
> +#ifdef USE_GTK
> +  WebKitWebView *webview;
> +#endif
> +  double value;
> +
> +  CHECK_LIVE_XWIDGET (xwidget);
> +  xw = XXWIDGET (xwidget);
> +  CHECK_WEBKIT_WIDGET (xw);
> +
> +  block_input ();
> +#ifdef USE_GTK
> +  webview = WEBKIT_WEB_VIEW (xw->widget_osr);
> +  value = webkit_web_view_get_estimated_load_progress (webview);
> +#elif defined NS_IMPL_COCOA
> +  value = nsxwidget_webkit_estimated_load_progress (xw);
> +#endif
> +
> +  unblock_input ();
> +
> +  return make_float (value);
> +}
> +
>  DEFUN ("xwidget-webkit-goto-uri",
>         Fxwidget_webkit_goto_uri, Sxwidget_webkit_goto_uri,
>         2, 2, 0,
> @@ -3810,28 +3841,6 @@ DEFUN ("xwidget-webkit-back-forward-list", Fxwidget_webkit_back_forward_list,
>    return list3 (back, here, forward);
>  }
>  
> -DEFUN ("xwidget-webkit-estimated-load-progress",
> -       Fxwidget_webkit_estimated_load_progress, Sxwidget_webkit_estimated_load_progress,
> -       1, 1, 0, doc: /* Get the estimated load progress of XWIDGET, a WebKit widget.
> -Return a value ranging from 0.0 to 1.0, based on how close XWIDGET
> -is to completely loading its page.  */)
> -  (Lisp_Object xwidget)
> -{
> -  struct xwidget *xw;
> -  WebKitWebView *webview;
> -  double value;
> -
> -  CHECK_LIVE_XWIDGET (xwidget);
> -  xw = XXWIDGET (xwidget);
> -  CHECK_WEBKIT_WIDGET (xw);
> -
> -  block_input ();
> -  webview = WEBKIT_WEB_VIEW (xw->widget_osr);
> -  value = webkit_web_view_get_estimated_load_progress (webview);
> -  unblock_input ();
> -
> -  return make_float (value);
> -}
>  #endif
>  
>  DEFUN ("xwidget-webkit-set-cookie-storage-file",
> @@ -3874,19 +3883,23 @@ DEFUN ("xwidget-webkit-stop-loading", Fxwidget_webkit_stop_loading,
>  XWIDGET as part of loading a page.  */)
>    (Lisp_Object xwidget)
>  {
> -#ifdef USE_GTK
>    struct xwidget *xw;
> +#ifdef USE_GTK
>    WebKitWebView *webview;
> +#endif
>  
>    CHECK_LIVE_XWIDGET (xwidget);
>    xw = XXWIDGET (xwidget);
>    CHECK_WEBKIT_WIDGET (xw);
>  
>    block_input ();
> +#ifdef USE_GTK
>    webview = WEBKIT_WEB_VIEW (xw->widget_osr);
>    webkit_web_view_stop_loading (webview);
> -  unblock_input ();
> +#elif defined NS_IMPL_COCOA
> +  nsxwidget_webkit_stop_loading (xw);
>  #endif
> +  unblock_input ();
>  
>    return Qnil;
>  }
> @@ -3936,8 +3949,9 @@ syms_of_xwidget (void)
>  #ifdef USE_GTK
>    defsubr (&Sxwidget_webkit_load_html);
>    defsubr (&Sxwidget_webkit_back_forward_list);
> -  defsubr (&Sxwidget_webkit_estimated_load_progress);
>  #endif
> +
> +  defsubr (&Sxwidget_webkit_estimated_load_progress);
>    defsubr (&Skill_xwidget);
>  
>    DEFSYM (QCxwidget, ":xwidget");

None of this seems to affect anything other than the NS build, so it's
fine by me.

All in all, all your changes are fine for Emacs 29 by me.  What they
need is a proper commit message.  See the node "Style of Change Logs" in
the GNU Coding Standards for examples of how to do this:

  https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs

Thanks again for working on Emacs.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: bobodeangelis <at> gmail.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Tue, 10 Jan 2023 15:40:44 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: emacs-devel <at> gnu.org,  60703 <at> debbugs.gnu.org
> Date: Tue, 10 Jan 2023 17:59:04 +0800
> 
> > --- a/lisp/xwidget.el
> > +++ b/lisp/xwidget.el
> > @@ -924,8 +924,9 @@ xwidget-webkit-reload
> >  (defun xwidget-webkit-current-url ()
> >    "Display the current xwidget webkit URL and place it on the `kill-ring'."
> >    (interactive nil xwidget-webkit-mode)
> > -  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
> > -    (message "URL: %s" (kill-new (or url "")))))
> > +  (let ((url (or (xwidget-webkit-uri (xwidget-webkit-current-session)) "")))
> > +    (kill-new url)
> > +    (message "URL: %s" url)))
> >  
> >  (defun xwidget-webkit-browse-history ()
> >    "Display a buffer containing the history of page loads."
> 
> This change is fine for Emacs 29.1.  Eli, WDYT?

Fine by me.

> These changes are also fine for Emacs 29, but the TODO seems excessive.
> Eli, any comments here?

I don't have an opinion, so if it's OK in your opinion, I'm fine with
adding this.  Are we sure this will not affect any important aspects
of the build, except where we already have problems?  (I don't see any
description of the problems and their solution in the commit log or
the comments.)

> All in all, all your changes are fine for Emacs 29 by me.  What they
> need is a proper commit message.  See the node "Style of Change Logs" in
> the GNU Coding Standards for examples of how to do this:
> 
>   https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs

I don't see copyright assignment for Andrew, so this should be settled
before accepting the contribution of this magnitude.

> Thanks again for working on Emacs.

Seconded.

P.S. Please don't cross-post to emacs-devel.

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Po Lu <luangruo <at> yahoo.com>, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Tue, 10 Jan 2023 18:33:25 -0500

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

Thank you both.

@Po Lu <luangruo <at> yahoo.com>, I will work on the changes you mentioned.
Can you clarify the C++-style comments vs C style ones? When should we use
"/* */" and when should we use "//"?
I looked around the code and it doesn't seem very consistent, but I can
help clean up the xwidget-related files.

@Eli,

> Are we sure this will not affect any important aspects
> of the build, except where we already have problems?  (I don't see any
> description of the problems and their solution in the commit log or
> the comments.)
>
This should only affect builds on Mac machines, and makes it so the xwidget
feature doesn't leak memory anymore. Importantly, I do not have access to
any Linux machine, so I can't be 100% sure they're not affected. But as far
as I can tell, the changes are limited to Mac builds using the
"-with-xwidgets" feature.
I'll add a better explanation in the upcoming Change Log, but essentially,
the problems were simply memory leaks. The leaked memory wasn't a lot, so
it seldom caused noticeable issues (at least in my experience).

I'm sending my answers to the copyright questionnaire to assign <at> gnu.org.
This is my first contribution, so let me know if I'm missing anything.

Best,
Andrew



On Tue, Jan 10, 2023 at 8:40 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Po Lu <luangruo <at> yahoo.com>
> > Cc: emacs-devel <at> gnu.org,  60703 <at> debbugs.gnu.org
> > Date: Tue, 10 Jan 2023 17:59:04 +0800
> >
> > > --- a/lisp/xwidget.el
> > > +++ b/lisp/xwidget.el
> > > @@ -924,8 +924,9 @@ xwidget-webkit-reload
> > >  (defun xwidget-webkit-current-url ()
> > >    "Display the current xwidget webkit URL and place it on the
> `kill-ring'."
> > >    (interactive nil xwidget-webkit-mode)
> > > -  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
> > > -    (message "URL: %s" (kill-new (or url "")))))
> > > +  (let ((url (or (xwidget-webkit-uri
> (xwidget-webkit-current-session)) "")))
> > > +    (kill-new url)
> > > +    (message "URL: %s" url)))
> > >
> > >  (defun xwidget-webkit-browse-history ()
> > >    "Display a buffer containing the history of page loads."
> >
> > This change is fine for Emacs 29.1.  Eli, WDYT?
>
> Fine by me.
>
> > These changes are also fine for Emacs 29, but the TODO seems excessive.
> > Eli, any comments here?
>
> I don't have an opinion, so if it's OK in your opinion, I'm fine with
> adding this.  Are we sure this will not affect any important aspects
> of the build, except where we already have problems?  (I don't see any
> description of the problems and their solution in the commit log or
> the comments.)
>
> > All in all, all your changes are fine for Emacs 29 by me.  What they
> > need is a proper commit message.  See the node "Style of Change Logs" in
> > the GNU Coding Standards for examples of how to do this:
> >
> >   https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs
>
> I don't see copyright assignment for Andrew, so this should be settled
> before accepting the contribution of this magnitude.
>
> > Thanks again for working on Emacs.
>
> Seconded.
>
> P.S. Please don't cross-post to emacs-devel.
>

[Message part 2 (text/html, inline)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: bobodeangelis <at> gmail.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Wed, 11 Jan 2023 09:15:25 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> I don't have an opinion, so if it's OK in your opinion, I'm fine with
> adding this.  Are we sure this will not affect any important aspects
> of the build, except where we already have problems?  (I don't see any
> description of the problems and their solution in the commit log or
> the comments.)

Yes, these are fixes to a piece of code that is normally off by default,
and did not work prior to those changes.

> I don't see copyright assignment for Andrew, so this should be settled
> before accepting the contribution of this magnitude.

Right, thanks for bringing that up.

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Sat, 21 Jan 2023 01:13:01 -0500

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

Thank you again for the feedback. Attached is the updated patch.
@Po Lu <luangruo <at> yahoo.com>, you were right about the dealloc method: I
removed it since it was unnecessary.
I think I fixed all the issues you pointed out, but let me know if there's
any other changes needed.

About the copyright assignment, I sent the attached email to assign <at> gnu.org
ten days ago, but haven't gotten a reply yet. Just want to make sure I'm
going about it the right way: do I need to do anything else to get this
done? It's my first contribution so I may be missing something.


On Tue, Jan 10, 2023 at 8:15 PM Po Lu <luangruo <at> yahoo.com> wrote:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > I don't have an opinion, so if it's OK in your opinion, I'm fine with
> > adding this.  Are we sure this will not affect any important aspects
> > of the build, except where we already have problems?  (I don't see any
> > description of the problems and their solution in the commit log or
> > the comments.)
>
> Yes, these are fixes to a piece of code that is normally off by default,
> and did not work prior to those changes.
>
> > I don't see copyright assignment for Andrew, so this should be settled
> > before accepting the contribution of this magnitude.
>
> Right, thanks for bringing that up.
>

[Message part 2 (text/html, inline)]

[Andrew John De Angelis.eml (message/rfc822, attachment)]

[xwidget-patches-1.patch (application/octet-stream, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Sat, 21 Jan 2023 09:16:39 +0200

> From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> Date: Sat, 21 Jan 2023 01:13:01 -0500
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
> 
> About the copyright assignment, I sent the attached email to assign <at> gnu.org ten days ago, but haven't
> gotten a reply yet.

Are you sure?  I have in my email archives a response to your email
from copyright-clerk <at> fsf.org that was sent on Dec 28, with the PDF of
your assignment attached; you were supposed to sign it and email the
scan of the signed form to them.  Not sure why you sent another
request on Jan 10.  What am I missing here?

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Sat, 21 Jan 2023 10:32:19 -0500

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

Sorry about the confusion!
Looks like I missed that reply.
Some quick clarification though: the first assignment I sent out was for
some modifications to shell.el (bug#60385; see also the thread "New
Package: sticky-shell" in emacs-devel: I had a package idea and it was
mentioned to add it as a feature instead). I then 1) missed the reply from
copyright-clerk <at> fsf.org, 2) assumed I'd need an additional copyright
assignment for the xwidget-patch, as it concerns different files.
Is the first assignment good enough for all subsequent files I change? If
that's the case I'll go ahead and sign it and send it back asap. Just
wanted to make sure I'm following the correct procedure.

Thanks for walking me through this.

On Sat, Jan 21, 2023 at 2:16 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> > Date: Sat, 21 Jan 2023 01:13:01 -0500
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
> >
> > About the copyright assignment, I sent the attached email to
> assign <at> gnu.org ten days ago, but haven't
> > gotten a reply yet.
>
> Are you sure?  I have in my email archives a response to your email
> from copyright-clerk <at> fsf.org that was sent on Dec 28, with the PDF of
> your assignment attached; you were supposed to sign it and email the
> scan of the signed form to them.  Not sure why you sent another
> request on Jan 10.  What am I missing here?
>

[Message part 2 (text/html, inline)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Sat, 21 Jan 2023 17:41:15 +0200

> From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> Date: Sat, 21 Jan 2023 10:32:19 -0500
> Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
> 
> Sorry about the confusion!
> Looks like I missed that reply.
> Some quick clarification though: the first assignment I sent out was for some modifications to shell.el
> (bug#60385; see also the thread "New Package: sticky-shell" in emacs-devel: I had a package idea and it
> was mentioned to add it as a feature instead). I then 1) missed the reply from copyright-clerk <at> fsf.org, 2)
> assumed I'd need an additional copyright assignment for the xwidget-patch, as it concerns different files.
> Is the first assignment good enough for all subsequent files I change? If that's the case I'll go ahead and sign
> it and send it back asap. Just wanted to make sure I'm following the correct procedure.

The assignment will be good for all your future contributions to
Emacs, but first you need to do what the copyright clerk asked you to
do: print the PDF form, sign it, scan it, and email it back.  Then
you'll get another email saying that your paperwork is complete, and
then we can accept contributions from you.

Thanks.

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Thu, 26 Jan 2023 18:45:33 -0500

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

Thanks for the explanation. I sent out the signed copy yesterday.
I noticed a couple things about the previous patch, so I'm sending this one
with a few updates:

   - `xwidget-webkit-current-url` in lisp/xwidget.el: check if the url
   variable is non-nil before calling `kill-new' on it. This avoids killing an
   empty string, which would be pointless. We still alert the user that
   something's wrong by messaging "URL: nil" (although getting a nil url seems
   very unlikely).
   - in src/nsxwidget.m: formatting (keep lines below 80 char), and a brief
   comment describing the purpose of some of the newly added functions


I'm also attaching a draft of the ChangeLog. Let me know if you'd like me
to make any changes there.

I do have a question about the X11/GTK implementations for xwidget. I'm not
sure I understand the relationship between the two: from the preprocessor
macros it seems at times they are separate and at times that one is an
addendum to the other.
I'm asking because I want to make sure I'm using the preprocessor macros
correctly: specifically, the function
`xwidget-webkit-estimated-load-progress` within xwidget.c used to be inside
a "#ifdef USE_GTK" block. I moved it outside of the block, and separated
the GTK implementation from the NS_IMPL_COCOA implementation. Is this a
problem for the X11 build? Should I instead put the whole function within a
block along these lines: "#if defined USE_GTK || defined NS_IMPL_COCOA" ?


On Sat, Jan 21, 2023 at 10:41 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> > Date: Sat, 21 Jan 2023 10:32:19 -0500
> > Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
> >
> > Sorry about the confusion!
> > Looks like I missed that reply.
> > Some quick clarification though: the first assignment I sent out was for
> some modifications to shell.el
> > (bug#60385; see also the thread "New Package: sticky-shell" in
> emacs-devel: I had a package idea and it
> > was mentioned to add it as a feature instead). I then 1) missed the
> reply from copyright-clerk <at> fsf.org, 2)
> > assumed I'd need an additional copyright assignment for the
> xwidget-patch, as it concerns different files.
> > Is the first assignment good enough for all subsequent files I change?
> If that's the case I'll go ahead and sign
> > it and send it back asap. Just wanted to make sure I'm following the
> correct procedure.
>
> The assignment will be good for all your future contributions to
> Emacs, but first you need to do what the copyright clerk asked you to
> do: print the PDF form, sign it, scan it, and email it back.  Then
> you'll get another email saying that your paperwork is complete, and
> then we can accept contributions from you.
>
> Thanks.
>

[Message part 2 (text/html, inline)]

[ChangeLog.andrewda (application/octet-stream, attachment)]

[xwidget-patches-2.patch (application/octet-stream, attachment)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Fri, 27 Jan 2023 08:44:20 +0800

Andrew De Angelis <bobodeangelis <at> gmail.com> writes:

> Thanks for the explanation. I sent out the signed copy yesterday. 
> I noticed a couple things about the previous patch, so I'm sending this one with a few updates:
>
> * `xwidget-webkit-current-url` in lisp/xwidget.el: check if the url variable is non-nil before calling `kill-new' on it. This avoids killing an empty
>  string, which would be pointless. We still alert the user that something's wrong by messaging "URL: nil" (although getting a nil url seems
>  very unlikely).
> * in src/nsxwidget.m: formatting (keep lines below 80 char), and a brief comment describing the purpose of some of the newly added
>  functions
>
> I'm also attaching a draft of the ChangeLog. Let me know if you'd like me to make any changes there. 
>
> I do have a question about the X11/GTK implementations for xwidget. I'm not sure I understand the relationship between the two: from the
> preprocessor macros it seems at times they are separate and at times that one is an addendum to the other.
> I'm asking because I want to make sure I'm using the preprocessor macros correctly: specifically, the function
> `xwidget-webkit-estimated-load-progress` within xwidget.c used to be inside a "#ifdef USE_GTK" block. I moved it outside of the block, and
> separated the GTK implementation from the NS_IMPL_COCOA implementation. Is this a problem for the X11 build? Should I instead put the
> whole function within a block along these lines: "#if defined USE_GTK || defined NS_IMPL_COCOA" ?

No, that's not a problem.  The X11 implementation shares most of the
widget manipulation code with the PGTK one, so both are simply under
``#ifdef USE_GTK''.

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Sun, 19 Feb 2023 16:00:44 -0500

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

Hi all, just wanted to check on this: will you be able to include my
changes in Emacs-29?
Don't mean to put pressure or anything, just wondering if there's any
additional things you need from me, and if there's a timeline on when the
changes will be in.

Thanks!

On Thu, Jan 26, 2023 at 7:44 PM Po Lu <luangruo <at> yahoo.com> wrote:

> Andrew De Angelis <bobodeangelis <at> gmail.com> writes:
>
> > Thanks for the explanation. I sent out the signed copy yesterday.
> > I noticed a couple things about the previous patch, so I'm sending this
> one with a few updates:
> >
> > * `xwidget-webkit-current-url` in lisp/xwidget.el: check if the url
> variable is non-nil before calling `kill-new' on it. This avoids killing an
> empty
> >  string, which would be pointless. We still alert the user that
> something's wrong by messaging "URL: nil" (although getting a nil url seems
> >  very unlikely).
> > * in src/nsxwidget.m: formatting (keep lines below 80 char), and a brief
> comment describing the purpose of some of the newly added
> >  functions
> >
> > I'm also attaching a draft of the ChangeLog. Let me know if you'd like
> me to make any changes there.
> >
> > I do have a question about the X11/GTK implementations for xwidget. I'm
> not sure I understand the relationship between the two: from the
> > preprocessor macros it seems at times they are separate and at times
> that one is an addendum to the other.
> > I'm asking because I want to make sure I'm using the preprocessor macros
> correctly: specifically, the function
> > `xwidget-webkit-estimated-load-progress` within xwidget.c used to be
> inside a "#ifdef USE_GTK" block. I moved it outside of the block, and
> > separated the GTK implementation from the NS_IMPL_COCOA implementation.
> Is this a problem for the X11 build? Should I instead put the
> > whole function within a block along these lines: "#if defined USE_GTK ||
> defined NS_IMPL_COCOA" ?
>
> No, that's not a problem.  The X11 implementation shares most of the
> widget manipulation code with the PGTK one, so both are simply under
> ``#ifdef USE_GTK''.
>

[Message part 2 (text/html, inline)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Mon, 20 Feb 2023 10:41:41 +0800

Andrew De Angelis <bobodeangelis <at> gmail.com> writes:

> Hi all, just wanted to check on this: will you be able to include my changes in Emacs-29?
> Don't mean to put pressure or anything, just wondering if there's any additional things you need from me, and if there's a timeline on when
> the changes will be in.

I think this is up to Eli, but as I said earlier, your changes LGTM.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Mon, 20 Feb 2023 13:47:37 +0200

> From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> Date: Sun, 19 Feb 2023 16:00:44 -0500
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
> 
> Hi all, just wanted to check on this: will you be able to include my changes in Emacs-29?
> Don't mean to put pressure or anything, just wondering if there's any additional things you need from me,
> and if there's a timeline on when the changes will be in.

Please post the patches rebased on the current emacs-29 branch.

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Tue, 21 Feb 2023 00:24:06 -0500

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

Done. (git diff forked/emacs-29 > emacs-29-xwidget-patches.patch)

Let me know if there's anything I should do. Thanks!

Best,
Andrew

On Mon, Feb 20, 2023 at 6:47 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> > Date: Sun, 19 Feb 2023 16:00:44 -0500
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703 <at> debbugs.gnu.org
> >
> > Hi all, just wanted to check on this: will you be able to include my
> changes in Emacs-29?
> > Don't mean to put pressure or anything, just wondering if there's any
> additional things you need from me,
> > and if there's a timeline on when the changes will be in.
>
> Please post the patches rebased on the current emacs-29 branch.
>

[Message part 2 (text/html, inline)]

[emacs-29-xwidget-patches.patch (application/octet-stream, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Wed, 22 Feb 2023 15:27:00 +0200

> From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> Date: Tue, 21 Feb 2023 00:24:06 -0500
> Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
> 
> Done. (git diff forked/emacs-29 > emacs-29-xwidget-patches.patch)
> 
> Let me know if there's anything I should do. Thanks!

Thanks, what is missing now is the commit log message.  Please see
CONTRIBUTE and the examples in the Git repository, for how we format
the log messages.

One minor comment to the code:

> +DEFUN ("xwidget-webkit-estimated-load-progress",
> +       Fxwidget_webkit_estimated_load_progress, Sxwidget_webkit_estimated_load_progress,
> +       1, 1, 0, doc: /* Get the estimated load progress of XWIDGET, a WebKit widget.
> +Return a value ranging from 0.0 to 1.0, based on how close XWIDGET
> +is to completely loading its page.  */)

The first line of a doc string should be a single complete sentence.
This is because commands like "M-x apropos" show only the first line.

(Yes, I'm aware that you just moved existing code, but still: let's
fix this while we are at that.)

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

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Thu, 23 Feb 2023 22:47:41 -0500

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

Let me know if this ChangeLog is fine or you'd like me to make any changes.

The first line of a doc string should be a single complete sentence.
>
I'm not sure I follow: in this case, isn't  this the first line: "Get the
estimated load progress of XWIDGET, a WebKit widget."? It renders fine when
I run `M-x apropos xwidget-webkit-estimated-load-progress`. But happy to
make any additional changes if needed.



On Wed, Feb 22, 2023 at 8:26 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> > Date: Tue, 21 Feb 2023 00:24:06 -0500
> > Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
> >
> > Done. (git diff forked/emacs-29 > emacs-29-xwidget-patches.patch)
> >
> > Let me know if there's anything I should do. Thanks!
>
> Thanks, what is missing now is the commit log message.  Please see
> CONTRIBUTE and the examples in the Git repository, for how we format
> the log messages.
>
> One minor comment to the code:
>
> > +DEFUN ("xwidget-webkit-estimated-load-progress",
> > +       Fxwidget_webkit_estimated_load_progress,
> Sxwidget_webkit_estimated_load_progress,
> > +       1, 1, 0, doc: /* Get the estimated load progress of XWIDGET, a
> WebKit widget.
> > +Return a value ranging from 0.0 to 1.0, based on how close XWIDGET
> > +is to completely loading its page.  */)
>
> The first line of a doc string should be a single complete sentence.
> This is because commands like "M-x apropos" show only the first line.
>
> (Yes, I'm aware that you just moved existing code, but still: let's
> fix this while we are at that.)
>

[Message part 2 (text/html, inline)]

[ChangeLog.andrewda (application/octet-stream, attachment)]

Message #58 received at 60703-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: luangruo <at> yahoo.com, 60703-done <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Thu, 02 Mar 2023 12:57:54 +0200

> From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> Date: Thu, 23 Feb 2023 22:47:41 -0500
> Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
> 
> Let me know if this ChangeLog is fine or you'd like me to make any changes.

Thanks, installed on the emacs-29 branch, and closing the bug.

>  The first line of a doc string should be a single complete sentence.
> 
> I'm not sure I follow: in this case, isn't  this the first line: "Get the estimated load progress of XWIDGET, a
> WebKit widget."? It renders fine when I run `M-x apropos xwidget-webkit-estimated-load-progress`. But
> happy to make any additional changes if needed.

Please ignore that part: I was confused.

Please in the future always accompany the changes with a
ChangeLog-style commit log message, and mention the bug number there
if you know it.  Also, use of "git format-patch" is encouraged.  These
measures make the job of installing your changes much easier.

Message #61 received at 60703-done <at> debbugs.gnu.org (full text, mbox):

From: Andrew De Angelis <bobodeangelis <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 60703-done <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Sat, 4 Mar 2023 18:00:03 -0500

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

Awesome, thank you so much! Will keep all your suggestions in mind for my
future contributions.

Small question: what are the criteria to be added to `etc/AUTHORS`? Just
wondering if/how my contribution could be mentioned there.

On Thu, Mar 2, 2023 at 5:58 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Andrew De Angelis <bobodeangelis <at> gmail.com>
> > Date: Thu, 23 Feb 2023 22:47:41 -0500
> > Cc: luangruo <at> yahoo.com, 60703 <at> debbugs.gnu.org
> >
> > Let me know if this ChangeLog is fine or you'd like me to make any
> changes.
>
> Thanks, installed on the emacs-29 branch, and closing the bug.
>
> >  The first line of a doc string should be a single complete sentence.
> >
> > I'm not sure I follow: in this case, isn't  this the first line: "Get
> the estimated load progress of XWIDGET, a
> > WebKit widget."? It renders fine when I run `M-x apropos
> xwidget-webkit-estimated-load-progress`. But
> > happy to make any additional changes if needed.
>
> Please ignore that part: I was confused.
>
> Please in the future always accompany the changes with a
> ChangeLog-style commit log message, and mention the bug number there
> if you know it.  Also, use of "git format-patch" is encouraged.  These
> measures make the job of installing your changes much easier.
>

[Message part 2 (text/html, inline)]

Message #64 received at 60703-done <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Andrew De Angelis <bobodeangelis <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 60703-done <at> debbugs.gnu.org
Subject: Re: bug#60703: Patches to xwidget code
Date: Sun, 05 Mar 2023 08:07:17 +0800

Andrew De Angelis <bobodeangelis <at> gmail.com> writes:

> Awesome, thank you so much! Will keep all your suggestions in mind for my future contributions.
>
> Small question: what are the criteria to be added to `etc/AUTHORS`? Just wondering if/how my contribution could be mentioned there.

AUTHORS is generated automatically prior to each release of Emacs, so
you don't have to do anything aside from waiting for Emacs 29.1 to be
released.

Thanks.

Previous Next

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