Re: [PATCH v5] Enable xwidgets on macOS

[Eli Zaretskii]

> From: Sungbin Jo <address@hidden>
> Date: Fri, 19 Jul 2019 13:16:54 +0900
> Cc: address@hidden, address@hidden, address@hidden,
>  address@hidden, Sungbin Jo <address@hidden>
> 
> ---
>  configure.ac                     |  34 +-
>  lisp/xwidget.el                  | 326 +++++++++++++----
>  nextstep/templates/Info.plist.in |  12 +-
>  src/Makefile.in                  |   1 +
>  src/emacs.c                      |   2 +-
>  src/nsterm.m                     |  22 +-
>  src/nsxwidget.h                  |  80 ++++
>  src/nsxwidget.m                  | 611 +++++++++++++++++++++++++++++++
>  src/xwidget.c                    | 255 ++++++++++++-
>  src/xwidget.h                    |  50 ++-
>  10 files changed, 1289 insertions(+), 104 deletions(-)
>  create mode 100644 src/nsxwidget.h
>  create mode 100644 src/nsxwidget.m

Thanks for working on this.

Please include the commit log message, formatted according to
ChangeLog rules (see CONTRIBUTE for the details).  Some of the
questions below are because the log message was missing, and the
rationale for some changes was therefore not stated.

Also, I see that this changeset includes changes that are unrelated to
macOS support of xwidgets; I suggest to separate these two parts, as
the other part should be considered in its effect on all platforms.

> +(defun xwidget-webkit-split-below ()
> +  "Clone current URL into a new widget place in new window below.
> +Get the URL of current session, then browse to the URL
> +in `split-window-below' with a new xwidget webkit session."

The second sentence is unclear: what do you mean by "browse to the URL
in `split-window-below'"?  How does one "browse to" something, and
what does "in" mean when `split-window-below' is a command?

> +(defun xwidget-webkit-split-right ()
> +  "Clone current URL into a new widget place in new window right.
> +Get the URL of current session, then browse to the URL
> +in `split-window-right' with a new xwidget webkit session."

Same here.

>      (define-key map (kbd "SPC")                 'xwidget-webkit-scroll-up)
> +    (define-key map (kbd "S-SPC")               'xwidget-webkit-scroll-down)

Couldn't this new binding be a nuisance if the user holds the Shift
key for some reason?

> -    (define-key map [remap scroll-up]           'xwidget-webkit-scroll-up)
> +    (define-key map [remap scroll-up]           
> 'xwidget-webkit-scroll-up-line)
>      (define-key map [remap scroll-up-command]   'xwidget-webkit-scroll-up)
>  
> -    (define-key map [remap scroll-down]         'xwidget-webkit-scroll-down)
> +    (define-key map [remap scroll-down]         
> 'xwidget-webkit-scroll-down-line)
[...]
> -    (define-key map [remap previous-line]       'xwidget-webkit-scroll-down)
> -    (define-key map [remap next-line]           'xwidget-webkit-scroll-up)
> +    (define-key map [remap previous-line]       
> 'xwidget-webkit-scroll-down-line)
> +    (define-key map [remap next-line]           
> 'xwidget-webkit-scroll-up-line)

What is the rationale for these changes?

>      (define-key map [remap scroll-down-command] 'xwidget-webkit-scroll-down)
>  
>      (define-key map [remap forward-char]        
> 'xwidget-webkit-scroll-forward)
>      (define-key map [remap backward-char]       
> 'xwidget-webkit-scroll-backward)
>      (define-key map [remap right-char]          
> 'xwidget-webkit-scroll-forward)
>      (define-key map [remap left-char]           
> 'xwidget-webkit-scroll-backward)
>  
>      ;; (define-key map [remap move-beginning-of-line] 'image-bol)
>      ;; (define-key map [remap move-end-of-line]       'image-eol)
>      (define-key map [remap beginning-of-buffer] 'xwidget-webkit-scroll-top)
>      (define-key map [remap end-of-buffer]       
> 'xwidget-webkit-scroll-bottom)
> +    ;; For macOS xwidget webkit, we don't support multiple views for a
> +    ;; model, instead, create a new session and model behind the scene.
> +    (when (memq window-system '(mac ns))
> +      (define-key map [remap split-window-below] 'xwidget-webkit-split-below)
> +      (define-key map [remap split-window-right] 
> 'xwidget-webkit-split-right))

It could be confusing to have the same key invoke different commands
on different platforms.  So maybe it is better to simply display a
message saying multiple views aren't supported on macOS?

> +(defun xwidget-webkit-scroll-up (&optional n)
> +  "Scroll webkit up by N pixels or window height pixels.
> +Stop if the bottom edge of the page is reached.
> +If N is omitted or nil, scroll up by window height pixels."

Suggest to rename N to ARG and reword the doc string:

     "Scroll webkit up by ARG pixels; or full window height if no ARG.
   Stop if bottom of page is reached.
   Interactively, ARG is the prefix numeric argument.
   Negative ARG scrolls down."

> +(defun xwidget-webkit-scroll-down (&optional n)
> +  "Scroll webkit down by N pixels or window height pixels.
> +Stop if the top edge of the page is reached.
> +If N is omitted or nil, scroll down by window height pixels."

Same here.

> +(defcustom xwidget-webkit-scroll-line-height 50
> +  "Default line height in pixels to scroll xwidget webkit."
> +  :type 'integer)
[...]
> +(defcustom xwidget-webkit-scroll-char-width 10
> +  "Default char height in pixels to scroll xwidget webkit."
> +  :type 'integer)

Why do we need these defaults?  Can't we use the height and the width
of the default face's font instead?

> +(defun xwidget-webkit-scroll-forward (&optional n)
> +  "Scroll webkit forwards by N chars.

I suggest to say "scroll horizontally, otherwise "by N characters"
sounds surprising.

> +(defun xwidget-webkit-scroll-backward (&optional n)
> +  "Scroll webkit backwards by N chars.

Please use "back", not "backwards".

> @@ -184,7 +253,7 @@ xwidget-webkit-scroll-bottom
>    (interactive)
>    (xwidget-webkit-execute-script
>     (xwidget-webkit-current-session)
> -   "window.scrollTo(pageXOffset, window.document.body.clientHeight);"))
> +   "window.scrollTo(pageXOffset, window.document.body.scrollHeight);"))

Why this change?

> @@ -219,43 +287,141 @@ xwidget-webkit-callback
>         "error: callback called for xwidget with dead buffer")
>      (with-current-buffer (xwidget-buffer xwidget)
>        (cond ((eq xwidget-event-type 'load-changed)
> -             (xwidget-webkit-execute-script
> -              xwidget "document.title"
> -              (lambda (title)
> -                (xwidget-log "webkit finished loading: '%s'" title)
> -                ;;TODO - check the native/internal scroll
> -                ;;(xwidget-adjust-size-to-content xwidget)
> -                (xwidget-webkit-adjust-size-to-window xwidget)
> -                (rename-buffer (format "*xwidget webkit: %s *" title))))
> -             (pop-to-buffer (current-buffer)))
> +             ;; We do not change selected window for the finish of loading a 
> page.
> +             ;; And do not adjust webkit size to window here, the selected 
> window
> +             ;; can be the mini-buffer window unwantedly.
> +             (let ((title (xwidget-webkit-title xwidget)))
> +               (xwidget-log "webkit finished loading: %s" title)
> +               (rename-buffer (format "*xwidget webkit: %s *" title) t)))
>              ((eq xwidget-event-type 'decide-policy)
>               (let ((strarg  (nth 3 last-input-event)))
>                 (if (string-match ".*#\\(.*\\)" strarg)
>                     (xwidget-webkit-show-id-or-named-element
>                      xwidget
>                      (match-string 1 strarg)))))
> +            ;; TODO: Response handling other than download.
> +            ((eq xwidget-event-type 'download-callback)
> +             (let ((url  (nth 3 last-input-event))
> +                   (mime-type (nth 4 last-input-event))
> +                   (file-name (nth 5 last-input-event)))
> +               (xwidget-webkit-save-as-file url mime-type file-name)))
>              ((eq xwidget-event-type 'javascript-callback)
>               (let ((proc (nth 3 last-input-event))
>                     (arg  (nth 4 last-input-event)))
> -               (funcall proc arg)))
> +               ;; Some javascript return vector as result
> +               (funcall proc (if (vectorp arg) (seq-into arg 'list) arg))))
>              (t (xwidget-log "unhandled event:%s" xwidget-event-type))))))

Can you explain the rationale for these changes?

> +(defvar isearch-search-fun-function)
> +(when (memq window-system '(mac ns))
> +  (defcustom xwidget-webkit-enable-plugins nil
> +    "Enable plugins for xwidget webkit.
> +If non-nil, plugins are enabled.  Otherwise, disabled."
> +    :type 'boolean))

I think all defcustoms here should be platform-independent.  You can
say in the doc string that the value is only used on macOS.

>  (define-derived-mode xwidget-webkit-mode
>      special-mode "xwidget-webkit" "Xwidget webkit view mode."
>      (setq buffer-read-only t)
> +    (setq cursor-type nil)

Why this change?

> +    (setq-local isearch-search-fun-function
> +                #'xwidget-webkit-search-fun-function)
> +    (setq-local isearch-lazy-highlight nil)

And these?

> +(defcustom xwidget-webkit-download-dir "~/Downloads/"
> +  "Directory where download file saved."

"Directory where download files are saved."

> +  :type 'string)

Shouldn't this be 'file instead?

Also, whenever you add or modify a defcustom, please always supply a
:version tag.

> +(defun xwidget-webkit-save-as-file (url mime-type &optional file-name)
> +  "For XWIDGET webkit, save URL resource of MIME-TYPE as FILE-NAME."

The doc string doesn't say what file name will be used if FILE-NAME is
omitted or nil.

> +  (let ((save-name (read-file-name
> +                    (format "Save '%s' file as: " mime-type)

This prompt will be confusing.  It will, for example say

  Save app/xdiff file as:

Maybe this prompt would be slightly better:

  (format "Save URL `%s' of type `%s' in file/directory: " url mime-type)

> +    (if (file-directory-p save-name)
> +        (setq save-name
> +              (expand-file-name (file-name-nondirectory file-name) 
> save-name)))

This should be mentioned in the doc string.

> +(defcustom xwidget-webkit-bookmark-jump-new-session nil
> +  "Control bookmark jump to use new session or not.
> +If non-nil, it will use a new session.  Otherwise, it will use
> +`xwidget-webkit-last-session'.  When you set this variable to
> +nil, consider further customization with
> +`xwidget-webkit-last-session-buffer'."
> +  :type 'boolean)

The first sentence will be more informative if it said

  If non-nil, use a new xwidget webkit session after bookmark jump.

Also, a :version tag is missing.

>  (defun xwidget-webkit-bookmark-make-record ()
>    "Integrate Emacs bookmarks with the webkit xwidget."

This doc string doesn't describe what the function does.

> +;; Initialize last search text length variable when isearch starts

Please add a period at the end of this sentence.

> +(add-hook 'isearch-mode-hook
> +          (lambda ()
> +            (setq xwidget-webkit-isearch-last-length 0)))
> +
> +;; This is minimal. Regex and incremental search will be nice
                     ^^
Our convention is to have two spaces between sentences.

More generally, I wonder whether its a good idea to unconditionally
add to isearch-mode-hook just because this file was loaded.

> +(defvar xwidget-webkit-search-js "
> +var xwSearchForward = %s;
> +var xwSearchRepeat = %s;
> +var xwSearchString = '%s';
> +if (window.getSelection() && !window.getSelection().isCollapsed) {
> +  if (xwSearchRepeat) {
> +    if (xwSearchForward)
> +      window.getSelection().collapseToEnd();
> +    else
> +      window.getSelection().collapseToStart();
> +  } else {
> +    if (xwSearchForward)
> +      window.getSelection().collapseToStart();
> +    else {
> +      var sel = window.getSelection();
> +      window.getSelection().collapse(sel.focusNode, sel.focusOffset + 1);
> +    }
> +  }
> +}
> +window.find(xwSearchString, false, !xwSearchForward, true, false, true);
> +")

This defvar should have a doc string.

> +;; Utility functions, wanted in `window.el'

What do you mean by "wanted" here?

> @@ -506,23 +691,27 @@ xwidget-webkit-goto-url
>  (defun xwidget-webkit-back ()
>    "Go back in history."
>    (interactive)
> -  (xwidget-webkit-execute-script (xwidget-webkit-current-session)
> -                                 "history.go(-1);"))
> +  (xwidget-webkit-goto-history (xwidget-webkit-current-session) -1))

What is the rationale for this change?

> +(defun xwidget-webkit-forward ()
> +  "Go forward in history."

The doc string should somehow make clear that this function is about
xwidgets.  A reference to the history variable would also be nice.

>  (defun xwidget-webkit-current-url ()
> -  "Get the webkit url and place it on the kill-ring."
> +  "Get the current xwidget webkit URL."
>    (interactive)
> -  (xwidget-webkit-execute-script
> -   (xwidget-webkit-current-session)
> -   "document.URL" (lambda (rv)
> -                    (let ((url (kill-new (or rv ""))))
> -                      (message "url: %s" url)))))
> +  (xwidget-webkit-uri (xwidget-webkit-current-session)))
> +
> +(defun xwidget-webkit-current-url-message-kill ()
> +  "Display the current xwidget webkit URL and place it on the `kill-ring'."
> +  (interactive)
> +  (message "URL: %s" (kill-new (or (xwidget-webkit-current-url) ""))))

You are changing the user-visible behavior here.  Why is that a good
idea?

In any case, user-visible changes should be called out in NEWS.

> +  /* If the last rect is too large (ex, xwidget webkit), update at
> +     every move, or resizing by dragging modeline or vertical split is
> +     very hard to make its way.  */
> +  if (dragging && (r->size.width > 32 || r->size.height > 32))

Is it wise to have these values hard-coded?  What do these values
represent?

> -  /* Has movement gone beyond last rect we were tracking?  */
> -  if (x < r->origin.x || x >= r->origin.x + r->size.width
> +  /* Has movement gone beyond last rect we were tracking? */

Why did you change the comment here?

> @@ -611,24 +660,59 @@ x_draw_xwidget_glyph_string (struct glyph_string *s)
>       initialization.  */
>    struct xwidget *xww = s->xwidget;
>    struct xwidget_view *xv = xwidget_view_lookup (xww, s->w);
> +  int text_area_x, text_area_y, text_area_width, text_area_height;
>    int clip_right;
>    int clip_bottom;
>    int clip_top;
>    int clip_left;
>  
>    int x = s->x;
> -  int y = s->y + (s->height / 2) - (xww->height / 2);
> +  int y = s->y;

Why this change?

> +  /* Resize xwidget webkit if its container window size is changed in
> +     some ways, for example, a buffer became hidden in small split
> +     window, then it can appear front in merged whole window.  */
> +  if (EQ (xww->type, Qwebkit)
> +      && (xww->width != text_area_width || xww->height != text_area_height))
> +    {
> +      Lisp_Object xwl;
> +      XSETXWIDGET (xwl, xww);
> +      Fxwidget_resize (xwl,
> +                       make_int (text_area_width),
> +                       make_int (text_area_height));
> +    }

And this one?

> +DEFUN ("xwidget-webkit-uri",
> +       Fxwidget_webkit_uri, Sxwidget_webkit_uri,
> +       1, 1, 0,
> +       doc: /* Get the current URL of XWIDGET webkit.  */)
> +  (Lisp_Object xwidget)
> +{
> +  WEBKIT_FN_INIT ();
> +#ifdef USE_GTK
> +  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
> +  return build_string (webkit_web_view_get_uri (wkwv));
> +#elif defined NS_IMPL_COCOA
> +  return nsxwidget_webkit_uri (xw);
> +#endif
> +}
> +
> +DEFUN ("xwidget-webkit-title",
> +       Fxwidget_webkit_title, Sxwidget_webkit_title,
> +       1, 1, 0,
> +       doc: /* Get the current title of XWIDGET webkit.  */)
> +  (Lisp_Object xwidget)
> +{
> +  WEBKIT_FN_INIT ();
> +#ifdef USE_GTK
> +  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
> +  return build_string (webkit_web_view_get_title (wkwv));
> +#elif defined NS_IMPL_COCOA
> +  return nsxwidget_webkit_title (xw);
> +#endif

These new functions should be called out in NEWS.

> +DEFUN ("xwidget-webkit-goto-history",
> +       Fxwidget_webkit_goto_history, Sxwidget_webkit_goto_history,
> +       2, 2, 0,
> +       doc: /* Make the XWIDGET webkit load REL-POS (-1, 0, 1) page in 
> browse history.  */)
> +  (Lisp_Object xwidget, Lisp_Object rel_pos)
> +{
> +  WEBKIT_FN_INIT ();
> +  CHECK_RANGED_INTEGER (rel_pos, -1, 1); /* -1, 0, 1 */
> +#ifdef USE_GTK
> +  WebKitWebView *wkwv = WEBKIT_WEB_VIEW (xw->widget_osr);
> +  switch (XFIXNAT (rel_pos)) {
> +  case -1: webkit_web_view_go_back (wkwv); break;
> +  case 0: webkit_web_view_reload (wkwv); break;
> +  case 1: webkit_web_view_go_forward (wkwv); break;
> +  }
> +#elif defined NS_IMPL_COCOA
> +  nsxwidget_webkit_goto_history (xw, XFIXNAT (rel_pos));
> +#endif
>    return Qnil;
>  }

Likewise.

> @@ -759,11 +930,12 @@ DEFUN ("xwidget-webkit-execute-script",
>  {
>    WEBKIT_FN_INIT ();
>    CHECK_STRING (script);
> -  if (!NILP (fun) && !FUNCTIONP (fun))
> +  if (!FUNCTIONP (fun))
>      wrong_type_argument (Qinvalid_function, fun);

Why this change?

> +#elif defined NS_IMPL_COCOA
> +  return nsxwidget_get_size(XXWIDGET (xwidget));
                             ^^
Our convention is to leave one space between the name of a function
and the opening parenthesis of the argument list.

> @@ -909,14 +1098,19 @@ DEFUN ("delete-xwidget-view",
>  {
>    CHECK_XWIDGET_VIEW (xwidget_view);
>    struct xwidget_view *xv = XXWIDGET_VIEW (xwidget_view);
> -  gtk_widget_destroy (xv->widgetwindow);
>    Vxwidget_view_list = Fdelq (xwidget_view, Vxwidget_view_list);
> +#ifdef USE_GTK
> +  gtk_widget_destroy (xv->widgetwindow);

This changes the order of calls.  What is the reason for that?

Thanks again for working on this.