[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5] Enable xwidgets on macOS
From: |
조성빈 |
Subject: |
Re: [PATCH v5] Enable xwidgets on macOS |
Date: |
Wed, 24 Jul 2019 03:36:45 +0900 |
> 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.
- [PATCH v4] Enable xwidgets on macOS, Sungbin Jo, 2019/07/18
- [PATCH v5] Enable xwidgets on macOS, Sungbin Jo, 2019/07/19
- Re: [PATCH v5] Enable xwidgets on macOS, Eli Zaretskii, 2019/07/27
- Re: [PATCH v5] Enable xwidgets on macOS, 조성빈, 2019/07/27
- Re: [PATCH v5] Enable xwidgets on macOS, Eli Zaretskii, 2019/07/27
- Re: [PATCH v5] Enable xwidgets on macOS, 조성빈, 2019/07/29
- Re: [PATCH v5] Enable xwidgets on macOS, Alan Third, 2019/07/29
- Re: [PATCH v5] Enable xwidgets on macOS, Stefan Monnier, 2019/07/29