[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] Add xwidget webkit support for macOS Cocoa
From: |
Stefan Monnier |
Subject: |
Re: [PATCH v3] Add xwidget webkit support for macOS Cocoa |
Date: |
Wed, 05 Jun 2019 09:55:24 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) |
Thanks, looks good for the part I can review.
See additional nitpicks in case nobody comes up with more
substantial comments.
> +(defun xwidget-webkit-split-below ()
> + "Clone current URL into a new widget place in new window below.
Great.
> +(defun xwidget-webkit-split-right ()
> + "Get the URL of current session, then browse to the URL \
> +in `split-window-right' with a new xwidget webkit session."
You missed this one, tho.
> + (format "window.scrollBy(0, %d);"
> + (or n (xwidget-window-inside-pixel-height (selected-window))))))
Great.
> - "window.scrollBy(0, -50);"))
> + (cond ((null n)
> + (format "window.scrollBy(0, %d);"
> + (- (xwidget-window-inside-pixel-height
> (selected-window)))))
> + (t (format "window.scrollBy(0, %d);" (- n))))))
You missed this one, tho.
> +(defcustom xwidget-webkit-scroll-line-height 50
> + "Default line height in pixels for scroll xwidget webkit."
> + :type 'integer
> + :group 'xwidget)
Those `:group 'xwidget` are redundant since it defaults to the last
`defgroup` in the buffer anyway.
> + ;; TODO: Response handling other than download.
> + ((eq xwidget-event-type 'response-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)))
BTW, where does the "response-callback" name come from?
According to the above code, this is used in the case of downloads, so
why is it not called "download-callback"?
> +(defun xwidget-webkit-save-as-file (url mime-type &optional file-name)
> + "For XWIDGET webkit, save URL resource of MIME-TYPE as FILE-NAME."
> + (let ((save-name (read-file-name
> + (format "Save '%s' file as: " mime-type)
> + xwidget-webkit-download-dir
> + (expand-file-name
> + file-name
> + xwidget-webkit-download-dir) nil file-name)))
Again, please don't pass `file-name` as INITIAL arg: leave it nil.
> - `((page . ,(xwidget-webkit-current-url))
> - (handler . (lambda (bmk) (browse-url
> - (bookmark-prop-get bmk 'page)))))))
> -
> + `((page . ,(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))))))))
I see you reverted to `page`, which is fine, but then the
(bookmark-prop-get bmk 'filename) needs to be changed back to
(bookmark-prop-get bmk 'page) as well, shouldn't it?
Also, I don't really understand the new code:
- how do we know that `browse-url` will use the xwidget browser rather than
any other? I think we should either call `browse-url` and presume it
can be *any* browser, or call xwidget-webkit-browse-url instead.
- Why do we need the switch-to-buffer? Why not rely on
(xwidget-webkit-)browse-url to Do The Right Thing?
- switch-to-buffer fails miserably in configs such as mine
(separate minibuffer frame and dedicated windows), so better avoid it
in Elisp and use things like pop-to-buffer or pop-to-buffer-same-window.
> + ;; Insert invisible url, good default for next `g' to browse url.
> + (let ((start (point)))
> + (insert url)
> + (put-text-property start (+ start (length url)) 'invisible t))
> (setq xw (xwidget-insert 1 'webkit bufname
Please use `start` rather than 1.
Stefan
- Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa, 조성빈, 2019/06/02
- Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa, Alan Third, 2019/06/03
- Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa, Stefan Monnier, 2019/06/03
- Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa, Paul Eggert, 2019/06/03
- [PATCH v2] Add xwidget webkit support for macOS Cocoa, Sungbin Jo, 2019/06/03
- Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa, Stefan Monnier, 2019/06/03
- Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa, Andreas Schwab, 2019/06/04
- [PATCH v2] Add xwidget webkit support for macOS Cocoa, Sungbin Jo, 2019/06/04
- [PATCH v3] Add xwidget webkit support for macOS Cocoa, Sungbin Jo, 2019/06/04
- Re: [PATCH v3] Add xwidget webkit support for macOS Cocoa,
Stefan Monnier <=
- Re: [PATCH v3] Add xwidget webkit support for macOS Cocoa, Eli Zaretskii, 2019/06/05
- Re: [PATCH v3] Add xwidget webkit support for macOS Cocoa, Alan Third, 2019/06/23
- Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa, Alan Third, 2019/06/04