GNU bug report logs - #61394
30.0.50; [PATCH] Image-dired thumb name based on content

Previous Next

Package: emacs;

Reported by: Manuel Giraud <manuel <at> ledu-giraud.fr>

Date: Thu, 9 Feb 2023 19:08:02 UTC

Severity: normal

Tags: patch

Found in version 30.0.50

Done: Eli Zaretskii <eliz <at> gnu.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61394 in the body.
You can then email your comments to 61394 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 09 Feb 2023 19:08:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Manuel Giraud <manuel <at> ledu-giraud.fr>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 09 Feb 2023 19:08:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Image-dired thumb name based on content
Date: Thu, 09 Feb 2023 20:06:50 +0100
[Message part 1 (text/plain, inline)]
Hi,

Here is a proposal for a modification of how image-dired generates thumb
names.  With this patch, a thumb name will depend on the content of the
associated image.

Pros:
        - the thumb name is unique even if the file is moved or renamed
        - you keep a centralized thumb directory
Cons:
        - none (as it fits my usage :-)

[0001-Image-dired-thumb-name-based-on-content.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]

In GNU Emacs 30.0.50 (build 1, x86_64-unknown-openbsd7.2, cairo version
 1.17.6) of 2023-02-09 built on computer
Repository revision: 1518fc5d7c5bedbbe35053696c7ec06020c81b05
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: OpenBSD computer 7.2 GENERIC.MP#933 amd64

Configured using:
 'configure --prefix=/home/manuel/emacs --bindir=/home/manuel/bin
 --with-x-toolkit=no --without-sound --without-compress-install
 CPPFLAGS=-I/usr/local/include LDFLAGS=-L/usr/local/lib'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBOTF LIBXML2 MODULES NOTIFY KQUEUE OLDXMENU PDUMPER PNG RSVG
SQLITE3 THREADS TIFF TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM ZLIB

Important settings:
  value of $LC_ALL: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Dired by name

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  gnus-dired-mode: t
  display-time-mode: t
  display-battery-mode: t
  server-mode: t
  shell-dirtrack-mode: t
  repeat-mode: t
  desktop-save-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  buffer-read-only: t
  line-number-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:
/home/manuel/.emacs.d/elpa/ef-themes-0.10.0/theme-loaddefs hides /home/manuel/emacs/share/emacs/30.0.50/lisp/theme-loaddefs
/home/manuel/.emacs.d/elpa/transient-0.3.7/transient hides /home/manuel/emacs/share/emacs/30.0.50/lisp/transient

Features:
(shadow sort mail-extr macros emacsbug whitespace magit-patch wdired
magit-extras face-remap magit-bookmark magit-submodule magit-obsolete
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func magit-diff smerge-mode diff
git-commit log-edit pcvs-util magit-core magit-autorevert magit-margin
magit-transient magit-process with-editor magit-mode transient magit-git
magit-section benchmark shortdoc executable dabbrev find-dired ffap
tabify cus-start image-dired-dired pulse misearch multi-isearch cl-print
help-fns radix-tree magit-utils dash image-file image-converter
image-dired image-dired-tags image-dired-external image-dired-util
org-indent org-element org-persist org-id org-refile avl-tree oc-basic
ol-eww ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect ol-docview
ol-bibtex bibtex ol-bbdb ol-w3m ol-doi org-link-doi texinfo
texinfo-loaddefs reveal rng-xsd xsd-regexp rng-cmpct rng-nxml rng-valid
rng-loc rng-uri rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn
nxml-ns nxml-mode nxml-outln nxml-rap nxml-util nxml-enc xmltok css-mode
treesit smie sgml-mode facemenu imenu eww xdg url-queue mm-url doc-view
jka-compr image-mode exif view pascal bug-reference paredit edmacro
vc-hg vc-svn conf-mode vc-dir ewoc vc autorevert filenotify vc-git
diff-mode vc-dispatcher add-log gnus-dired time battery exwm-randr
xcb-randr exwm-config exwm exwm-input xcb-keysyms xcb-xkb exwm-manage
exwm-floating xcb-cursor xcb-render exwm-layout exwm-workspace exwm-core
xcb-ewmh xcb-icccm xcb xcb-xproto xcb-types xcb-debug kmacro server
stimmung-themes modus-operandi-theme modus-themes ytdious osm mingus
libmpdee reporter edebug debug backtrace transmission diary-lib
diary-loaddefs color calc-bin calc-ext calc calc-loaddefs rect calc-macs
w3m-load mu4e mu4e-org mu4e-main mu4e-view mu4e-headers mu4e-compose
mu4e-draft mu4e-actions smtpmail mu4e-search mu4e-lists mu4e-bookmarks
mu4e-mark mu4e-message flow-fill mule-util hl-line mu4e-contacts
mu4e-update mu4e-folders mu4e-server mu4e-context mu4e-vars mu4e-helpers
mu4e-config bookmark ido supercite regi ebdb-message ebdb-gnus gnus-msg
gnus-art mm-uu mml2015 mm-view mml-smime smime gnutls dig gnus-sum shr
pixel-fill kinsoku url-file svg dom 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 gmm-utils mailheader gnus-win gnus
nnheader gnus-util mail-utils range mm-util mail-prsvr ebdb-mua ebdb-com
crm ebdb-format ebdb mailabbrev eieio-opt cl-extra help-mode speedbar
ezimage dframe eieio-base pcase timezone org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src ob-comint org-pcomplete org-list
org-footnote org-faces org-entities ob-emacs-lisp ob-core ob-eval
org-cycle org-table ol org-fold org-fold-core org-keys oc org-loaddefs
find-func cal-menu calendar cal-loaddefs org-version org-compat org-macs
visual-basic-mode cl web-mode derived disp-table erlang-start
smart-tabs-mode skeleton cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs slime-asdf grep slime-tramp
tramp rx tramp-loaddefs trampver tramp-integration cus-edit cus-load
wid-edit files-x tramp-compat shell pcomplete parse-time iso8601
time-date ls-lisp format-spec slime-fancy slime-indentation
slime-cl-indent cl-indent slime-trace-dialog slime-fontifying-fu
slime-package-fu slime-references slime-compiler-notes-tree advice
slime-scratch slime-presentations bridge slime-macrostep macrostep
slime-mdot-fu slime-enclosing-context slime-fuzzy slime-fancy-trace
slime-fancy-inspector slime-c-p-c slime-editing-commands slime-autodoc
slime-repl slime-parse slime apropos compile text-property-search etags
fileloop generator xref project arc-mode archive-mode noutline outline
icons pp comint ansi-osc ansi-color ring hyperspec thingatpt
slime-autoloads dired-aux dired-x dired dired-loaddefs notifications
dbus xml repeat easy-mmode desktop frameset stimmung-themes-autoloads
rust-mode-autoloads ebdb-autoloads magit-autoloads debbugs-autoloads
git-commit-autoloads magit-section-autoloads ef-themes-autoloads
with-editor-autoloads paredit-autoloads dash-autoloads ytdious-autoloads
transmission-autoloads transient-autoloads exwm-autoloads
hyperbole-autoloads detached-autoloads info 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 cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib 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 kqueue lcms2 dynamic-setting system-font-setting
font-render-setting cairo xinput2 x multi-tty make-network-process
emacs)

Memory information:
((conses 16 890208 177320)
 (symbols 48 60888 5)
 (strings 32 208408 17024)
 (string-bytes 1 6407691)
 (vectors 16 117900)
 (vector-slots 8 2299033 117223)
 (floats 8 645 483)
 (intervals 56 40488 2510)
 (buffers 984 144))

-- 
Manuel Giraud

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 10 Feb 2023 15:14:02 GMT) Full text and rfc822 format available.

Message #8 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Basil Contovounesios <contovob <at> tcd.ie>
To: 61394 <at> debbugs.gnu.org
Cc: Manuel Giraud <manuel <at> ledu-giraud.fr>
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 10 Feb 2023 15:13:27 +0000
Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of text editors" [2023-02-09 20:06 +0100] wrote:

> +(defun image-dired-content-sha1 (filename)
> +  "Compute the SHA-1 of a part of FILENAME."
> +  (with-temp-buffer
> +    (let ((file-size (file-attribute-size (file-attributes filename)))
> +	  (chunk-size 4096))
> +      (insert-file-contents filename nil 0 (min chunk-size file-size))

Can't we unconditionally pass END=chunk-size to insert-file-contents,
even for smaller files?

Thanks,

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 10 Feb 2023 18:47:02 GMT) Full text and rfc822 format available.

Message #11 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Basil Contovounesios <contovob <at> tcd.ie>
Cc: 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 10 Feb 2023 19:46:02 +0100
[Message part 1 (text/plain, inline)]
Basil Contovounesios <contovob <at> tcd.ie> writes:

> Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" [2023-02-09 20:06 +0100] wrote:
>
>> +(defun image-dired-content-sha1 (filename)
>> +  "Compute the SHA-1 of a part of FILENAME."
>> +  (with-temp-buffer
>> +    (let ((file-size (file-attribute-size (file-attributes filename)))
>> +	  (chunk-size 4096))
>> +      (insert-file-contents filename nil 0 (min chunk-size file-size))
>
> Can't we unconditionally pass END=chunk-size to insert-file-contents,
> even for smaller files?

From fileio.c:4076, it seems that you are right:

--8<---------------cut here---------------start------------->8---
      /* The likely offset where we will stop reading.  We could read
	 more (or less), if the file grows (or shrinks) as we read it.  */
      off_t likely_end = min (end_offset, st.st_size);
--8<---------------cut here---------------end--------------->8---

So here is an update version of this patch.  I've tested it on small 400
bytes icons and it works also.
[0001-Image-dired-thumb-name-based-on-content.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
Thanks,
-- 
Manuel Giraud

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 11 Feb 2023 09:52:02 GMT) Full text and rfc822 format available.

Message #14 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50;
 [PATCH] Image-dired thumb name based on content
Date: Sat, 11 Feb 2023 11:50:33 +0200
> Cc: 61394 <at> debbugs.gnu.org
> Date: Fri, 10 Feb 2023 19:46:02 +0100
> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> +(defun image-dired-content-sha1 (filename)
> +  "Compute the SHA-1 of a part of FILENAME."

Not "part of FILENAME", but "the first 4KiB of FILENAME's contents".

Btw, using only the first 4KiB would mean a collision is still
possible, albeit rarely, right?  So your use case of having all the
thumbnails in the same directory could sometimes fail, right?

> +  (with-temp-buffer
> +    (insert-file-contents filename nil 0 4096)

Please use insert-file-contents-literally here.  It should be much
faster, and we only care about the file's bytestream anyway.

>  (defun image-dired-thumb-name (file)
>    "Return absolute file name for thumbnail FILE.
>  Depending on the value of `image-dired-thumbnail-storage', the
>  file name of the thumbnail will vary:
> -- For `use-image-dired-dir', make a SHA1-hash of the image file's
> -  directory name and add that to make the thumbnail file name
> -  unique.
> +- For `image-dired', make a SHA1-hash of some of the image file.
>  - For `per-directory' storage, just add a subdirectory.
>  - For `standard' storage, produce the file name according to the
>    Thumbnail Managing Standard.  Among other things, an MD5-hash

This doc string "needs work".  Could you please fix it as part of the
patch, even though most of the problems are not due to this patch?  In
any case, please either say here that only the first 4KiB of the file's
contents are SHA1-hashed or include a link to the new function.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 11 Feb 2023 12:31:02 GMT) Full text and rfc822 format available.

Message #17 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 11 Feb 2023 13:30:48 +0100
Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 61394 <at> debbugs.gnu.org
>> Date: Fri, 10 Feb 2023 19:46:02 +0100
>> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> 
>> +(defun image-dired-content-sha1 (filename)
>> +  "Compute the SHA-1 of a part of FILENAME."
>
> Not "part of FILENAME", but "the first 4KiB of FILENAME's contents".

Yes, I'll fix that.

> Btw, using only the first 4KiB would mean a collision is still
> possible, albeit rarely, right?  So your use case of having all the
> thumbnails in the same directory could sometimes fail, right?

The 4KiB was "quite large but not so much" guess.  I've made tests with
the following code:

--8<---------------cut here---------------start------------->8---
(defun sha1-test (filename size)
  (with-temp-buffer
    (insert-file-contents-literally filename nil 0 size)
    (sha1 (current-buffer))))

;; From 1KiB to 64KiB
(list
 (benchmark-run-compiled 1000
   (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 10)))
 (benchmark-run-compiled 1000
   (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 11)))
 (benchmark-run-compiled 1000
   (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 12)))
 (benchmark-run-compiled 1000
   (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 13)))
 (benchmark-run-compiled 1000
   (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 14)))
 (benchmark-run-compiled 1000
   (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 15)))
 (benchmark-run-compiled 1000
   (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 16))))
--8<---------------cut here---------------end--------------->8---

And here are the results on my machine:
((0.664336771 1 0.14466495299998883)
 (0.707937024 2 0.28811983400001395)
 (0.940229304 3 0.44037704100000497) ;; <- 4KiB
 (1.672118528 4 0.7672738199999856)
 (2.6194289370000003 6 1.046699996000001)
 (3.169999951 11 1.5916382949999957)
 (6.547043287 21 3.195145416999992))

So this 4KiB seems practical: about 1 second for one thousand run.
WDYT?

About collision, my wild guess here is that, as we are considering
images, most of the modifications on these images we'll have an impact
on those first 4KiB anyway.  But you're that collision is still possible
and the thumb could be wrong.  I'll try to find out what is the
probability of a SHA-1 collision on 4KiB of data.

>> +  (with-temp-buffer
>> +    (insert-file-contents filename nil 0 4096)
>
> Please use insert-file-contents-literally here.  It should be much
> faster, and we only care about the file's bytestream anyway.

Thanks, I'll do that too.

>>  (defun image-dired-thumb-name (file)
>>    "Return absolute file name for thumbnail FILE.
>>  Depending on the value of `image-dired-thumbnail-storage', the
>>  file name of the thumbnail will vary:
>> -- For `use-image-dired-dir', make a SHA1-hash of the image file's
>> -  directory name and add that to make the thumbnail file name
>> -  unique.
>> +- For `image-dired', make a SHA1-hash of some of the image file.
>>  - For `per-directory' storage, just add a subdirectory.
>>  - For `standard' storage, produce the file name according to the
>>    Thumbnail Managing Standard.  Among other things, an MD5-hash
>
> This doc string "needs work".  Could you please fix it as part of the
> patch, even though most of the problems are not due to this patch?  In
> any case, please either say here that only the first 4KiB of the file's
> contents are SHA1-hashed or include a link to the new function.

You're right it is even not complete for `per-directory'.  I could try
to come up with a fix.  Thanks.
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 11 Feb 2023 14:55:02 GMT) Full text and rfc822 format available.

Message #20 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 11 Feb 2023 16:53:25 +0200
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Sat, 11 Feb 2023 13:30:48 +0100
> 
> > Btw, using only the first 4KiB would mean a collision is still
> > possible, albeit rarely, right?  So your use case of having all the
> > thumbnails in the same directory could sometimes fail, right?
> 
> The 4KiB was "quite large but not so much" guess.  I've made tests with
> the following code:
> 
> --8<---------------cut here---------------start------------->8---
> (defun sha1-test (filename size)
>   (with-temp-buffer
>     (insert-file-contents-literally filename nil 0 size)
>     (sha1 (current-buffer))))
> 
> ;; From 1KiB to 64KiB
> (list
>  (benchmark-run-compiled 1000
>    (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 10)))
>  (benchmark-run-compiled 1000
>    (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 11)))
>  (benchmark-run-compiled 1000
>    (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 12)))
>  (benchmark-run-compiled 1000
>    (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 13)))
>  (benchmark-run-compiled 1000
>    (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 14)))
>  (benchmark-run-compiled 1000
>    (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 15)))
>  (benchmark-run-compiled 1000
>    (sha1-test "/tmp/a-5MiB-photo.jpg" (expt 2 16))))
> --8<---------------cut here---------------end--------------->8---
> 
> And here are the results on my machine:
> ((0.664336771 1 0.14466495299998883)
>  (0.707937024 2 0.28811983400001395)
>  (0.940229304 3 0.44037704100000497) ;; <- 4KiB
>  (1.672118528 4 0.7672738199999856)
>  (2.6194289370000003 6 1.046699996000001)
>  (3.169999951 11 1.5916382949999957)
>  (6.547043287 21 3.195145416999992))
> 
> So this 4KiB seems practical: about 1 second for one thousand run.
> WDYT?

I only commented on that because you said it allows you to use a
single directory.  Otherwise, I have no problems with using just 4KiB.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 11 Feb 2023 22:34:02 GMT) Full text and rfc822 format available.

Message #23 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 11 Feb 2023 23:33:19 +0100
Eli Zaretskii <eliz <at> gnu.org> writes:

[...]

> I only commented on that because you said it allows you to use a
> single directory.  Otherwise, I have no problems with using just 4KiB.

A single directory is already what is used now when
image-dired-thumbnail-storage is set to 'image-dired (the default).
Only the SHA-1 is taken from the file path.  I'll fix my patch with your
suggestions then.
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 11 Feb 2023 23:07:02 GMT) Full text and rfc822 format available.

Message #26 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sun, 12 Feb 2023 00:06:32 +0100
[Message part 1 (text/plain, inline)]
So, here is another version of the patch.  I'm not quite sure for the
docstring of image-dired-thumb-name.
[0001-Image-dired-thumb-name-based-on-contents.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
-- 
Manuel Giraud

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sun, 12 Feb 2023 03:03:02 GMT) Full text and rfc822 format available.

Message #29 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>, Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50;
 [PATCH] Image-dired thumb name based on content
Date: Sun, 12 Feb 2023 03:02:25 +0000
Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:

> So, here is another version of the patch.

What is the performance impact of this?  Could we see some benchmarks?
I routinely open folders with hundreds of files that are several
megabytes each, so I think this is the type of benchmark I would be
interested in.  This is common when working with images from a digital
camera.

If it has too much of a performance impact, we could consider making it
optional (and disable it by default).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sun, 12 Feb 2023 21:54:02 GMT) Full text and rfc822 format available.

Message #32 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sun, 12 Feb 2023 22:53:40 +0100
Stefan Kangas <stefankangas <at> gmail.com> writes:

> Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> So, here is another version of the patch.
>
> What is the performance impact of this?  Could we see some benchmarks?

The performance impact is important.  Here are the results from a list
of images of mine:
--8<---------------cut here---------------start------------->8---
(length *images*) -> 3664

(benchmark-run-compiled 10 (dolist (im *images*) (sha1 im)))
 -> (0.367976492 1 0.2809483390000196)
 
(benchmark-run-compiled 10 (dolist (im *images*) (image-dired-contents-sha1 im)))
 -> (72.115512605 84 26.079076938000014)
--8<---------------cut here---------------end--------------->8---

OTOH, using image-dired on a directory of 245 photos before and after
this patch I cannot feel any difference (after the thumbnails are done
of course).

> I routinely open folders with hundreds of files that are several
> megabytes each, so I think this is the type of benchmark I would be
> interested in.  This is common when working with images from a digital
> camera.
>
> If it has too much of a performance impact, we could consider making it
> optional (and disable it by default).

Maybe we could have this in another option for
`image-dired-thumbnail-storage'?  What do you think of
'image-dired-contents?
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Wed, 15 Feb 2023 14:20:01 GMT) Full text and rfc822 format available.

Message #35 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50;
 [PATCH] Image-dired thumb name based on content
Date: Wed, 15 Feb 2023 06:19:02 -0800
Manuel Giraud <manuel <at> ledu-giraud.fr> writes:

> The performance impact is important.  Here are the results from a list
> of images of mine:
> --8<---------------cut here---------------start------------->8---
> (length *images*) -> 3664
>
> (benchmark-run-compiled 10 (dolist (im *images*) (sha1 im)))
>  -> (0.367976492 1 0.2809483390000196)
>
> (benchmark-run-compiled 10 (dolist (im *images*) (image-dired-contents-sha1 im)))
>  -> (72.115512605 84 26.079076938000014)
> --8<---------------cut here---------------end--------------->8---

Thanks.  That's a slowdown by a factor close to 100, so while I think
the feature sounds useful, it should indeed be made optional.

> Maybe we could have this in another option for
> `image-dired-thumbnail-storage'?

I think 'image-dired-thumbnail-storage' is already complicated enough,
and I'd rather not complicate it further.

> What do you think of 'image-dired-contents?

Hmm, it sounds a bit too nondescript.  How about something like
'image-dired-thumbnail-naming'?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Wed, 15 Feb 2023 15:37:02 GMT) Full text and rfc822 format available.

Message #38 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Wed, 15 Feb 2023 16:35:56 +0100
Stefan Kangas <stefankangas <at> gmail.com> writes:

[...]

> Thanks.  That's a slowdown by a factor close to 100, so while I think
> the feature sounds useful, it should indeed be made optional.

I'm ok with that.

>> Maybe we could have this in another option for
>> `image-dired-thumbnail-storage'?
>
> I think 'image-dired-thumbnail-storage' is already complicated enough,
> and I'd rather not complicate it further.

I don't understand here.  Do you suggest introducing another
custom/variable?  'image-dired-thumbnail-storage' has currently only 3
possible values.  I was suggesting adding one more.

>> What do you think of 'image-dired-contents?
>
> Hmm, it sounds a bit too nondescript.  How about something like
> 'image-dired-thumbnail-naming'?

Do you mean as a name for a new variable? (I was talking about a new
possible value for image-dired-thumbnail-storage)
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sun, 19 Feb 2023 14:07:02 GMT) Full text and rfc822 format available.

Message #41 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50;
 [PATCH] Image-dired thumb name based on content
Date: Sun, 19 Feb 2023 06:06:18 -0800
Manuel Giraud <manuel <at> ledu-giraud.fr> writes:

>> I think 'image-dired-thumbnail-storage' is already complicated enough,
>> and I'd rather not complicate it further.
>
> I don't understand here.  Do you suggest introducing another
> custom/variable?  'image-dired-thumbnail-storage' has currently only 3
> possible values.  I was suggesting adding one more.

That variable relates to *where* to store the thumbnail files, not how
to name the thumbnail files.  I might want to use `image-dired' or
`per-directory' using either thumbnail naming scheme.

So a new variable makes more sense, as it is orthogonal from
`image-dired-thumbnail-storage'.

>> Hmm, it sounds a bit too nondescript.  How about something like
>> 'image-dired-thumbnail-naming'?
>
> Do you mean as a name for a new variable? (I was talking about a new
> possible value for image-dired-thumbnail-storage)

Yes.  However, we currently have `image-dired-thumb-size', so perhaps
`image-dired-thumb-naming' is better.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sun, 19 Feb 2023 14:44:02 GMT) Full text and rfc822 format available.

Message #44 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sun, 19 Feb 2023 15:43:26 +0100
Stefan Kangas <stefankangas <at> gmail.com> writes:

> Manuel Giraud <manuel <at> ledu-giraud.fr> writes:
>
>>> I think 'image-dired-thumbnail-storage' is already complicated enough,
>>> and I'd rather not complicate it further.
>>
>> I don't understand here.  Do you suggest introducing another
>> custom/variable?  'image-dired-thumbnail-storage' has currently only 3
>> possible values.  I was suggesting adding one more.
>
> That variable relates to *where* to store the thumbnail files, not how
> to name the thumbnail files.  I might want to use `image-dired' or
> `per-directory' using either thumbnail naming scheme.
>
> So a new variable makes more sense, as it is orthogonal from
> `image-dired-thumbnail-storage'.
>
>>> Hmm, it sounds a bit too nondescript.  How about something like
>>> 'image-dired-thumbnail-naming'?
>>
>> Do you mean as a name for a new variable? (I was talking about a new
>> possible value for image-dired-thumbnail-storage)
>
> Yes.  However, we currently have `image-dired-thumb-size', so perhaps
> `image-dired-thumb-naming' is better.

Ok.  I like that.  I'll try to come up with something but I think I'll
do some cleaning in the process as currently
'image-dired-thumbnail-storage' decides more than just the "where"
(eg. add "thumb" somewhere).
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sun, 19 Feb 2023 16:20:02 GMT) Full text and rfc822 format available.

Message #47 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50;
 [PATCH] Image-dired thumb name based on content
Date: Sun, 19 Feb 2023 08:19:23 -0800
Manuel Giraud <manuel <at> ledu-giraud.fr> writes:

> Ok.  I like that.  I'll try to come up with something but I think I'll
> do some cleaning in the process as currently
> 'image-dired-thumbnail-storage' decides more than just the "where"
> (eg. add "thumb" somewhere).

Sounds good, thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Mon, 20 Feb 2023 09:21:02 GMT) Full text and rfc822 format available.

Message #50 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Mon, 20 Feb 2023 10:20:21 +0100
Hi,

While at that, I see that for an `image-dired-thumbnail-storage' other
than 'standard* the thumbnail name is hardcoded to "jpg" ("png" for
'standard* but it seems to be mandated by said standard).  Should it
stay that way or could we extract the extension from the image filename?
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 25 Feb 2023 18:46:01 GMT) Full text and rfc822 format available.

Message #53 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 25 Feb 2023 19:45:34 +0100
[Message part 1 (text/plain, inline)]
Hi,

Here is a new version of the patch that, I think, addresses all the
issues raised by Stefan.
[0001-New-option-image-dired-thumb-naming-bug-61394.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
-- 
Manuel Giraud

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Wed, 26 Jul 2023 19:19:02 GMT) Full text and rfc822 format available.

Message #56 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: contovob <at> tcd.ie, Eli Zaretskii <eliz <at> gnu.org>, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Wed, 26 Jul 2023 21:18:12 +0200
Hi Stefan,

Just a "ping" on this.  Do you think it could go in Emacs 30?
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 27 Jul 2023 07:04:01 GMT) Full text and rfc822 format available.

Message #59 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 27 Jul 2023 10:04:23 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: contovob <at> tcd.ie,  Eli Zaretskii <eliz <at> gnu.org>,  61394 <at> debbugs.gnu.org
> Date: Wed, 26 Jul 2023 21:18:12 +0200
> 
> Hi Stefan,
> 
> Just a "ping" on this.  Do you think it could go in Emacs 30?

Thanks for the reminder.  I think we should install this now.  So
please rebase on the current master branch and repost, and I will
install it then.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 27 Jul 2023 13:54:01 GMT) Full text and rfc822 format available.

Message #62 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 27 Jul 2023 15:52:55 +0200
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks for the reminder.  I think we should install this now.  So
> please rebase on the current master branch and repost, and I will
> install it then.

Thanks.  Here is the rebased version of the patch.
-- 
Manuel Giraud
[0001-New-option-image-dired-thumb-naming-bug-61394.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 27 Jul 2023 14:16:02 GMT) Full text and rfc822 format available.

Message #65 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 27 Jul 2023 17:16:26 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Thu, 27 Jul 2023 15:52:55 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Thanks for the reminder.  I think we should install this now.  So
> > please rebase on the current master branch and repost, and I will
> > install it then.
> 
> Thanks.  Here is the rebased version of the patch.

Thanks, installed.  But now 1 set in image-dired-util-tests fails; it
sounds like the method of producing per-directory thumbnails has
changed, and now produces a SHA-1 hash?  Can you please look into
fixing that test?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 27 Jul 2023 21:31:02 GMT) Full text and rfc822 format available.

Message #68 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 27 Jul 2023 23:30:29 +0200
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:

[...]

> Thanks, installed.  But now 1 set in image-dired-util-tests fails; it
> sounds like the method of producing per-directory thumbnails has
> changed, and now produces a SHA-1 hash?  Can you please look into
> fixing that test?

Ok, I think this patch should do.
-- 
Manuel Giraud
[0001-Fix-test-after-83b6a8a5147.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 28 Jul 2023 06:55:02 GMT) Full text and rfc822 format available.

Message #71 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 28 Jul 2023 09:55:13 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Thu, 27 Jul 2023 23:30:29 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> [...]
> 
> > Thanks, installed.  But now 1 set in image-dired-util-tests fails; it
> > sounds like the method of producing per-directory thumbnails has
> > changed, and now produces a SHA-1 hash?  Can you please look into
> > fixing that test?
> 
> Ok, I think this patch should do.

Thanks, the tests now pass, but I wonder about this part:

>      (should (equal (cdr (file-name-split
> -                         (image-dired-thumb-name "/tmp/foo.jpg")))
> -                   '("tmp" ".image-dired" "foo.jpg.thumb.jpg")))
> +                         (image-dired-thumb-name abs-path)))
> +                   (list "tmp" ".image-dired" hash-name)))

Does this mean that thumbnail naming under 'per-directory' has
changed, and it now uses the SHA-1 hash of the base-name?  IOW, does
this mean your changes for bug#61394 included incompatible changes in
behavior?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 28 Jul 2023 09:34:02 GMT) Full text and rfc822 format available.

Message #74 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 28 Jul 2023 11:33:19 +0200
Eli Zaretskii <eliz <at> gnu.org> writes:

[...]

> Thanks, the tests now pass, but I wonder about this part:
>
>>      (should (equal (cdr (file-name-split
>> -                         (image-dired-thumb-name "/tmp/foo.jpg")))
>> -                   '("tmp" ".image-dired" "foo.jpg.thumb.jpg")))
>> +                         (image-dired-thumb-name abs-path)))
>> +                   (list "tmp" ".image-dired" hash-name)))
>
> Does this mean that thumbnail naming under 'per-directory' has
> changed, and it now uses the SHA-1 hash of the base-name?  IOW, does
> this mean your changes for bug#61394 included incompatible changes in
> behavior?

Yes I think it does.  My patch for bug#61394 changes the previous
behaviour of 'image-dired-thumbnail-storage'.  Now
'image-dired-thumbnail-storage' defines where (ie. in which directory)
the thumbnails are stored and I introduce 'image-dired-thumb-naming'
which tells how thumbnail file ared named (ie. the file name part sans
directory).

'image-dired-thumb-naming' is meaning less if
'image-dired-thumbnail-storage' is one of the "standard*" method because
those methods already define storage locations, file names and even
sizes.  But for the "per-directory" method, I'm using
'image-dired-thumb-naming'.  As we are talking about thumbnail I did not
think it was a big deal but if it is I can prepare a patch, on top of
the one in place, and then 'image-dired-thumb-naming' will be used only
for the "image-dired" storage method.  WDYT?
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 28 Jul 2023 12:20:02 GMT) Full text and rfc822 format available.

Message #77 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 28 Jul 2023 15:20:35 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Fri, 28 Jul 2023 11:33:19 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> [...]
> 
> > Thanks, the tests now pass, but I wonder about this part:
> >
> >>      (should (equal (cdr (file-name-split
> >> -                         (image-dired-thumb-name "/tmp/foo.jpg")))
> >> -                   '("tmp" ".image-dired" "foo.jpg.thumb.jpg")))
> >> +                         (image-dired-thumb-name abs-path)))
> >> +                   (list "tmp" ".image-dired" hash-name)))
> >
> > Does this mean that thumbnail naming under 'per-directory' has
> > changed, and it now uses the SHA-1 hash of the base-name?  IOW, does
> > this mean your changes for bug#61394 included incompatible changes in
> > behavior?
> 
> Yes I think it does.  My patch for bug#61394 changes the previous
> behaviour of 'image-dired-thumbnail-storage'.  Now
> 'image-dired-thumbnail-storage' defines where (ie. in which directory)
> the thumbnails are stored and I introduce 'image-dired-thumb-naming'
> which tells how thumbnail file ared named (ie. the file name part sans
> directory).
> 
> 'image-dired-thumb-naming' is meaning less if
> 'image-dired-thumbnail-storage' is one of the "standard*" method because
> those methods already define storage locations, file names and even
> sizes.  But for the "per-directory" method, I'm using
> 'image-dired-thumb-naming'.  As we are talking about thumbnail I did not
> think it was a big deal but if it is I can prepare a patch, on top of
> the one in place, and then 'image-dired-thumb-naming' will be used only
> for the "image-dired" storage method.  WDYT?

I don't think I understand all the aspects of this, as I use neither
image-dired nor the thumbnails.  But it sounds like an incompatible
change in behavior wrt what we have in Emacs 29?  If so, how do we
expect this to work for users who will have configured their Emacs for
some particular storage type, and then upgrade to Emacs 30 when that
is released?  Will the existing thumbnails still work for them?  Will
Emacs 30 now start storing thumbnails under differently-formatted
names, even though the user didn't change his/her configuration?

In general, any incompatible change in behavior (if there is indeed
such a change caused by this changeset) is undesirable.  So I'd like
first to discuss why there was a need for the behavior change in the
first place.

(I'm sorry that I didn't realize there was a change in behavior before
installing the changeset.  It seems we never discussed that aspect in
the bug#61394 discussion thread.)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 28 Jul 2023 16:01:02 GMT) Full text and rfc822 format available.

Message #80 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 28 Jul 2023 18:00:52 +0200
Eli Zaretskii <eliz <at> gnu.org> writes:

[...]

>> Yes I think it does.  My patch for bug#61394 changes the previous
>> behaviour of 'image-dired-thumbnail-storage'.  Now
>> 'image-dired-thumbnail-storage' defines where (ie. in which directory)
>> the thumbnails are stored and I introduce 'image-dired-thumb-naming'
>> which tells how thumbnail file ared named (ie. the file name part sans
>> directory).
>> 
>> 'image-dired-thumb-naming' is meaning less if
>> 'image-dired-thumbnail-storage' is one of the "standard*" method because
>> those methods already define storage locations, file names and even
>> sizes.  But for the "per-directory" method, I'm using
>> 'image-dired-thumb-naming'.  As we are talking about thumbnail I did not
>> think it was a big deal but if it is I can prepare a patch, on top of
>> the one in place, and then 'image-dired-thumb-naming' will be used only
>> for the "image-dired" storage method.  WDYT?
>
> I don't think I understand all the aspects of this, as I use neither
> image-dired nor the thumbnails.  But it sounds like an incompatible
> change in behavior wrt what we have in Emacs 29?  If so, how do we
> expect this to work for users who will have configured their Emacs for
> some particular storage type, and then upgrade to Emacs 30 when that
> is released?  Will the existing thumbnails still work for them?  Will
> Emacs 30 now start storing thumbnails under differently-formatted
> names, even though the user didn't change his/her configuration?

AFAIU image-dired thumbnails' generation, this will automatically work
for users from Emacs 29... but you are right that new thumbnails (with
new names) will be generated.  This will be true for users of the
"per-directory" storage method.  This will also be true when one user
customize the 'image-dired-thumb-naming' to 'sha1-contents'.

> In general, any incompatible change in behavior (if there is indeed
> such a change caused by this changeset) is undesirable.  So I'd like
> first to discuss why there was a need for the behavior change in the
> first place.

If you talk about the behaviour change for the "per-directory" method,
as I said, I can restore it back to what it was.
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 28 Jul 2023 18:44:02 GMT) Full text and rfc822 format available.

Message #83 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 28 Jul 2023 21:44:01 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Fri, 28 Jul 2023 18:00:52 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > In general, any incompatible change in behavior (if there is indeed
> > such a change caused by this changeset) is undesirable.  So I'd like
> > first to discuss why there was a need for the behavior change in the
> > first place.
> 
> If you talk about the behaviour change for the "per-directory" method,
> as I said, I can restore it back to what it was.

Yes, I think we should do that, thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 29 Jul 2023 09:52:02 GMT) Full text and rfc822 format available.

Message #86 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 29 Jul 2023 11:51:19 +0200
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:

[...]

>> If you talk about the behaviour change for the "per-directory" method,
>> as I said, I can restore it back to what it was.
>
> Yes, I think we should do that, thanks.

Hi Eli,

So I think that this patch does that but then 5efc7b22cec should be
reverted.

I'm not so sure about the 'image-dired-thumb-name' docstring.

BTW, I'm not running "make check" often but I have some failures here
and there (2 in TRAMP, 2 in EPG...): is it expected?
-- 
Manuel Giraud
[0001-Revert-thumbnail-naming-for-per-directory-storage-me.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 29 Jul 2023 10:35:02 GMT) Full text and rfc822 format available.

Message #89 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 29 Jul 2023 13:34:47 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Sat, 29 Jul 2023 11:51:19 +0200
> 
> So I think that this patch does that but then 5efc7b22cec should be
> reverted.
> 
> I'm not so sure about the 'image-dired-thumb-name' docstring.

Thanks, will look into that soon.

> BTW, I'm not running "make check" often but I have some failures here
> and there (2 in TRAMP, 2 in EPG...): is it expected?

Please report them as separate bugs.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 29 Jul 2023 10:48:01 GMT) Full text and rfc822 format available.

Message #92 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
 text editors" <bug-gnu-emacs <at> gnu.org>
Cc: contovob <at> tcd.ie, 61394 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 stefankangas <at> gmail.com, Manuel Giraud <manuel <at> ledu-giraud.fr>
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 29 Jul 2023 12:47:38 +0200
Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:

Hi Manuel,

> BTW, I'm not running "make check" often but I have some failures here
> and there (2 in TRAMP, 2 in EPG...): is it expected?

No (speaking for Tramp). Could you pls show them?

Best regards, Michael.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 29 Jul 2023 10:49:01 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Sat, 29 Jul 2023 16:51:02 GMT) Full text and rfc822 format available.

Message #98 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Sat, 29 Jul 2023 18:50:27 +0200
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
>> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
>> Date: Sat, 29 Jul 2023 11:51:19 +0200
>> 
>> So I think that this patch does that but then 5efc7b22cec should be
>> reverted.
>> 
>> I'm not so sure about the 'image-dired-thumb-name' docstring.
>
> Thanks, will look into that soon.

Here is a new version of this patch.  I missed some precision in a
docstring in the previous one (sorry).
[0001-Revert-thumbnail-naming-for-per-directory-storage-me.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
>> BTW, I'm not running "make check" often but I have some failures here
>> and there (2 in TRAMP, 2 in EPG...): is it expected?
>
> Please report them as separate bugs.

Ok, I've created one for Tramp for the moment.
-- 
Manuel Giraud

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Mon, 31 Jul 2023 15:54:01 GMT) Full text and rfc822 format available.

Message #101 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Mon, 31 Jul 2023 17:53:01 +0200
Hi,

FTR, there is a thing that does not work with this "thumb name based on
content": upon rotation the thumb is not updated properly.

This comes from the fact that the image content has changed (obviously)
so the new thumb will have a new name and is not displayed in the
*image-dired* buffer 😅.  I'd have to find a way around it.
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Tue, 01 Aug 2023 17:06:02 GMT) Full text and rfc822 format available.

Message #104 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Tue, 01 Aug 2023 19:05:52 +0200
[Message part 1 (text/plain, inline)]
Manuel Giraud <manuel <at> ledu-giraud.fr> writes:

> Hi,
>
> FTR, there is a thing that does not work with this "thumb name based on
> content": upon rotation the thumb is not updated properly.
>
> This comes from the fact that the image content has changed (obviously)
> so the new thumb will have a new name and is not displayed in the
> *image-dired* buffer 😅.  I'd have to find a way around it.

Hi,

So here is a patch that fixes this issue.  Thanks.
-- 
Manuel Giraud
[0001-Fix-thumbnail-update-when-thumb-name-is-based-on-ima.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Wed, 02 Aug 2023 11:43:02 GMT) Full text and rfc822 format available.

Message #107 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Wed, 02 Aug 2023 14:42:20 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Tue, 01 Aug 2023 19:05:52 +0200
> 
> Manuel Giraud <manuel <at> ledu-giraud.fr> writes:
> 
> > Hi,
> >
> > FTR, there is a thing that does not work with this "thumb name based on
> > content": upon rotation the thumb is not updated properly.
> >
> > This comes from the fact that the image content has changed (obviously)
> > so the new thumb will have a new name and is not displayed in the
> > *image-dired* buffer 😅.  I'd have to find a way around it.
> 
> Hi,
> 
> So here is a patch that fixes this issue.  Thanks.

Thanks.  Maybe I'm missing something, but isn't the problem limited to
just one method of naming the thumbnail files?  If so, shouldn't these
changes be limited to that method alone?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Wed, 02 Aug 2023 18:01:02 GMT) Full text and rfc822 format available.

Message #110 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Wed, 02 Aug 2023 20:00:19 +0200
Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
>> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
>> Date: Tue, 01 Aug 2023 19:05:52 +0200
>> 
>> Manuel Giraud <manuel <at> ledu-giraud.fr> writes:
>> 
>> > Hi,
>> >
>> > FTR, there is a thing that does not work with this "thumb name based on
>> > content": upon rotation the thumb is not updated properly.
>> >
>> > This comes from the fact that the image content has changed (obviously)
>> > so the new thumb will have a new name and is not displayed in the
>> > *image-dired* buffer 😅.  I'd have to find a way around it.
>> 
>> Hi,
>> 
>> So here is a patch that fixes this issue.  Thanks.
>
> Thanks.  Maybe I'm missing something, but isn't the problem limited to
> just one method of naming the thumbnail files?  If so, shouldn't these
> changes be limited to that method alone?

Yes.  First I had a test about the method around the call to
'image-dired-update-thumbnail-at-point' but then I realize that nothing
will be done for others methods as I'm doing a test on the thumb name
change:
        ... (unless (string= thumb old-thumb) ...
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Wed, 02 Aug 2023 18:17:02 GMT) Full text and rfc822 format available.

Message #113 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Wed, 02 Aug 2023 21:16:53 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Wed, 02 Aug 2023 20:00:19 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Thanks.  Maybe I'm missing something, but isn't the problem limited to
> > just one method of naming the thumbnail files?  If so, shouldn't these
> > changes be limited to that method alone?
> 
> Yes.  First I had a test about the method around the call to
> 'image-dired-update-thumbnail-at-point' but then I realize that nothing
> will be done for others methods as I'm doing a test on the thumb name
> change:
>         ... (unless (string= thumb old-thumb) ...

This at least warrants a comment, so that others won't have to be
bothered by the same questions.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 03 Aug 2023 08:44:01 GMT) Full text and rfc822 format available.

Message #116 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 03 Aug 2023 11:43:49 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Sat, 29 Jul 2023 18:50:27 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> >> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> >> Date: Sat, 29 Jul 2023 11:51:19 +0200
> >> 
> >> So I think that this patch does that but then 5efc7b22cec should be
> >> reverted.
> >> 
> >> I'm not so sure about the 'image-dired-thumb-name' docstring.
> >
> > Thanks, will look into that soon.
> 
> Here is a new version of this patch.  I missed some precision in a
> docstring in the previous one (sorry).

Thanks, installed on the master branch.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 03 Aug 2023 11:11:02 GMT) Full text and rfc822 format available.

Message #119 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 03 Aug 2023 13:10:43 +0200
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
>> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
>> Date: Wed, 02 Aug 2023 20:00:19 +0200
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> > Thanks.  Maybe I'm missing something, but isn't the problem limited to
>> > just one method of naming the thumbnail files?  If so, shouldn't these
>> > changes be limited to that method alone?
>> 
>> Yes.  First I had a test about the method around the call to
>> 'image-dired-update-thumbnail-at-point' but then I realize that nothing
>> will be done for others methods as I'm doing a test on the thumb name
>> change:
>>         ... (unless (string= thumb old-thumb) ...
>
> This at least warrants a comment, so that others won't have to be
> bothered by the same questions.

What about this one?  I've also permuted some instructions to avoid a
flicker when updating the thumbnail.
-- 
Manuel Giraud
[0001-Fix-thumbnail-update-when-thumb-name-is-based-on-ima.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 03 Aug 2023 11:39:01 GMT) Full text and rfc822 format available.

Message #122 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 03 Aug 2023 14:38:14 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Thu, 03 Aug 2023 13:10:43 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> Yes.  First I had a test about the method around the call to
> >> 'image-dired-update-thumbnail-at-point' but then I realize that nothing
> >> will be done for others methods as I'm doing a test on the thumb name
> >> change:
> >>         ... (unless (string= thumb old-thumb) ...
> >
> > This at least warrants a comment, so that others won't have to be
> > bothered by the same questions.
> 
> What about this one?  I've also permuted some instructions to avoid a
> flicker when updating the thumbnail.

Thanks, a few minor comments below:

> +(defun image-dired-update-thumbnail-at-point ()
> +  "Update the thumbnail at point if the original image file has been
> +modified.  Take care of uncaching and removing the old thumbnail
> +file."

The first line of the doc string should be a single complete sentence.
(Just move the "modified" part to the first line, I think there's
enough space there.)

> +  (when (image-dired-image-at-point-p)
> +    (let* ((file (image-dired-original-file-name))
> +           (thumb (expand-file-name (image-dired-thumb-name file)))
> +           (image (get-text-property (point) 'display)))
> +      (when image
> +        (let ((old-thumb (plist-get (cdr image) :file)))
> +          ;; If 'thumb' and 'old-thumb' are the same file names, do
> +          ;; nothing.  This would be the case when
> +          ;; 'image-dired-thumb-naming' is set to 'sha1-filename'.

Isn't that true for _any_ method except the single one for which you
need this patch?  I thought only names based on SHA1 of the contents
are affected and need this additional renaming?  The comment seems to
say something else.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 03 Aug 2023 16:52:02 GMT) Full text and rfc822 format available.

Message #125 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 03 Aug 2023 18:51:28 +0200
Eli Zaretskii <eliz <at> gnu.org> writes:

[...]

>> +(defun image-dired-update-thumbnail-at-point ()
>> +  "Update the thumbnail at point if the original image file has been
>> +modified.  Take care of uncaching and removing the old thumbnail
>> +file."
>
> The first line of the doc string should be a single complete sentence.
> (Just move the "modified" part to the first line, I think there's
> enough space there.)

I don't understand.  "Update the thumbnail at point." does not seem
sufficient because the fact that the original image might be modified is
the main point of this function.

>> +  (when (image-dired-image-at-point-p)
>> +    (let* ((file (image-dired-original-file-name))
>> +           (thumb (expand-file-name (image-dired-thumb-name file)))
>> +           (image (get-text-property (point) 'display)))
>> +      (when image
>> +        (let ((old-thumb (plist-get (cdr image) :file)))
>> +          ;; If 'thumb' and 'old-thumb' are the same file names, do
>> +          ;; nothing.  This would be the case when
>> +          ;; 'image-dired-thumb-naming' is set to 'sha1-filename'.
>
> Isn't that true for _any_ method except the single one for which you
> need this patch?  I thought only names based on SHA1 of the contents
> are affected and need this additional renaming?  The comment seems to
> say something else.

What about:

"When 'image-dired-thumb-naming' is set to 'sha1-contents', 'thumb' and
'old-thumb' could be different file names.  Update the thumbnail then."
-- 
Manuel Giraud




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Thu, 03 Aug 2023 18:31:02 GMT) Full text and rfc822 format available.

Message #128 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Thu, 03 Aug 2023 21:30:33 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Thu, 03 Aug 2023 18:51:28 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> +(defun image-dired-update-thumbnail-at-point ()
> >> +  "Update the thumbnail at point if the original image file has been
> >> +modified.  Take care of uncaching and removing the old thumbnail
> >> +file."
> >
> > The first line of the doc string should be a single complete sentence.
> > (Just move the "modified" part to the first line, I think there's
> > enough space there.)
> 
> I don't understand.  "Update the thumbnail at point." does not seem
> sufficient because the fact that the original image might be modified is
> the main point of this function.

What I had in mind is this:

 (defun image-dired-update-thumbnail-at-point ()
   "Update the thumbnail at point if the original image file has been modified.
 This function uncaches and removes the thumbnail file under the old name."

OK?

> What about:
> 
> "When 'image-dired-thumb-naming' is set to 'sha1-contents', 'thumb' and
> 'old-thumb' could be different file names.  Update the thumbnail then."

Much better, thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 04 Aug 2023 07:45:02 GMT) Full text and rfc822 format available.

Message #131 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 04 Aug 2023 09:44:41 +0200
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
>> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
>> Date: Thu, 03 Aug 2023 18:51:28 +0200
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >> +(defun image-dired-update-thumbnail-at-point ()
>> >> +  "Update the thumbnail at point if the original image file has been
>> >> +modified.  Take care of uncaching and removing the old thumbnail
>> >> +file."
>> >
>> > The first line of the doc string should be a single complete sentence.
>> > (Just move the "modified" part to the first line, I think there's
>> > enough space there.)
>> 
>> I don't understand.  "Update the thumbnail at point." does not seem
>> sufficient because the fact that the original image might be modified is
>> the main point of this function.
>
> What I had in mind is this:
>
>  (defun image-dired-update-thumbnail-at-point ()
>    "Update the thumbnail at point if the original image file has been modified.
>  This function uncaches and removes the thumbnail file under the old name."
>
> OK?

Ok.  Sorry I misread your reply ("move" not "remove").  BTW, I'm using
'M-q' on docstring and my fill-column is set to 70 (the default I
guess): is there a better way?

>> What about:
>> 
>> "When 'image-dired-thumb-naming' is set to 'sha1-contents', 'thumb' and
>> 'old-thumb' could be different file names.  Update the thumbnail then."
>
> Much better, thanks.

Ok, so here is a new patch.
[0001-Fix-thumbnail-update-when-thumb-name-is-based-on-ima.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
-- 
Manuel Giraud

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 04 Aug 2023 10:56:01 GMT) Full text and rfc822 format available.

Message #134 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 04 Aug 2023 13:55:22 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Fri, 04 Aug 2023 09:44:41 +0200
> 
> > What I had in mind is this:
> >
> >  (defun image-dired-update-thumbnail-at-point ()
> >    "Update the thumbnail at point if the original image file has been modified.
> >  This function uncaches and removes the thumbnail file under the old name."
> >
> > OK?
> 
> Ok.  Sorry I misread your reply ("move" not "remove").  BTW, I'm using
> 'M-q' on docstring and my fill-column is set to 70 (the default I
> guess): is there a better way?

70 is okay as the default value, but when you cannot squeeze the first
line into 70 columns, it is okay to use up to 79.

> >> What about:
> >> 
> >> "When 'image-dired-thumb-naming' is set to 'sha1-contents', 'thumb' and
> >> 'old-thumb' could be different file names.  Update the thumbnail then."
> >
> > Much better, thanks.
> 
> Ok, so here is a new patch.

Thanks, installed.

Should this bug now be closed, or is there something else to do here?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61394; Package emacs. (Fri, 04 Aug 2023 13:38:01 GMT) Full text and rfc822 format available.

Message #137 received at 61394 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394 <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 04 Aug 2023 15:37:29 +0200
Eli Zaretskii <eliz <at> gnu.org> writes:

[...]

> Thanks, installed.
>
> Should this bug now be closed, or is there something else to do here?

Thanks.  I think it can be closed this feature is complete AFAICT.
-- 
Manuel Giraud




Reply sent to Eli Zaretskii <eliz <at> gnu.org>:
You have taken responsibility. (Fri, 04 Aug 2023 14:06:02 GMT) Full text and rfc822 format available.

Notification sent to Manuel Giraud <manuel <at> ledu-giraud.fr>:
bug acknowledged by developer. (Fri, 04 Aug 2023 14:06:02 GMT) Full text and rfc822 format available.

Message #142 received at 61394-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: contovob <at> tcd.ie, stefankangas <at> gmail.com, 61394-done <at> debbugs.gnu.org
Subject: Re: bug#61394: 30.0.50; [PATCH] Image-dired thumb name based on
 content
Date: Fri, 04 Aug 2023 17:05:56 +0300
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Cc: stefankangas <at> gmail.com,  contovob <at> tcd.ie,  61394 <at> debbugs.gnu.org
> Date: Fri, 04 Aug 2023 15:37:29 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> [...]
> 
> > Should this bug now be closed, or is there something else to do here?
> 
> Thanks.  I think it can be closed this feature is complete AFAICT.

Thanks, done.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 02 Sep 2023 11:24:06 GMT) Full text and rfc822 format available.

This bug report was last modified 208 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.