Skip to content

Conversation

@Boruch-Baum
Copy link
Contributor

This PR was originally shared on the mailing list ~[13091]. It fixes the handling of the START and SIZE args for the global history display, applies cosmetic changes to that display, and adds an option for all forms of history display to be done in a new buffer.

+ New boolean defcustom w3m-history-in-new-buffer, default to current
  behavior: nil. Applied to global and buffer-local history displays.

+ BUGFIX: Function w3m-db-history was not allowing an interactive user
  to set its options.

+ BUGFIX: Function w3m-about-db-history was not properly handling its
  START and SIZE arguments.

+ Cosmetic changes to global history listing
  + Extend width of title string to maximum.
  + Center heading
  + Page navigation links aligned to page edges
  + Remove extraneous ';' after url's
  + Unicode ellipsis sign used instead of three ascii dots (buys two
    characters for long titles).
  + Add ellipsis when truncating url's
  + All dates display full timestamp, with today's entries labeled
    "Today".

+ Modified docstrings to explain the functions' options.
@tsuchm tsuchm requested review from yamaoka and removed request for yamaoka February 15, 2019 09:48
@yamaoka
Copy link
Contributor

yamaoka commented Feb 18, 2019

Sorry for the delay. I'm reviewing your #4 changes now.
Hopefully merge to the master tomorrow with some changes.

@yamaoka
Copy link
Contributor

yamaoka commented Feb 19, 2019

Pushed to master by two steps (yours and mine).
Let me know if there is any mistake or any discrepancy
with your original plan. Thanks for your contribution.

I've replaced this kind of form

(w3m-goto-url URL :save-pos t)

with

(w3m-goto-url URL nil nil nil nil nil nil nil t)

in two places. Is it right? Otherwise, is there a means
to use such a convenient way?

@Boruch-Baum
Copy link
Contributor Author

@yamaoka : I don't think I can get to this today; I've spent too much time already today working on other parts of the project.

@Boruch-Baum
Copy link
Contributor Author

@yamaoka : Diff'ing against 7fa00f9, I like much of what your changes accomplish. Here are the points that I have questions about (all line numbers are in file w3m.el):

  1. [line 10821] (width (- (w3m-display-width) 21))
    I had 19 instead of 21, to squeeze in more space; was it giving you trouble with wide characters or something?

  2. [lines 11005-11021] I think my version is better

2.1) There exists a variable w3m-db-history-display-size which is by default set nil, and is passed to the function as SIZE when called interactively. When a person sets that default variable as non-nil, your version does not allow the default to be changed (ie. enter a number instead of pressing return at the prompt), and more importantly, does not prompt for START, so the user loses that functionality.

2.2) It seems that you're checking for an X11 version of emacs, and in that case you are performing an extra check, and possibly prompting the user. If you really do all that, it should also be done for non-X11 emacs also?

2.3) The check seems to have a bug because it is being performed based upon START, but that argument will always be zero at that point. It should be performed after the user is prompted for START.

2.4) The check is based upon a size comparison, and seems to be in order to avoid something being to long or too large, but it isn't taking into account SIZE, or the ability of the user to begin at any START point and click on the 'prev' link to go back in the list. I don't see how this check will ever be useful. If the purpose is to avoid a long wait while a single page is being processed, then the check should be after prompting for SIZE, only if the user accepted the default of 0, and be based upon some MAX-PAGE-DISPLAY-SIZE (not max database size), and the message should be a warning to the effect "That would be a very long page (n lines). Proceed anyway?", and upon a no, exit.

2.5) What happens when the user answers no to the prompt? In that case, the use has told us not to continue, but then the function does continue! It proceeds to prompt for START and SIZE.

  1. function w3m-db-history-fix-indentation I don't know how to test, so I'll take your word for it.

@Boruch-Baum
Copy link
Contributor Author

I have a follow-up: When viewing all history on a single page, your version doesn't fit an entire time-stamp in the viewable page. The problem is only noticeable if your history includes entries prior to 'today'

@yamaoka
Copy link
Contributor

yamaoka commented Feb 21, 2019

@Boruch-Baum

  1. [line 10821] (width (- (w3m-display-width) 21))
    I had 19 instead of 21, to squeeze in more space; was it giving you trouble with wide
    characters or something?

For mainly non-ASCII characters of which the width is interpreted variously as the case may be.
The typical one is the ellipsis character "…" that you introduced. w3m seems to recognize
that its width is 1, however it is displayed by a wide character (width=2) in my Emacs environment.
Here is a recipe to see it (you may not see the difference, though).

(with-temp-buffer
(insert "\

404 Not Found 23:59:59 2019-12-31
Long Title… 23:59:59 2020-01-01
\n") (w3m-rendering-half-dump 'utf-8) (w3m-decode-entities) (concat "\n" (buffer-string)))
  1. function w3m-db-history-fix-indentation I don't know how to test, so I'll take your word for it.

Because of the above reason, the indentation for Time/Date portions vary line by line.
The function arranges the indentation column all to be the minimum one of them.

To be continued

@yamaoka
Copy link
Contributor

yamaoka commented Feb 21, 2019

@Boruch-Baum
The broken part in the previous comment is here:
Elisp.gz

@yamaoka
Copy link
Contributor

yamaoka commented Feb 21, 2019

@Boruch-Baum

2.1) There exists a variable w3m-db-history-display-size which is by default set nil, and is
passed to the function as SIZE when called interactively. When a person sets that default
variable as non-nil, your version does not allow the default to be changed (ie. enter a
number instead of pressing return at the prompt), and more importantly, does not prompt for
START, so the user loses that functionality.

Nah. There is no interactive spec in the w3m-db-history' function (i.e., there is no operand in the (interactive) line), so start' and `size' will be nil if it is called interactively, and a user
should always be prompted for them. You wrote in the comment:

(t ; called interactively; possibly indirectly

I imagined that the indirect case would probably be to call this function from a custom ELisp
command that specifies start' and/or size' by something like prompting a user. In that case, this
function should honor those arguments rather than prompting a user for them again, I thought.

To be continued

@yamaoka
Copy link
Contributor

yamaoka commented Feb 21, 2019

@Boruch-Baum

2.2) It seems that you're checking for an X11 version of emacs, and in that case you are
performing an extra check, and possibly prompting the user. If you really do all that, it
should also be done for non-X11 emacs also?

I came to think I should withdraw the GUI stuff. The reason I added it is because I felt funny
that the command prompts me so as to enter start' and size' using the keyboard even if I launched
this command by the mouse click on the tool bar button. To raise the yes-or-no dialog box was a
desperate measure as there is no easy way to create a box for entering a number. Anyway it is bad
design that it does differs from the way when not using GUI.

To be continued

@yamaoka
Copy link
Contributor

yamaoka commented Feb 22, 2019

@Boruch-Baum
After all I've removed the GUI stuff and made the w3m-db-history function simple.
Could you review it?

@Boruch-Baum
Copy link
Contributor Author

So, as summary, using my original point numbering as reference:

[line 10821] (width (- (w3m-display-width) 21)): @yamaoka says that this is because how 'wide' characters appear differently in different environments. If so, is this really the correct solution? It doesn't properly handle the character in my utf-8 environment, and what about any number of wide characters that could be present in the URL title? Shouldn't there be some kind of environment check and count of all the wide characters, instead of just adding '2' in all cases, even if its not needed?

2.1) There exists a variable w3m-db-history-display-size

@yamaoka : The most common way to use function w3m-db-history is by an interactive call to function w3m-history with the prefix argument. (C-u h keybinding by default). That is where the size arg is set non-nil to the default value.

2.2) It seems that you're checking for an X11 version

There is value in performing some form of size test, but not the test that the code performs, and not only in the GUI/X11 case.

@yamaoka : As I'm writing I see that you've pushed another commit about this, removing some GUI stuff, and adding an interactive list. The idea for the interactive list had been made by Filipp Gunbin on the mailing list ~13096.

@Boruch-Baum
Copy link
Contributor Author

@yamaoka : I just checked your update in commit 0c956f8. When called directly interactively it does seem to work; however, the default design case of being called using C-u h does not work. That is the case I mentioned as 'indirect interactive' that I had designed for in my original PR commit. Try C-u h and see.

* Properly handle indirect interactive call to the function

* Add sanity check to prevent long waits for creation of very long
  history pages.

* Add programming note.

* Set sane default for variable w3m-db-history-display-size
* It turns out to be useless in all cases, because it only becomes an
  issue when the user has explicitly changed the defcustom default
  value of w3m-db-history-page-size
@Boruch-Baum
Copy link
Contributor Author

Boruch-Baum commented Feb 22, 2019

I just posted two commits: 169a3f6 restores the functionality, and adds a sanity check for long pages, in a way that I think was the original intent for the check that I criticized above. However, I then proceeded to remove that in the next commit 6fdff93 because I don't see that the check ever makes any sense, because it would only be invoked when the user explicitly changes the defcustom value w3m-db-history-page-size, in which case the user has already told us his preference!

I also did sneakily change the default for that value to make it more a sane 100 instead of based upon 500 entries.

Also, I fast-forward'ed this PR branch so that it can be merged in the standard way again.

+ Apply (string-width) to the ellipsis and enclosure characters so
  that they are properly accounted for in all coding systems and
  fonts.

+ Change enclosure characters from "&foo;" to prettier, single unicode
  ones.
@Boruch-Baum
Copy link
Contributor Author

My recent commit 57ca0ef should account properly for the ellipsis and bracket characters, no matter how they may be changed in the future, and for all encodings and fonts, I hope.

@yamaoka
Copy link
Contributor

yamaoka commented Feb 25, 2019

@Boruch-Baum

[line 10821] (width (- (w3m-display-width) 21)): @yamaoka says that this is because how 'wide' characters appear differently in different environments. If so, is this really the correct solution?

w3m-db-history-fix-indentation and tweaking the width (-21) are no more than workarounds.

It doesn't properly handle the character in my utf-8 environment, and what about any number of wide characters that could be present in the URL title? Shouldn't there be some kind of environment check and count of all the wide characters, instead of just adding '2' in all cases, even if its not needed?

That is a w3m problem, not that of Emacs or emacs-w3m, I think (though Emacs on TTY makes a mysterious behavior). Emacs (at least the ones I have) says that the string width of "…" is 2 no matter what display (X or TTY) or font is used. Please try the attached ELisp snippet and

Eval: (and (search-forward "bar" nil t) (goto-char (match-beginning 0)))

in the beginning of each "foo" line.
ELisp2.gz

@yamaoka
Copy link
Contributor

yamaoka commented Feb 25, 2019

@Boruch-Baum

@yamaoka : [...]Try C-u h and see.

What does C-u h mean? Is it C-u NUMBER M-x w3m-db-history RET, i.e., passing the prefix argument to the function?

@Boruch-Baum
Copy link
Contributor Author

Boruch-Baum commented Feb 25, 2019 via email

@Boruch-Baum
Copy link
Contributor Author

Boruch-Baum commented Feb 25, 2019 via email

@emacs-w3m-notifier
Copy link

emacs-w3m-notifier commented Feb 25, 2019 via email

@yamaoka
Copy link
Contributor

yamaoka commented Feb 25, 2019

[3]ELisp2.gz

I did. What I get is:

foo…        bar
           `bar' starts at the column 48
foo**       bar
           `bar' starts at the column 48

But I don't understand why or how that's relevant. For (string-width "…"), I get an answer of '1'...

Oh, I get '2' in every version of Emacsen (currently 26.1, 26.1.92, 27.0.50).  However w3m seems to recognize it '1'.  This is the cause of our disagreement!  In the 80 column width Emacs window, ELisp2 shows:

foo…                                    bar                                   
                                         `bar' starts at the column 41
foo**                                   bar                                   
                                        `bar' starts at the column 40

I also realized that this is due to the value of 'current-language-environment' of Emacs.  If I change it from Japanese to English, (string-width "…") gets 1, even if "…" is displayed as a wide character.  So, a possible solution might be to bind language-environment to English locally in emacs-w3m buffers.  I don't know if it can be and is halmless, though.

@yamaoka
Copy link
Contributor

yamaoka commented Feb 25, 2019

@Boruch-Baum

What does C-u h mean?

C-u M-x w3m-history

So, is what you want to do something like this?

(defun xxx-w3m-db-history (&optional start size prefix-arg)
  "docstring"
  (interactive (list nil nil current-prefix-arg))
  (if prefix-arg
      (setq start (read-number
		   "How far back in the history to start displaying: "
		   0)
	    size (read-number
		  "How many entries per page (0 for all on one page): "
		  (or w3m-db-history-display-size 0)))
    (or start (setq start 0))
    (or size (setq size (or w3m-db-history-display-size 0))))
  (message "START=%s, SIZE=%s" start size))

@tats
Copy link
Contributor

tats commented Feb 25, 2019

For (string-width "…"), I get an answer of '1'...
Oh, I get '2' in every version of Emacsen (currently 26.1, 26.1.92, 27.0.50).
However w3m seems to recognize it '1'.

The width of "…" is ambiguous.

w3m recognizes its width is 1 by default (east_asian_width=0).
When w3m -o east_asian_width=1 (or "east_asian_width 1" is in the
config file), w3m recognizes its width is 2.

I also realized that this is due to the value of 'current-language-environment' of Emacs.

As you realized, when non-CJK environment such as LANG=en_US.UTF-8,
Emacs prefers charset unicode, and (string-width "…") is 1.
When LANG=ja_JP.UTF-8, Emacs prefers charset japanese-jisx0208,
and (string-width "…") is 2. Type C-u C-x = at "…" to show
charset/display information in Emacs.

@Boruch-Baum
Copy link
Contributor Author

Boruch-Baum commented Feb 25, 2019 via email

@Boruch-Baum
Copy link
Contributor Author

Boruch-Baum commented Feb 25, 2019 via email

@yamaoka
Copy link
Contributor

yamaoka commented Feb 27, 2019

I recognized what was w3m-db-history for me.  That is:

・When I used (very rarely) it, I was using the tool bar button
 because no key is bound to the command by default (lynx-like map).
・Not specifying start and size caused no inconvenience because
 the arrived-db size is not so large if it is the default (500).

If anything, it is annoying if it always prompts me for the arguments, and perhaps I might not understand the necessity to improve the function.  But does't it (troublesomeness of requiring to enter the arguments) go for almost users?  That is why I brought up the example (xxx-w3m-db-history) that requires user's inputs only if the prefix argument is given and called interactively.

@yamaoka
Copy link
Contributor

yamaoka commented Feb 27, 2019

@tats

The width of "…" is ambiguous.

Thanks.  So, instead of using <table>...</table>, it might possibly be better to generate the whole db-history contents by Emacs with the well aligned indentation, surround it with <pre>...</pre>, and pass to "w3m -halfdump".  Furthermore, a way something like the following might be able to use in order to re-indent the Time/Date column after rendering the contents (as w3m-db-history-fix-indentation does it now).

(let ((buffer (get-buffer-create "*testing*"))
      (cur (selected-window)))
  (switch-to-buffer-other-window buffer)
  (erase-buffer)
  (insert "foo..."
          (propertize " " 'display `(space :align-to 40))
          "bar\n"
          "寿限無寿限無五劫の擦り切れ…"
          (propertize " " 'display `(space :align-to 40))
          "長久命の長助\n")
  (select-window cur))

@Boruch-Baum
Copy link
Contributor Author

Boruch-Baum commented Feb 27, 2019 via email

@yamaoka
Copy link
Contributor

yamaoka commented Mar 1, 2019

But does't it (troublesomeness of requiring to enter the
arguments) go for almost users?

Yes.

So, how about the new implementation (already pushed to master)?

(defun w3m-db-history (&optional start size)
  "[...]
If this function is called interactively with the prefix argument,
prompt a user for START and SIZE if the prefix argument is not a
number (i.e., `C-u').  Otherwise if the prefix argument is a number
(i.e., `C-u NUM'), use it as START and leave SIZE nil, that will be
overridden by `w3m-db-history-display-size' or 0."
  (interactive "P")

But then we need another keybinding, or to change another keybinding. Currently, 'h' is display's a buffer's history, and 'H' navigates to a user's home page. I never use 'H' (I use my bookmark page instead), but others might use it a lot.

I think the keys "h", "j", "k", "l", "J", and "K" in the default keymap 'w3m-lynx-like-map' are unnecessary for Emacs users.  So, I think we can occupy "h" and "H" (or "H" and "h"?) for 'w3m-history' and 'w3m-db-history' respectively.  How about bringing up such a proposal to the list?

@Boruch-Baum
Copy link
Contributor Author

Boruch-Baum commented Mar 1, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants