[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5] Enable xwidgets on macOS
From: |
Eli Zaretskii |
Subject: |
Re: [PATCH v5] Enable xwidgets on macOS |
Date: |
Sat, 27 Jul 2019 13:41:01 +0300 |
> From: 조성빈 <address@hidden>
> Date: Fri, 26 Jul 2019 03:51:11 +0900
> Cc: address@hidden,
> address@hidden
>
> Hello, I have prepared three patches to apply on master.
Thanks. Alan, could you please review the NS specific parts?
> diff --git a/etc/NEWS b/etc/NEWS
> index 5313270411..52b49bab75 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2440,6 +2440,18 @@ was able to 'set' modifiers without the knowledge of
> the user.
> ** On NS multicolor font display is enabled again since it is also
> implemented in Emacs on free operating systems via Cairo drawing.
>
> +** On macOS, Xwidget is now supported.
> +If Emacs was built with xwidget support, you can access the embedded
> +webkit browser with 'M-x xwidget-webkit-browse-url'. Viewing two
> +instances of xwidget webkit is not supported.
> +
> +*** New functions for xwidget-webkit mode
> +'xwidget-webkit-clone-and-split-below',
> +'xwidget-webkit-clone-and-split-right'.
> +
> +*** New variable 'xwidget-webkit-enable-plugins'.
The last two entries are not specific to macOS, are they? If so, they
should be in the general sections of NEWS.
> diff --git a/etc/TODO b/etc/TODO
> index a065763933..bda1cf8f9e 100644
> --- a/etc/TODO
> +++ b/etc/TODO
> @@ -640,15 +640,6 @@ from the emacsclient process.
>
> This sections contains features found in other official Emacs ports.
>
> -**** Support for xwidgets
> -
> -Emacs 25 has support for xwidgets, a system to include operating
> -system components into an Emacs buffer. The components range from
> -simple buttons to webkit (effectively, a web browser).
> -
> -Currently, xwidgets works only for the gtk+ framework but it is
> -designed to be compatible with multiple Emacs ports.
The MS-Windows and non-GTK builds of Emacs on X still don't support
xwidgets, so I think this entry should not be deleted in its entirety,
it should still say that some configurations don't support xwidgets.
> +(defun xwidget-webkit-clone-and-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 should be reworded:
Get the URL of current session, then browse that URL in another
window after splitting the selected window horizontally.
> +(defun xwidget-webkit-clone-and-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 (but use "vertically" instead of "horizontally").
> (defvar bookmark-make-record-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."))
It is better to have this defvar unconditionally, and tell in the doc
string that it only has effect on macOS.
> @@ -228,13 +247,14 @@ offscreen_damage_event (GtkWidget *widget, GdkEvent
> *event,
> if (GTK_IS_WIDGET (xv_widget))
> gtk_widget_queue_draw (GTK_WIDGET (xv_widget));
> else
> - printf ("Warning, offscreen_damage_event received invalid xv
> pointer:%p\n",
> - xv_widget);
> + message ("Warning, offscreen_damage_event received invalid xv
> pointer:%p\n",
> + xv_widget);
Why replace printf by message?
> +#elif defined NS_IMPL_COCOA
> + nsxwidget_resize_view(xv, xw->width, xw->height);
^
Space before the opening parenthesis is missing.
> +*** Functions 'xwidget-webkit-scroll-up', 'xwidget-webkit-scroll-down'
> +now supports scrolling arbitrary pixel values. It now treats the
^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^
"support" (plural) "by arbitrary pixel values". "They now treat" (plural).
> +(defun xwidget-webkit-scroll-up-line (&optional n)
> + "Scroll webkit up by N lines.
> +The height of line is calculated with `window-font-height'.
> +Stop if the bottom edge of the page is reached.
> +If N is omitted or nil, scroll up by one line."
> + (interactive "p")
> + (xwidget-webkit-scroll-up (* n (window-font-height))))
> +
> +(defun xwidget-webkit-scroll-down-line (&optional n)
> + "Scroll webkit down by N lines.
> +The height of line is calculated with `window-font-height'.
> +Stop if the top edge of the page is reached.
> +If N is omitted or nil, scroll down by one line."
> + (interactive "p")
> + (xwidget-webkit-scroll-down (* n (window-font-height))))
> +
> +(defun xwidget-webkit-scroll-forward (&optional n)
> + "Scroll webkit horizontally by N chars.
> +The width of char is calculated with `window-font-width'.
> +If N is ommited or nil, scroll forwards by one char."
> + (interactive "p")
> (xwidget-webkit-execute-script
> (xwidget-webkit-current-session)
> - "window.scrollBy(50, 0);"))
> -
> -(defun xwidget-webkit-scroll-backward ()
> - "Scroll webkit backwards."
> - (interactive)
> + (format "window.scrollBy(%d, 0);"
> + (* n (window-font-width)))))
> +
> +(defun xwidget-webkit-scroll-backward (&optional n)
> + "Scroll webkit back by N chars.
> +The width of char is calculated with `window-font-width'.
> +If N is ommited or nil, scroll backwards by one char."
> + (interactive "p")
These commands should say in their doc strings that interactively N is
the prefix numeric argument.
> diff --git a/etc/NEWS b/etc/NEWS
> index f9a42f73be..c7f980f212 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2469,6 +2469,10 @@ instances of xwidget webkit is not supported.
>
> *** New variable 'xwidget-webkit-enable-plugins'.
>
> +** On macOS, downloading files from xwidget-webkit is supported.
> +
> +*** New variable 'xwidget-webkit-download-dir'.
The macOS-specific part of this should be in the non-free OS part of
NEWS.
> +(defun xwidget-webkit-save-as-file (url mime-type file-name)
> + "For XWIDGET webkit, save URL of MIME-TYPE to location specified by user.
> +FILE-NAME combined with `xwidget-webkit-download-dir' is the default file
> name
> +of the prompt when reading. When the file name the user specified is a
> +directory, URL is saved at the specified directory as FILE-NAME."
The last sentence is unclear: what will be the directory where the URL
will be saved, and what will be the name of the saved file in that
directory?
- [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, Alan Third, 2019/07/19
- Re: [PATCH v5] Enable xwidgets on macOS, Eli Zaretskii, 2019/07/20
- Re: [PATCH v5] Enable xwidgets on macOS, 조성빈, 2019/07/23
- Re: [PATCH v5] Enable xwidgets on macOS, 조성빈, 2019/07/25
- Re: [PATCH v5] Enable xwidgets on macOS, Eli Zaretskii, 2019/07/26
- Re: [PATCH v5] Enable xwidgets on macOS, Richard Stallman, 2019/07/26
- Re: [PATCH v5] Enable xwidgets on macOS, Eli Zaretskii, 2019/07/27
- Re: [PATCH v5] Enable xwidgets on macOS,
Eli Zaretskii <=
- 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
- Re: [PATCH v5] Enable xwidgets on macOS, 조성빈, 2019/07/30
- Re: [PATCH v5] Enable xwidgets on macOS, Stefan Monnier, 2019/07/30
- Re: [PATCH v5] Enable xwidgets on macOS, 조성빈, 2019/07/31
- Re: [PATCH v5] Enable xwidgets on macOS, 조성빈, 2019/07/30
- Re: [PATCH v5] Enable xwidgets on macOS, Juri Linkov, 2019/07/30