bug#51716: 29.0.50; [PATCH] Expose xwidget navigation history to Lisp code

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> This should be @var, not @code.

>> +in its navigation history.
>> +
>> +If @var{rel-pos} is zero, the current page will be reloaded instead.
>> +@end defun
>> +
>> +@defun xwidget-webkit-back-forward-list xwidget &optional limit
>
> Hmm... given that we have xwidget-webkit-goto-history, why do we need
> this new function as well?  Or why do we need both?

`xwidget-webkit-goto-history' only lets you navigate to a history index:
it doesn't tell you what that index actually is, or indeed, if it even
exists at all.

>> +The returned value is a list of the form @code{(@var{back} @var{here}
>> +@var{forward})}

> This list should be wrapped in @w{..}, so that it is kept unbroken
> between 2 lines.

Thanks.

>> where @var{here} is the current navigation item,
>> +while @var{back} is a list of items containing the history behind the
>> +current navigation item, and @var{forward} is a list of items in front
>> +of the current navigation item.
>
> The notions of "behind" and "in front" are not well-defined in this
> context.  The text should make more clear what does each one of those
> mean.

I tried to clarify, thanks.

>>                                  @var{back}, @var{here} and
>> +@var{forward} can all be @code{nil}.

> What is the meaning of each one being nil?  The text leaves that
> unsaid.

BACK being nil means that we are at the beginning of the history.
FORWARD being nil means that there is nothing after the current place in
history.  HERE being nil means that WebKit hasn't recorded any pages in
history yet.

I will add that to the documentation, thanks.

> Please use @w{..} here as well.  basically you should use it for any
> form that has embedded whitespace, and can therefore be split between
> two lines when Texinfo fills and justifies the text.

Done, thanks.

>>                           In these lists, @var{idx} is an index that
>> +can be passed to @code{xwidget-webkit-goto-history}, @var{title} is
>> +the human-readable title of the item, and @var{uri} is the URI of the
>> +item.

> URI, not URL?

Yes, as WebKit allows any URI to appear in history items.

>> +DEFUN ("xwidget-webkit-back-forward-list", 
>> Fxwidget_webkit_back_forward_list,
>> +       Sxwidget_webkit_back_forward_list, 1, 2, 0,
>> +       doc: /* Return the navigation history of XWIDGET, a WebKit xwidget.
>> +
>> +The history is returned as a list of the form (BACK HERE FORWARD),
>    ^^^^^^^^^^^^^^^^^^^^^^^
> Passive tense alert!

[...]

> "lists of ... , which are lists" uses "lists" twice, which is
> redundant.  Better say
>
>    ... are lists of elements of the form (IDX TITLE URI) ...

Thanks, I fixed.

> This belongs to the manual, not a doc string.

Thanks, I moved it there.

> Each one or both together? the text is ambiguous about that.

I tried to fix that, please check.  Thanks.

>> +#ifdef USE_GTK
>> +  webview = WEBKIT_WEB_VIEW (xw->widget_osr);
>
> I guess this again means this function is a no-op without GTK?  Then
> let's not define it except in the GTK build.

Done, thanks.

>> +  list = webkit_web_view_get_back_forward_list (webview);
>> +  item = webkit_back_forward_list_get_current_item (list);
>> +  lim = XFIXNAT (limit);
>> +
>> +  if (item)
>> +    {
>> +      item_title = webkit_back_forward_list_item_get_title (item);
>> +      item_uri = webkit_back_forward_list_item_get_uri (item);
>> +      here = list3 (make_fixnum (0),
>> +                build_string (item_title ? item_title : ""),
>> +                build_string (item_uri ? item_uri : ""));
>> +    }
>> +  parent = webkit_back_forward_list_get_back_list_with_limit (list, lim);
>> +
>> +  if (parent)
>> +    {
>> +      for (i = 1, tem = parent; parent; parent = parent->next, ++i)
>> +    {
>> +      item = tem->data;
>> +      item_title = webkit_back_forward_list_item_get_title (item);
>> +      item_uri = webkit_back_forward_list_item_get_uri (item);
>> +      title = build_string (item_title ? item_title : "");
>> +      uri = build_string (item_uri ? item_uri : "");

> build_string can produce either multibyte or unibyte strings.  Which
> ones do we want?  And shouldn't we decode the strings that come from
> WebKit?

Sorry for that. We want UTF-8 strings, as that's the format of strings
which come from WebKitGTK.  (And most GLib-related software, as well).

>> +      back = Fcons (list3 (make_fixnum (-i), title, uri), back);
>> +    }
>> +    }
>> +
>> +  g_list_free (parent);
>> +
>> +  parent = webkit_back_forward_list_get_forward_list_with_limit (list, lim);
>> +
>> +  if (parent)
>> +    {
>> +      for (i = 1, tem = parent; parent; parent = parent->next, ++i)
>> +    {
>> +      item = tem->data;
>> +      item_title = webkit_back_forward_list_item_get_title (item);
>> +      item_uri = webkit_back_forward_list_item_get_uri (item);
>> +      title = build_string (item_title ? item_title : "");
>> +      uri = build_string (item_uri ? item_uri : "");
>> +      forward = Fcons (list3 (make_fixnum (i), title, uri), forward);
>> +    }

> This generates the lists in the reverse order, doesn't it?

Yes, but what's important here is the index `i', not the order of the
list.  But I agree that it would be OK to nreverse the contents of that
list.

0001-Expose-xwidget-navigation-history-to-Lisp-code.patch
Description: Text Data