Re: [PATCH v5] Enable xwidgets on macOS

[조성빈]

> 2019. 7. 20. 오후 5:21, Eli Zaretskii <address@hidden> 작성:
> 
>> 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.

Ok, I’ll include the log message when committing.

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

AFAIU, I should split these changes into two-or-more commits. Am I correct?

>> +(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?

These functions used to be binded on split-window-{below,right}.
It used to create a new xwidget webkit session as the usual 
split-window-{below,right}
behavior is to display the same buffer, but xwidget webkit in macOS Cocoa 
doesn’t support
simultaneously displaying it in the same time.
These functions’ names changes to `xwidget-webkit-clone-and-split-below' in the 
next patch.

>> +(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?

This binding (S-SPC) exists in at least in macOS Safari & FF.
I think if one uses SPC to scroll up the view, they will expect S-SPC to scroll 
down.

>> -    (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?

The functions are now configurable.
The ones without the -line postfix gets an interactive argument of pixel values.
The ones with them gets an interactive argument of line numbers, and the value 
of
line height in pixel is a customizable variable.

>>     (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?

Reverted.

>> +(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.”

Ok.

>> +(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.

Ok.

>> +(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?

Removed defcustom and changed related functions to use `window-font-height’ and
`window-font-width'.

>> +(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.

Ok.

>> +(defun xwidget-webkit-scroll-backward (&optional n)
>> +  "Scroll webkit backwards by N chars.
> 
> Please use "back", not "backwards”.

Ok.

>> @@ -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?

It’s a fix to scroll to the bottom of the page.
The previous one has edge cases, e.g. when there are horizontal scroll bars.

>> @@ -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?

Hmm… The first hunk looks like it’s using the new function 
`xwidget-webkit-title'.
The call to `xwidget-webkit-adjust-size-to-window' was removed as the webkit can
finish reloading at any time, and if the mini-buffer is focused at the point,
the call misplaces the xwidget buffer.

The second hunk is for handling download-callback events in xwidget. 
The event generator function is there, but it is only called in Cocoa Webkit.
It’s not implemented in the GTK one, but it wouldn’t matter running.

I reverted the third hunk and related 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.

Should I change it to a defvar? Or should I remove this variable at all?

>> (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?

I vaguely remember both were because of implementing website isearch.
I remember isearch working months ago… but it doesn’t now. :-(
I reverted the related changes.
I’ll take a look at them other time.

>> +(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?

Ok.

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

Ok.

>> +(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.

While fixing the doc string, I found out FILE-NAME shouldn’t be an optional 
argument :-(
Fixed.

>> +  (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)

Ok.

>> +    (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.

Ok.

>> +(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.

Ok.

>> (defun xwidget-webkit-bookmark-make-record ()
>>   "Integrate Emacs bookmarks with the webkit xwidget."
> 
> This doc string doesn't describe what the function does.

Ok.

>> +;; 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.

Removed all isearch-related code.

>> +(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?

Removed the mention of `window.el'.

>> @@ -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?

It’s using the newly defined function in C `xwidget-webkit-goto-history'.

>> +(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.

Changed doc string, but I’m not sure about what the history variable you’re 
referencing is.

>> (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.

IMHO, the new name is more clearer.
However, I reverted to the previous behavior.

>> +  /* 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?

Sorry, I’m not sure about how these values are selected.
Looks like it’s just heuristics, but I’m not sure.

>> -  /* 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?

Reverted.

>> @@ -611,24 +660,59 @@ when there are  (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?

I’m not sure why xwidgets should be top-aligned, it looks like it’s due to
personal preference of the original patch author.
Reverted.

>> +  /* 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?

This is due to the removement of the call `xwidget-webkit-adjust-size-to-window'
explained above.

>> +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.

Ok.

>> +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?

That was an mistake. Reverted.

>> +#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.

Fixed.

>> @@ -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?

Reverted.

> Thanks again for working on this.