bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

[Visuwesh]

saveplace-pdf-view package [1] uses a custom "place" format to save and
restore the last position for PDF files viewed via doc-view package.

One such "place" is

    ("/home/viz/tmp/piron1976.pdf"
      (pdf-view-bookmark "piron1976.pdf"
                 (filename . "~/tmp/piron1976.pdf")
                 (position . 1)
                 (last-modified 25805 5369 546130 641000)
                 (page . 2)
                 (slice)
                 (size . fit-width)
                 (origin)
                 (handler . pdf-view-bookmark-jump-handler)))

This fails with the following backtrace because 
(cdr (pdf-view-bookmark ...) is a list,

Debugger entered--Lisp error: (wrong-type-argument stringp ("piron1976.pdf" (filename . "~/tmp/piron1976.pdf") (position . 1) (last-modified 25805 5369 546130 641000) (page . 2) (slice) (size . fit-width) (origin) (handler . pdf-view-bookmark-jump-handler)))
  expand-file-name(("piron1976.pdf" (filename . "~/tmp/piron1976.pdf") (position . 1) (last-modified 25805 5369 546130 641000) (page . 2) (slice) (size . fit-width) (origin) (handler . pdf-view-bookmark-jump-handler)))
  #f(compiled-function (sym val) #<bytecode 0x44095c5cb302811>)(save-place-abbreviate-file-names nil)
  custom-initialize-reset(save-place-abbreviate-file-names (funcall #'#f(compiled-function () #<bytecode 0x19800016fe914>)))
  custom-declare-variable(save-place-abbreviate-file-names (funcall #'#f(compiled-function () #<bytecode 0x19800016fe914>)) "If non-nil, abbreviate file names before saving th..." :type boolean :set #f(compiled-function (sym val) #<bytecode 0x44095c5cb302811>) :version "28.1")
  byte-code("\300\301\302\303\304DD\305\306\307\310\311\312\313&\11\210\300\314\302\303\315DD\316\306\307%\210\300\317\302\303\320DD\321\306\322%\210\300\323\302\303\324DD\325\312..." [custom-declare-variable save-place-abbreviate-file-names funcall function #f(compiled-function () #<bytecode 0x19800016fe914>) "If non-nil, abbreviate file names before saving th..." :type boolean :set #f(compiled-function (sym val) #<bytecode 0x44095c5cb302811>) :version "28.1" save-place-save-skipped #f(compiled-function () #<bytecode 0x19800016fe5d4>) "If non-nil, remember files matching `save-place-sk..." save-place-skip-check-regexp #f(compiled-function () #<bytecode -0x7810fef85890ea4>) "Regexp whose file names shall not be checked for r..." regexp save-place-ignore-files-regexp #f(compiled-function ()
  #<bytecode -0x6b951d2e7268727>) "Regexp matching files for which no position should..." "24.1"] 10)
  require(saveplace)
  eval-buffer(#<buffer  *load*> nil "/home/viz/lib/emacs/straight/build/saveplace-pdf-v..." nil t)  ; Reading at buffer position 1694
  load-with-code-conversion("/home/viz/lib/emacs/straight/build/saveplace-pdf-v..." "/home/viz/lib/emacs/straight/build/saveplace-pdf-v..." nil t)
  require(saveplace-pdf-view)
  (progn (require 'saveplace-pdf-view))
  eval((progn (require 'saveplace-pdf-view)) t)
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  (eros--eval-overlay (eval-last-sexp eval-last-sexp-arg-internal) (point))
  eros-eval-last-sexp(nil)
  funcall-interactively(eros-eval-last-sexp nil)
  call-interactively(eros-eval-last-sexp nil nil)
  command-execute(eros-eval-last-sexp)

Checking if the cdr of that list is a string before calling
expand-file-name/abbreviate-file-name fixes the issue for me.

Will attach patch once bug number is generated.

1. https://github.com/nicolaisingh/saveplace-pdf-view


In GNU Emacs 30.0.50 (build 3, x86_64-pc-linux-gnu, X toolkit, Xaw
 scroll bars) of 2023-08-04 built on astatine
Repository revision: 60e5f212182ca2f41f89a4315075e38433bc8ac0
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux trixie/sid

Configured using:
 'configure --with-sound=alsa --with-x-toolkit=lucid --with-json
 --without-xaw3d --without-gconf --without-libsystemd --without-cairo'
Configured features:
ACL DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG JSON
LIBOTF LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XFT
XIM XINPUT2 XPM LUCID ZLIB
Important settings:
  value of $LC_MONETARY: ta_IN.UTF-8
  value of $LC_NUMERIC: ta_IN.UTF-8
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Ement-Room:Not-syncing

Minor modes in effect:
  buffer-face-mode: t
  recentf-mode: t
  server-mode: t
  eros-mode: t
  pdf-occur-global-minor-mode: t
  vz/random-frame-background-mode: t
  minibuffer-depth-indicate-mode: t
  repeat-mode: t
  display-time-mode: t
  display-battery-mode: t
  delete-selection-mode: t
  xterm-mouse-mode: t
  emacs-gc-stats-mode: t
  straight-use-package-mode: t
  straight-package-neutering-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  use-hard-newlines: t
  tab-bar-history-mode: t
  tab-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  undelete-frame-mode: t
  buffer-read-only: t
  visual-line-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow flyspell ispell ecomplete dabbrev emacsbug tramp-cmds log-edit
vc-dir whitespace cus-start cc-mode-expansions cc-mode cc-fonts cc-guess
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs ffap
find-dired grep wdired gnus-dired ement-tabulated-room-list ement
ement-notify ement-room dns ement-room-list ement-lib ement-api
ement-structs plz ement-macros taxy-magit-section magit-section
benchmark taxy svg-lib persist xref mule-util inspector edebug sort
gnus-cite mm-archive mail-extr textsec uni-scripts idna-mapping
ucs-normalize uni-confusable textsec-check gnus-async gnus-bcklg gnus-ml
network-stream nsm nndraft nnmh nndoc nnmaildir nnagent nnml nnnil
gnus-agent gnus-srvr gnus-score score-mode nnvirtual gnus-msg gnus-art
mm-uu mml2015 mm-view mml-smime smime gnutls dig nntp gnus-cache
gnus-sum shr pixel-fill kinsoku url-file svg gnus-group gnus-undo
gnus-start gnus-dbus gnus-cloud nnimap nnmail mail-source utf7 nnoo
gnus-spec gnus-int gnus-range message sendmail yank-media puny rfc822
mml mml-sec epa epg rfc6068 epg-config mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils
mailheader gnus-win gnus nnheader gnus-util mail-utils range mm-util
mail-prsvr sh-script smie treesit executable smerge-mode add-log
expand-region text-mode-expansions er-basic-expansions
expand-region-core expand-region-custom bug-reference help-fns
radix-tree cl-print pulse color avy misearch multi-isearch
shell-command+ face-remap reveal noutline outline tramp-cache time-stamp
tramp-sh tramp trampver tramp-integration files-x tramp-message
tramp-compat xdg shell pcomplete parse-time iso8601 tramp-loaddefs
recentf tree-widget vc-backup log-view pcvs-util vc diff vc-git
diff-mode vc-dispatcher typo inline cursor-sensor server paredit edmacro
kmacro eros checkdoc lisp-mnt flymake-proc flymake project warnings
thingatpt notifications time-date wordel-autoloads mines-autoloads
sokoban-autoloads ement-autoloads svg-lib-autoloads
taxy-magit-section-autoloads magit-section-autoloads dash-autoloads
taxy-autoloads persist-autoloads plz-autoloads nov-autoloads
esxml-autoloads kv-autoloads transmission-autoloads csv-mode-autoloads
lua-mode-autoloads nix-mode-autoloads gnuplot-autoloads
go-mode-autoloads racket-mode-autoloads eros-autoloads
writegood-mode-autoloads siege-mode-autoloads paredit-autoloads
puni-autoloads expand-region-autoloads filladapt-autoloads compose
scroll-other-window org-pdftools-autoloads org-noter-autoloads
change-env-autoloads math-delimiters-autoloads doct-autoloads
ob-async-autoloads async-autoloads emacs-ob-racket-autoloads
valign-autoloads cdlatex-autoloads auctex-autoloads tex-site tempo
pdf-occur ibuf-ext ibuffer ibuffer-loaddefs tablist advice
tablist-filter semantic/wisent/comp semantic/wisent
semantic/wisent/wisent semantic/util-modes semantic/util semantic
semantic/tag semantic/lex semantic/fw mode-local cedet pdf-isearch
let-alist pdf-misc imenu pdf-tools package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source eieio eieio-core json url-vars compile comint ansi-osc
ansi-color ring cus-edit wid-edit pdf-view password-cache bookmark
text-property-search jka-compr pdf-cache pdf-info tq pdf-util pdf-macs
image-mode exif pdf-tools-autoloads tablist-autoloads typo-autoloads
mb-depth repeat visual-fill-autoloads olivetti-autoloads time
format-spec battery filenotify dom tamil99 quail disp-table
lacarte-autoloads shell-command-plus-autoloads icons delsel xt-mouse
cus-load avy-autoloads skeleton icalendar diary-lib diary-loaddefs
cal-menu calendar cal-loaddefs dired-du-autoloads finder-inf dired-x
filecache imenu-xref-autoloads ert map byte-opt pp ewoc debug backtrace
find-func dbus xml derived chemtable-autoloads molar-mass-autoloads
vc-backup-autoloads compat-autoloads saveplace-pdf-view-autoloads rx
inspector-autoloads xr-autoloads emacs-gc-stats easy-mmode pcase
dired-aux dired dired-loaddefs emacs-gc-stats-autoloads
straight-autoloads cl-seq info cl-extra help-mode straight subr-x
cl-macs gv cl-loaddefs cl-lib bytecomp byte-compile vz-nh-theme
vz-options-theme rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting x-toolkit
xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 983800 273359) (symbols 48 42908 28)
 (strings 32 226910 9992) (string-bytes 1 12254797)
 (vectors 16 104106) (vector-slots 8 2152690 189597)
 (floats 8 895 10919) (intervals 56 24419 1685) (buffers 976 78))

bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

[Visuwesh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Only-abbreviate-expand-strings-when-adjusting-savepl.patch --]
[-- Type: text/x-diff, Size: 1209 bytes --]

From 92f9d2b43e1974bf1496e436ad49d3b687ee0d3f Mon Sep 17 00:00:00 2001
From: Visuwesh <visuweshm@gmail.com>
Date: Fri, 4 Aug 2023 21:47:11 +0530
Subject: [PATCH] Only abbreviate/expand strings when adjusting saveplace

* lisp/saveplace.el (save-place-abbreviate-file-names): Only call
abbreviate-file-name/expand-file-name on strings in :set function.
(bug#65055)
---
 lisp/saveplace.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 18d296ba2d..6386791337 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -154,7 +154,9 @@ save-place-abbreviate-file-names
                                  (if (listp v)
                                      (cl-loop for (k1 . v1) in v
                                               collect
-                                              (cons k1 (funcall fun v1)))
+                                              (cons k1 (if (stringp v1)
+                                                           (funcall fun v1)
+                                                         v1)))
                                    v)))
                   :key #'car
                   :from-end t
-- 
2.40.1

bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

[Eli Zaretskii]

> From: Visuwesh <visuweshm@gmail.com>
> Date: Fri, 04 Aug 2023 21:45:04 +0530
> 
> saveplace-pdf-view package [1] uses a custom "place" format to save and
> restore the last position for PDF files viewed via doc-view package.
> 
> One such "place" is
> 
>     ("/home/viz/tmp/piron1976.pdf"
>       (pdf-view-bookmark "piron1976.pdf"
>                  (filename . "~/tmp/piron1976.pdf")
>                  (position . 1)
>                  (last-modified 25805 5369 546130 641000)
>                  (page . 2)
>                  (slice)
>                  (size . fit-width)
>                  (origin)
>                  (handler . pdf-view-bookmark-jump-handler)))
> 
> This fails with the following backtrace because 
> (cdr (pdf-view-bookmark ...) is a list,

I don't understand: the valid format of save-place-alist is documented
in its doc string, so this looks like a bug in saveplace-pdf-view
package?  Or what am I missing?

bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

[Visuwesh]

[வெள்ளி ஆகஸ்ட் 04, 2023] Eli Zaretskii wrote:

>> From: Visuwesh <visuweshm@gmail.com>
>> Date: Fri, 04 Aug 2023 21:45:04 +0530
>> 
>> saveplace-pdf-view package [1] uses a custom "place" format to save and
>> restore the last position for PDF files viewed via doc-view package.
>> 
>> One such "place" is
>> 
>>     ("/home/viz/tmp/piron1976.pdf"
>>       (pdf-view-bookmark "piron1976.pdf"
>>                  (filename . "~/tmp/piron1976.pdf")
>>                  (position . 1)
>>                  (last-modified 25805 5369 546130 641000)
>>                  (page . 2)
>>                  (slice)
>>                  (size . fit-width)
>>                  (origin)
>>                  (handler . pdf-view-bookmark-jump-handler)))
>> 
>> This fails with the following backtrace because 
>> (cdr (pdf-view-bookmark ...) is a list,
>
> I don't understand: the valid format of save-place-alist is documented
> in its doc string, so this looks like a bug in saveplace-pdf-view
> package?  Or what am I missing?

Unfortunately, that's only half the story.  save-place-alist can also
have elements such as

    ("SOME-DIRECTORY" (dired-filename . "SOME-FILENAME"))

to save the place for dired buffers.  This is why the :set function of
the user option checks if second element of above place is a list and
also changes the "SOME-FILENAME" to be abbreviated/expanded.

But instead of checking if the second element of the place's cdr is a
string, the :set function simply assumes that it will be always be a
string.  This is not wrong as such since that's the only allowed format
as far as core Emacs is concerned but I think having the check in place
would help expand the save-place-alist format as the user desires.

bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

[Eli Zaretskii]

> From: Visuwesh <visuweshm@gmail.com>
> Cc: 65055@debbugs.gnu.org
> Date: Fri, 04 Aug 2023 23:43:56 +0530
> 
> [வெள்ளி ஆகஸ்ட் 04, 2023] Eli Zaretskii wrote:
> 
> >>     ("/home/viz/tmp/piron1976.pdf"
> >>       (pdf-view-bookmark "piron1976.pdf"
> >>                  (filename . "~/tmp/piron1976.pdf")
> >>                  (position . 1)
> >>                  (last-modified 25805 5369 546130 641000)
> >>                  (page . 2)
> >>                  (slice)
> >>                  (size . fit-width)
> >>                  (origin)
> >>                  (handler . pdf-view-bookmark-jump-handler)))
> >> 
> >> This fails with the following backtrace because 
> >> (cdr (pdf-view-bookmark ...) is a list,
> >
> > I don't understand: the valid format of save-place-alist is documented
> > in its doc string, so this looks like a bug in saveplace-pdf-view
> > package?  Or what am I missing?
> 
> Unfortunately, that's only half the story.  save-place-alist can also
> have elements such as
> 
>     ("SOME-DIRECTORY" (dired-filename . "SOME-FILENAME"))
> 
> to save the place for dired buffers.

Where is this documented, and when the change to support this
extension was added?

And in any way, the form you show above, which is used by
saveplace-pdf-view, is not even of that extended format, or am I again
missing something?

> But instead of checking if the second element of the place's cdr is a
> string, the :set function simply assumes that it will be always be a
> string.  This is not wrong as such since that's the only allowed format
> as far as core Emacs is concerned but I think having the check in place
> would help expand the save-place-alist format as the user desires.

I don't think we should tweak our code to support save-place-alist
formats that violate the documented formats.  If the documented format
is not followed, how do you know which elements of which parts of the
data structures are file names in the first place?

The right way of extending such data structures is to allow functions
as the values, and let those functions deal with non-standard formats.
The way saveplace-pdf-view does it, IIUC, is simply wrong: it just
happened to work by sheer luck with previous versions because Emacs
never actually acted on the assumption that a file name is in a
certain place in the data stricture.

bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

[Visuwesh]

[வெள்ளி ஆகஸ்ட் 04, 2023] Eli Zaretskii wrote:

>> Unfortunately, that's only half the story.  save-place-alist can also
>> have elements such as
>> 
>>     ("SOME-DIRECTORY" (dired-filename . "SOME-FILENAME"))
>> 
>> to save the place for dired buffers.
>
> Where is this documented

It is not.  The only way to know how this comes to be is to read
`save-place-to-alist'.  BTW, another specially handled major-mode is
hexl-mode.

>, and when the change to support this
> extension was added?

In commit d4466a91f271db0b414a605ede1a7befd403b950.

    commit d4466a91f271db0b414a605ede1a7befd403b950
    Author: Ivan Kanis <ivan@kanis.fr>
    Date:   Fri Jun 14 11:32:01 2013 +0200

        Add support for dired in saveplace.


> And in any way, the form you show above, which is used by
> saveplace-pdf-view, is not even of that extended format, or am I again
> missing something?

No, you are not missing anything.

>> But instead of checking if the second element of the place's cdr is a
>> string, the :set function simply assumes that it will be always be a
>> string.  This is not wrong as such since that's the only allowed format
>> as far as core Emacs is concerned but I think having the check in place
>> would help expand the save-place-alist format as the user desires.
>
> I don't think we should tweak our code to support save-place-alist
> formats that violate the documented formats.  If the documented format
> is not followed, how do you know which elements of which parts of the
> data structures are file names in the first place?

I understand your point,

> The right way of extending such data structures is to allow functions
> as the values, and let those functions deal with non-standard formats.
> The way saveplace-pdf-view does it, IIUC, is simply wrong: it just
> happened to work by sheer luck with previous versions because Emacs
> never actually acted on the assumption that a file name is in a
> certain place in the data stricture.

That was my idea two years back and till now but I simply didn't/don't
have the motivation to come up with a patch since I don't use saveplace
for anything but pdf buffers and saveplace-pdf-view did the job good
enough for me to not worry about a better way™.  In the absence of such
motivation, I will solve the problem in saveplace-pdf-view's side and
call it a day.  If/when the motivation returns, I guess we can revisit
this issue.
This report can be closed if you wish.

bug#65055: 30.0.50; save-place-abbreviation-file-names :set fails for saveplace-pdf-view places

[Eli Zaretskii]

> From: Visuwesh <visuweshm@gmail.com>
> Cc: 65055@debbugs.gnu.org
> Date: Sat, 05 Aug 2023 09:07:06 +0530
> 
> [வெள்ளி ஆகஸ்ட் 04, 2023] Eli Zaretskii wrote:
> 
> >> Unfortunately, that's only half the story.  save-place-alist can also
> >> have elements such as
> >> 
> >>     ("SOME-DIRECTORY" (dired-filename . "SOME-FILENAME"))
> >> 
> >> to save the place for dired buffers.
> >
> > Where is this documented
> 
> It is not.  The only way to know how this comes to be is to read
> `save-place-to-alist'.  BTW, another specially handled major-mode is
> hexl-mode.
> 
> >, and when the change to support this
> > extension was added?
> 
> In commit d4466a91f271db0b414a605ede1a7befd403b950.
> 
>     commit d4466a91f271db0b414a605ede1a7befd403b950
>     Author: Ivan Kanis <ivan@kanis.fr>
>     Date:   Fri Jun 14 11:32:01 2013 +0200
> 
>         Add support for dired in saveplace.

Thanks, but actually the format extension was done later, in commit
1d42e5b6396fd7234ff94e6fec7fd4f39d1faddb.

Anyway, what a mess!  I've now fixed the doc strings added by those
two commits, and documented the new format.

> > I don't think we should tweak our code to support save-place-alist
> > formats that violate the documented formats.  If the documented format
> > is not followed, how do you know which elements of which parts of the
> > data structures are file names in the first place?
> 
> I understand your point,
> 
> > The right way of extending such data structures is to allow functions
> > as the values, and let those functions deal with non-standard formats.
> > The way saveplace-pdf-view does it, IIUC, is simply wrong: it just
> > happened to work by sheer luck with previous versions because Emacs
> > never actually acted on the assumption that a file name is in a
> > certain place in the data stricture.
> 
> That was my idea two years back and till now but I simply didn't/don't
> have the motivation to come up with a patch since I don't use saveplace
> for anything but pdf buffers and saveplace-pdf-view did the job good
> enough for me to not worry about a better way™.  In the absence of such
> motivation, I will solve the problem in saveplace-pdf-view's side and
> call it a day.  If/when the motivation returns, I guess we can revisit
> this issue.
> This report can be closed if you wish.

OK, closing after installing the above-mentioned documentation fixes.