bug#58131: [PATCH] docview: new customization options for imenu

bug#58131: [PATCH] docview: new customization options for imenu

[Jose A Ortega Ruiz]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Tags: patch


A follow up to bug#58103 with a little fix for section title extraction
and a couple of easy to implement customizable options.  I've tried to
document them clearly enough in the docstrings and then just mention
their existence in the manual: is that a good practice or do we prefer
some duplication?

Cheers,
jao

In GNU Emacs 29.0.50 (build 20, x86_64-pc-linux-gnu, cairo version
 1.16.0) of 2022-09-27 built on rivendell
Repository revision: 7368cdd359325cb6ed83688178ae4b4eaf22f4d5
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure --prefix=/usr/local/stow/emacs29 --with-x-toolkit=no
 --with-imagemagick -C'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-docview-new-customization-options-for-imenu.patch --]
[-- Type: text/patch, Size: 3979 bytes --]

From a434024c4e452b4f2495921832b00e801568939e Mon Sep 17 00:00:00 2001
From: Jose A Ortega Ruiz <jao@gnu.org>
Date: Wed, 28 Sep 2022 03:02:57 +0100
Subject: [PATCH] docview: new customization options for imenu

* doc/emacs/misc.texi (DocView Navigation):
* lisp/doc-view.el (doc-view-imenu-title-format, doc-view-imenu-flatten):
(doc-view--imenu-subtree): customizable format for imenu entry titles,
and flag to disable nested submenus.
* lisp/doc-view.el (doc-view--pdf-outline): clean up whitespace
markers '\r' and '\t' in imenu item titles.
---
 doc/emacs/misc.texi |  9 +++++++--
 lisp/doc-view.el    | 29 +++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index 04f7f2e921..cef2580f3b 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -585,11 +585,16 @@ DocView Navigation
 @code{doc-view-resolution}.
 
 @vindex doc-view-imenu-enabled
+@vindex doc-view-imenu-flatten
+@vindex doc-view-imenu-format
   When the @command{mutool} program is available, DocView will use it
 to generate entries for an outline menu, making it accessible via the
 @code{imenu} facility (@pxref{Imenu}).  To disable this functionality
-even when @command{mutool} can be found on your system, customize
-the variable @code{doc-view-imenu-enabled} to the @code{nil} value.
+even when @command{mutool} can be found on your system, customize the
+variable @code{doc-view-imenu-enabled} to the @code{nil} value.  You
+can further customize how @code{imenu} items are formatted and
+displayed using the variables @code{doc-view-imenu-format} and
+@code{doc-view-flatten}.
 
 @node DocView Searching
 @subsection DocView Searching
diff --git a/lisp/doc-view.el b/lisp/doc-view.el
index fa583df12b..9e3bb6e46c 100644
--- a/lisp/doc-view.el
+++ b/lisp/doc-view.el
@@ -219,6 +219,23 @@ doc-view-imenu-enabled
   :type 'boolean
   :version "29.1")
 
+(defcustom doc-view-imenu-title-format "%t (%p)"
+  "Format string for document section titles in imenu.
+
+The special markers '%t' and '%p' are replaced by the section
+title and page number in this format string, which uses
+`format-spec'.
+
+For instance, setting this variable to \"%t\" will produce items
+showing only titles and no page number."
+  :type 'string
+  :version "29.1")
+
+(defcustom doc-view-imenu-flatten nil
+  "Whether to generate a flat list of sections instead of a nested tree."
+  :type 'boolean
+  :version "29.1")
+
 (defcustom doc-view-svg-background "white"
   "Background color for svg images.
 See `doc-view-mupdf-use-svg'."
@@ -1898,7 +1915,8 @@ doc-view--pdf-outline
       (goto-char (point-min))
       (while (re-search-forward doc-view--outline-rx nil t)
         (push `((level . ,(length (match-string 1)))
-                (title . ,(match-string 2))
+                (title . ,(replace-regexp-in-string "\\\\[rt]" " "
+                                                    (match-string 2)))
                 (page . ,(string-to-number (match-string 3))))
               outline)))
     (nreverse outline)))
@@ -1911,11 +1929,14 @@ doc-view--imenu-subtree
 level.  Returns that imenu alist together with any other pending outline
 entries at an upper level."
   (let ((level (alist-get 'level (car outline)))
+        (nested (not doc-view-imenu-flatten))
         (index nil))
-    (while (and (car outline) (<= level (alist-get 'level (car outline))))
+    (while (and (car outline)
+                (or nested (<= level (alist-get 'level (car outline)))))
       (let-alist (car outline)
-        (let ((title (format "%s (%s)" .title .page)))
-          (if (> .level level)
+        (let ((title (format-spec doc-view-imenu-title-format
+                                  `((?t . ,.title) (?p . ,.page)))))
+          (if (and nested (> .level level))
               (let ((sub (doc-view--imenu-subtree outline act))
                     (fst (car index)))
                 (setq index (cdr index))
-- 
2.37.2


[-- Attachment #3: Type: text/plain, Size: 130 bytes --]


-- 
Too often we enjoy the comfort of opinion without the discomfort of
thought. -John F. Kennedy, 35th US president (1917-1963)

bug#58131: [PATCH] docview: new customization options for imenu

[Lars Ingebrigtsen]

Jose A Ortega Ruiz <jao@gnu.org> writes:

> A follow up to bug#58103 with a little fix for section title extraction
> and a couple of easy to implement customizable options.  I've tried to
> document them clearly enough in the docstrings and then just mention
> their existence in the manual: is that a good practice or do we prefer
> some duplication?

It varies -- we don't want the manual to grow unbounded, so sometimes we
just mention variables if they're auxiliary things like this.  So I
think you have the right balance here, and I pushed it to Emacs 29.

bug#58131: [PATCH] docview: new customization options for imenu

[Eli Zaretskii]

> From: Jose A Ortega Ruiz <jao@gnu.org>
> Date: Wed, 28 Sep 2022 03:21:31 +0100
> 
> A follow up to bug#58103 with a little fix for section title extraction
> and a couple of easy to implement customizable options.  I've tried to
> document them clearly enough in the docstrings and then just mention
> their existence in the manual: is that a good practice or do we prefer
> some duplication?

It's a possibility.  But if you have nothing of essence to say about a
variable, why mention it in the manual at all?  We don't need to
mention every user option in the manual, only the important ones.  We
expect users who want to use a package to review its options (e.g., bu
"M-x customize-group") and decide which ones they want to change from
the default.

> +(defcustom doc-view-imenu-title-format "%t (%p)"
> +  "Format string for document section titles in imenu.
> +
> +The special markers '%t' and '%p' are replaced by the section
> +title and page number in this format string, which uses
> +`format-spec'.

Will users immediately understand what you mean by "document section
title" here?  If no, perhaps a sentence explaining what that is would
be beneficial.

> +(defcustom doc-view-imenu-flatten nil
> +  "Whether to generate a flat list of sections instead of a nested tree."

This doesn't mention imenu in the doc string; should it?

bug#58131: [PATCH] docview: new customization options for imenu

[Jose A Ortega Ruiz]

On Wed, Sep 28 2022, Eli Zaretskii wrote:

>> From: Jose A Ortega Ruiz <jao@gnu.org>
>> Date: Wed, 28 Sep 2022 03:21:31 +0100
>> 
>> A follow up to bug#58103 with a little fix for section title extraction
>> and a couple of easy to implement customizable options.  I've tried to
>> document them clearly enough in the docstrings and then just mention
>> their existence in the manual: is that a good practice or do we prefer
>> some duplication?
>
> It's a possibility.  But if you have nothing of essence to say about a
> variable, why mention it in the manual at all?  We don't need to
> mention every user option in the manual, only the important ones.  We
> expect users who want to use a package to review its options (e.g., bu
> "M-x customize-group") and decide which ones they want to change from
> the default.

That makes sense.  In this case, there's just 3 variables related to the
functionality, and my impression is that they provide options that users
often will want... which might be taken yet as an argument for /not/
adding them to the manual, now that i think of it :)

>> +(defcustom doc-view-imenu-title-format "%t (%p)"
>> +  "Format string for document section titles in imenu.
>> +
>> +The special markers '%t' and '%p' are replaced by the section
>> +title and page number in this format string, which uses
>> +`format-spec'.
>
> Will users immediately understand what you mean by "document section
> title" here?  If no, perhaps a sentence explaining what that is would
> be beneficial.

I would say they will: in the context of a docview imenu for a PDF
document, there's little else it could reasonably be.  But i'm biased: do
you think otherwise?

>> +(defcustom doc-view-imenu-flatten nil
>> +  "Whether to generate a flat list of sections instead of a nested tree."
>
> This doesn't mention imenu in the doc string; should it?

Given that the name of the variable does, i think mentioning it would
just make that first sentence longer without adding too much
information.  But again, as the implementor of the functionality,
everything feels "obvious" to me.

Thanks,
jao
-- 
Lying to ourselves is more deeply ingrained than lying to
others. -Fyodor Dostoevsky, novelist (1821-1881)

bug#58131: [PATCH] docview: new customization options for imenu

[Eli Zaretskii]

> From: Jose A Ortega Ruiz <jao@gnu.org>
> Cc: 58131@debbugs.gnu.org
> Date: Wed, 28 Sep 2022 15:05:50 +0100
> 
> >> +(defcustom doc-view-imenu-title-format "%t (%p)"
> >> +  "Format string for document section titles in imenu.
> >> +
> >> +The special markers '%t' and '%p' are replaced by the section
> >> +title and page number in this format string, which uses
> >> +`format-spec'.
> >
> > Will users immediately understand what you mean by "document section
> > title" here?  If no, perhaps a sentence explaining what that is would
> > be beneficial.
> 
> I would say they will: in the context of a docview imenu for a PDF
> document, there's little else it could reasonably be.  But i'm biased: do
> you think otherwise?

FWIW, I couldn't understand what that means.

How does being in the context of docview imenu for a PDF document help
understanding that here?  "Document section title" is general enough
terminology.  Using a "construct state" here doesn't help, either.

> >> +(defcustom doc-view-imenu-flatten nil
> >> +  "Whether to generate a flat list of sections instead of a nested tree."
> >
> > This doesn't mention imenu in the doc string; should it?
> 
> Given that the name of the variable does, i think mentioning it would
> just make that first sentence longer without adding too much
> information.  But again, as the implementor of the functionality,
> everything feels "obvious" to me.

My suggestion is

  Whether to flatten the list of sections in an imenu or show it nested.

bug#58131: [PATCH] docview: new customization options for imenu

[Jose A Ortega Ruiz]

On Wed, Sep 28 2022, Eli Zaretskii wrote:

>> From: Jose A Ortega Ruiz <jao@gnu.org>
>> Cc: 58131@debbugs.gnu.org
>> Date: Wed, 28 Sep 2022 15:05:50 +0100
>> 
>> >> +(defcustom doc-view-imenu-title-format "%t (%p)"
>> >> +  "Format string for document section titles in imenu.
>> >> +
>> >> +The special markers '%t' and '%p' are replaced by the section
>> >> +title and page number in this format string, which uses
>> >> +`format-spec'.
>> >
>> > Will users immediately understand what you mean by "document section
>> > title" here?  If no, perhaps a sentence explaining what that is would
>> > be beneficial.
>> 
>> I would say they will: in the context of a docview imenu for a PDF
>> document, there's little else it could reasonably be.  But i'm biased: do
>> you think otherwise?
>
> FWIW, I couldn't understand what that means.
>
> How does being in the context of docview imenu for a PDF document help
> understanding that here?  "Document section title" is general enough
> terminology.  Using a "construct state" here doesn't help, either.

To me, the context is that this is read by a user of the functionality,
wanting to fine-tune it (i rarely start exploring a functionality by
reading the docstring of one of its customizable variables).

Barring that: the Imenu section of the manual calls the items being
formatted here simply "definitions".  If we assume that the user knows
about imenu in general, one could have:

   "Format string for the imenu definitions extracted from documents."

Or, perhaps, trying to provide the missing context for not-yet-users:

   "Format string for the section titles extracted by imenu from docview documents."

Any better?

>> >> +(defcustom doc-view-imenu-flatten nil
>> >> +  "Whether to generate a flat list of sections instead of a nested tree."
>> >
>> > This doesn't mention imenu in the doc string; should it?
>> 
>> Given that the name of the variable does, i think mentioning it would
>> just make that first sentence longer without adding too much
>> information.  But again, as the implementor of the functionality,
>> everything feels "obvious" to me.
>
> My suggestion is
>
>   Whether to flatten the list of sections in an imenu or show it nested.

Sounds better to me too, yes.

Thanks,
jao
-- 
Nothing so soothes our vanity as a display of greater vanity in
others; it makes us vain, in fact, of our modesty.
  -Louis Kronenberger, writer (1904-1980)

bug#58131: [PATCH] docview: new customization options for imenu

[Eli Zaretskii]

> From: Jose A Ortega Ruiz <jao@gnu.org>
> Cc: 58131@debbugs.gnu.org
> Date: Wed, 28 Sep 2022 16:14:13 +0100
> 
> >> I would say they will: in the context of a docview imenu for a PDF
> >> document, there's little else it could reasonably be.  But i'm biased: do
> >> you think otherwise?
> >
> > FWIW, I couldn't understand what that means.
> >
> > How does being in the context of docview imenu for a PDF document help
> > understanding that here?  "Document section title" is general enough
> > terminology.  Using a "construct state" here doesn't help, either.
> 
> To me, the context is that this is read by a user of the functionality,
> wanting to fine-tune it (i rarely start exploring a functionality by
> reading the docstring of one of its customizable variables).

Think about the various Help commands, such as 'apropos': they show
the doc strings or just their first sentence entirely out of any
context.  Users who invoke such commands should be capable to
understand at a glance whether the command/variable is something they
should examine further.  So having important keywords in there helps
immensely to make such triage steps much faster and more efficient.

> Barring that: the Imenu section of the manual calls the items being
> formatted here simply "definitions".  If we assume that the user knows
> about imenu in general, one could have:
> 
>    "Format string for the imenu definitions extracted from documents."
> 
> Or, perhaps, trying to provide the missing context for not-yet-users:
> 
>    "Format string for the section titles extracted by imenu from docview documents."
> 
> Any better?

Yes, thanks.  My suggestion is a very minor variation of the latter:

 Format spec for imenu's display of section titles from docview documents.

bug#58131: [PATCH] docview: new customization options for imenu

[Jose A Ortega Ruiz]

On Wed, Sep 28 2022, Eli Zaretskii wrote:

[...]

> Think about the various Help commands, such as 'apropos': they show
> the doc strings or just their first sentence entirely out of any
> context.  Users who invoke such commands should be capable to
> understand at a glance whether the command/variable is something they
> should examine further.  So having important keywords in there helps
> immensely to make such triage steps much faster and more efficient.

Excellent point, thanks.

[...]

>> Any better?
>
> Yes, thanks.  My suggestion is a very minor variation of the latter:
>
>  Format spec for imenu's display of section titles from docview documents.

Looks good to me.  Would you like a patch with the two new strings or is
it quicker if you directly commit a change by yourself?


Cheers,
jao
-- 
All parts should go together without forcing. You must remember that
the parts you are reassembling were disassembled by you. Therefore, if
you can’t get them together again, there must be a reason. By all
means, do not use a hammer. —IBM Manual, 1925

bug#58131: [PATCH] docview: new customization options for imenu

[Eli Zaretskii]

> From: Jose A Ortega Ruiz <jao@gnu.org>
> Cc: 58131@debbugs.gnu.org
> Date: Wed, 28 Sep 2022 16:55:56 +0100
> 
> Looks good to me.  Would you like a patch with the two new strings or is
> it quicker if you directly commit a change by yourself?

No need, I made these changes already.

Thanks for the feedback.