emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa


From: Stefan Monnier
Subject: Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa
Date: Mon, 03 Jun 2019 22:40:01 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux)

I know nothing about programming on MacOS systems, nor about GUI widgets
(and even less about "x"widgets), and only very little about GUI
programming, so I only really looked at the Elisp part.

IOW, nothing really important, but that's all I can offer.

>  OPTION_DEFAULT_OFF([xwidgets],
> -  [enable use of some gtk widgets in Emacs buffers (requires gtk3)])
> +  [enable use of some gtk widgets in Emacs buffers (requires gtk3 or macOS 
> Cocoa)])

Under MacOS these aren't "gtk widgets" right?

>  (declare-function xwidget-buffer "xwidget.c" (xwidget))
>  (declare-function xwidget-size-request "xwidget.c" (xwidget))
>  (declare-function xwidget-resize "xwidget.c" (xwidget new-width new-height))
> +;; @callback - Prefer defun to lambda, not to be garbage collected
> +;; before its execution in `xwidget-webkit-callback'.
>  (declare-function xwidget-webkit-execute-script "xwidget.c"
>                    (xwidget script &optional callback))

I don't understand this comment.  It seems to hint at a GC bug, maybe
because the callback is not registered via `staticpro` or something like that?

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

You don't need a terminating \ to continue strings on the next line.
But the first line of a docstring should be less than 80 chars long and
should make sense on its own (sometimes we only show the first line of
a docstring).

E.g. the first line could be something like:

    "Clone current URL into a new widget place in new window below

> +    ;; 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 (kbd "C-x 2") 'xwidget-webkit-cx2)
> +      (define-key map (kbd "C-x 3") 'xwidget-webkit-cx3))

Rather than rebind C-x 2 and C-x 3, I think you should use remapping, as in:

    (define-key map [remap split-window-below] 'xwidget-webkit-cx2)
    (define-key map [remap split-window-right] 'xwidget-webkit-cx3)

Also the function names should probably refer to "below" and "right"
rather than to "cx2" and "cx3": the names should describe what the
function does rather than where it's expected to be bound.

> +   (cond ((null n)
> +          (format "window.scrollBy(0, %d);"
> +                  (xwidget-window-inside-pixel-height (selected-window))))
> +         (t (format "window.scrollBy(0, %d);" n)))))

Aka

    (format "window.scrollBy(0, %d);"
            (or n (xwidget-window-inside-pixel-height (selected-window))))

> +(defvar xwidget-webkit-scroll-line-height 50
> +  "Default line height in pixels for scroll xwidget webkit.")

Specifying such a distance in pixels is probably not a good idea in these
days where DPI can vary from 70 to 400.  Maybe you should use a multiple
of the default face's height or something like that?

> @@ -192,7 +252,7 @@ xwidget-webkit-scroll-bottom
>  (define-key (current-global-map) [xwidget-event] #'xwidget-event-handler)
>  (defun xwidget-log (&rest msg)
>    "Log MSG to a buffer."
> -  (let ((buf (get-buffer-create " *xwidget-log*")))
> +  (let ((buf (get-buffer-create "*xwidget-log*")))

Why?  Is this buffer expected to be displayed to the user?
If not, then a leading space is preferable.

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

Three (or more) semicolons means "section heading" (as used in ";;;
Commentary:" at the beginning of the file), so please don't use it like
you do here.

> +;;; TODO: Response handling other than download.

Same here.

> +               (if (vectorp arg)
> +                   (funcall proc (seq-into arg 'list))
> +                 (funcall proc arg))))

Aka

    (funcall proc (if (vectorp arg) (seq-into arg 'list) arg))

>              (t (xwidget-log "unhandled event:%s" xwidget-event-type))))))
>  
>  (defvar bookmark-make-record-function)
> +(defvar isearch-search-fun-function)
> +(when (memq window-system '(mac ns))
> +  (defvar xwidget-webkit-enable-plugins nil
> +    "Enable plugins for xwidget webkit.
> +If non-nil, plugins are enabled.  Otherwise, disabled."))

Why is this defvar here?  Shouldn't this be defined in the C code
(presumably via DEFVAR_BOOL) since it's only used in the C code?

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

If not used, then why have it as an argument?

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

Is `file-name` expected to be an absolute file name here?  If not, then
please use (expand-file-name file-name xwidget-webkit-download-dir)
as DEFAULT argument.

Also please don't provide the INITIAL argument since there's no reason
this read-file-name should behave differently than all others.

> +    (if (file-directory-p save-name)
> +        (setq save-name (concat (file-name-as-directory save-name) 
> file-name)))

Don't use `concat`: use `expand-file-name`.  This will save you from
needing file-name-as-directory.  Also, unless you're sure file-name is
not absolute, you might like to use (file-name-nondirectory file-name)
to avoid surprises.

> +(defvar 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'.")

Shouldn't this be a defcustom rather than a defvar?

>  (defun xwidget-webkit-bookmark-make-record ()
>    "Integrate Emacs bookmarks with the webkit xwidget."
>    (nconc (bookmark-make-record-default t t)
> -         `((page     . ,(xwidget-webkit-current-url))
> -           (handler  . (lambda (bmk) (browse-url
> -                                 (bookmark-prop-get bmk 'page)))))))
> -
> +         `((filename . ,(xwidget-webkit-current-url))
> +           (handler  . (lambda (bmk)
> +                         (browse-url
> +                          (bookmark-prop-get bmk 'filename)
> +                          xwidget-webkit-bookmark-jump-new-session)
> +                         (switch-to-buffer
> +                          (xwidget-buffer 
> (xwidget-webkit-last-session))))))))

Bookmarks can be saved, so please make sure the old "page" attribute
still works with the new code.

> +(defun xwidget-webkit-search-fun-function ()
> +  "Return the function which perform the search in xwidget webkit."
> +  (lambda (string &optional bound noerror count)
> +    (ignore bound noerror count)

Rather than use `ignore` you can simply add an underscore at the
beginning of the arg's names, as in

    (string &optional _bound _noerror _count)

> +      ;; Forward or backward
> +      (if (eq isearch-forward nil)
> +          (setq search-forward "false")
> +        (setq search-forward "true"))

Aka

    (setq search-forward (if isearch-forward "true" "false"))

BTW, even better, you can fold this directly into the `let`:

    (let* ((current-length (length string))
           (search-forward (if isearch-forward "true" "false"))
           ...

so you don't need the `setq` at all.

> +      (if (eq current-length xwidget-webkit-isearch-last-length)
> +          (setq search-repeat "true")
> +        (setq search-repeat "false"))

I think you can guess what I'm about to say here (I already added the
`*` at the end of the `let` above for that ;-)

> +      (if (eq isearch-forward nil)
> +          (goto-char (point-max))
> +        (goto-char (point-min)))

And here

    (goto-char (if isearch-forward (point-min) (point-max)))

> -  "javascript that finds the active element."
> +  "Javascript that finds the active element."

Good, thanks.

>  (defun xwidget-webkit-insert-string ()
> -  "Prompt for a string and insert it in the active field in the
> +  "Prompt for a string and insert it in the active field in the \
>  current webkit widget."

Better shorten it to fit with 80 (or even less) columns.
E.g. "Insert string into the active field in the current webkit widget."

This should normally take `string` as argument (and hence prompt from
within the `interactive`).  If that's not possible, please add a comment
explaining why.

> +    ;; @javascript-callback
> +    (defun xwidget-webkit-insert-string-cb (field)

Don't nest defuns within other defuns: it doesn't do what it intends to do.
Just move the inner defun outside, so the syntax matches the behavior.
Or just keep the `lambda` as it was before (and if that doesn't work,
then we should likely fix that rather than work around the problem).

> +     'xwidget-webkit-insert-string-cb)))

Please use #' rather than ' when quoting a function.

>      ;; The xwidget id is stored in a text property, so we need to have
>      ;; at least character in this buffer.
> -    (insert " ")
> +    ;; Insert invisible url, good default for next `g' to browse url.
> +    (insert url)
> +    (put-text-property 1 (+ 1 (length url)) 'invisible t)

Better avoid such immediate positions.  E.g. use

    (let ((start (point)))
      (insert url)
      (put-text-property start (point) 'invisible t)

>      (setq xw (xwidget-insert 1 'webkit bufname

And I think the `1` can then be changed to `start`.

> +(defun xwidget-webkit-current-url-message-kill ()
> +  "Message the current xwidget webkit URL and place it on the `kill-ring'."

"Message" is not really a verb and when used as such (which English lets
you do, admittedly), I tend to understand it as "send a message to the
current xwidget ...".  I think "Display" or "Show" will work better.

> +  (interactive)
> +  (message "url: %s" (kill-new (or (xwidget-webkit-current-url) ""))))

I'd use a capitalized "URL:".

> -  (xwidget-webkit-get-selection (lambda (selection) (kill-new selection))))
> -
> +  (xwidget-webkit-get-selection #'kill-new))

Nice!

As for the rest, I didn't notice any of the usual cosmetic problems
(such as lack of space before open parens), so from a purely cosmetic
point of view, it looked fine.

Of course something that's still missing is a commit message.
For nsxwidget.h and nsxwidget.m that's trivial (it need just say "New
files" for them), but it should also include some author information
since you're not the original author of (most of) this code, IIUC.


        Stefan




reply via email to

[Prev in Thread] Current Thread [Next in Thread]