emacs-devel
[Top][All Lists]
Advanced

[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: Sat, 27 Jul 2019 21:35:07 +0900

2019. 7. 27. 오후 7:41, Eli Zaretskii <address@hidden> 작성:

>> From: 조성빈 <address@hidden>
>> Date: Fri, 26 Jul 2019 03:51:11 +0900
>> Cc: address@hidden,
>> address@hidden
>> 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.

Ok, will do.

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

(I’m currently on my phone so I can’t really check but) I recall this part of 
the checklist was specific to the NS port? 

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

Ok, will do.

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

Ok, will do.

>> (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.

Ok, will do.

>> @@ -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?

Well, there wasn’t a particular problem, but it looked like that message was 
more used for warnings or errors. Should I revert it? 

>> +#elif defined NS_IMPL_COCOA
>> +              nsxwidget_resize_view(xv, xw->width, xw->height);
>                                      ^
> Space before the opening parenthesis is missing.

Ok, will fix.

>> +*** 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).

Ok, will fix.

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

Ok, will do.

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

Ok, will fix.

>> +(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?

Well, it was meaning that the URL will be saved at the directory the user 
specified in the prompt with the name FILE-NAME.

I’m not sure how I should reword it due to my  poor English skills; can you 
give me some suggestions? 



reply via email to

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