From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [PATCH v2] Add xwidget webkit support for macOS Cocoa Date: Mon, 03 Jun 2019 22:40:01 -0400 Message-ID: References: <12fa256f-5322-8a47-e8d0-5430e0fef741@cs.ucla.edu> <20190603235121.15533-1-pcr910303@icloud.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="16498"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org To: Sungbin Jo Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Jun 04 04:40:18 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hXzN3-00047G-SL for ged-emacs-devel@m.gmane.org; Tue, 04 Jun 2019 04:40:18 +0200 Original-Received: from localhost ([127.0.0.1]:44217 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hXzN2-00060N-Qp for ged-emacs-devel@m.gmane.org; Mon, 03 Jun 2019 22:40:16 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:50711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hXzMv-000609-U9 for emacs-devel@gnu.org; Mon, 03 Jun 2019 22:40:11 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hXzMt-0003Kk-Vi for emacs-devel@gnu.org; Mon, 03 Jun 2019 22:40:09 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:58755) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hXzMt-0003IF-MI for emacs-devel@gnu.org; Mon, 03 Jun 2019 22:40:07 -0400 Original-Received: from pmg1.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 8CAF910127B; Mon, 3 Jun 2019 22:40:05 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id AC5C3100A37; Mon, 3 Jun 2019 22:40:03 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1559616003; bh=aMbXG8uj8J85fcfma4jlRBxkziT1m2szgiCUdhW9QAs=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=WOHJ7auajslZpgBr234aY3XDPpbvnvYxIQM+Aapg0ROS5bKwzPgexmSOEuiFBA2/p uqvxqE1O5h0RitbwVvq5f80REx9KbI5zLkAauFRSeT5HY3V2097cdogT5ti+8WNdq9 F5+J0YaoIQ4cQKAcl6fdTX/CuB74I1wP0oTrDG6Vs1F9rs+sMIRC8I4mz/3+eWGqzm YyrtE4jfGAKDtZqQgAFByJ3LK/4sROI0ITsGUjh6E3GCOn5txr48qXx8zwJ1eS1/hc KMKMoDEXHrUkig2L9nVhLmoFTKBMbVWzxJZB0pLGwAiIoMrBZrbDQODfQpkg6QZPKN TnzmIOyG+HxTA== Original-Received: from ceviche (unknown [45.72.167.35]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 35BD7120B3F; Mon, 3 Jun 2019 22:40:03 -0400 (EDT) In-Reply-To: <20190603235121.15533-1-pcr910303@icloud.com> (Sungbin Jo's message of "Tue, 4 Jun 2019 08:51:20 +0900") X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 132.204.25.50 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:237237 Archived-At: 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