search

xenodium / ready-player

GNU General Public License v3.0
136 stars 7 forks source link

not working with gnu md5sum on linux #6

Closed Zilong-Li closed 3 months ago

Zilong-Li commented 3 months ago

Hi,

Thanks for the package.

I run into a problem on ubuntu where there is only md5sum available. When I alias md5sum as md5 simply, it can't work. Here is the traceback 

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  string-trim-right(nil nil)
  string-trim(nil)
  ready-player--make-md5("003-Audios/Awake_...")
xenodium commented 3 months ago

Ooop. I've been running it on macOS only. Just fixed in fa5b89dd61b4e0dda8f8238f35208d6f871ce41d. Can you update and retry please?

fuzzbomb commented 3 months ago

I also ran into this problem yesterday.

Unfortunately, the change in fa5b89d just turns the problem on it's head. Now it doesn't work with macOS.

It's a BSD versus GNU problem:

I managed to get it working using Emacs built-in (md5) function. Is there a particular reason why an external md5 executable is used?

I'll make a PR shortly.

xenodium commented 3 months ago

Unfortunately, the change in https://github.com/xenodium/ready-player/commit/fa5b89dd61b4e0dda8f8238f35208d6f871ce41d just turns the problem on it's head. Now it doesn't work with macOS.

Oop. I should have checked. Sorry! I'm on macOS myself, but I happened to have md5sum installed (homebrew), so I hadn't seen this break.

I don't know what the situation is on Windows.

Nor me, but we can wait until we hear from a Windows user.

I managed to get it working using Emacs built-in (md5) function.

That's what was used before https://github.com/xenodium/ready-player/commit/fa5b89dd61b4e0dda8f8238f35208d6f871ce41d

I'll make a PR shortly.

Maybe something like this? Work for ya?

(defun ready-player--make-md5 (fpath)
  "Make md5 hash for FPATH."
  (if (equal system-type 'darwin)
      (with-temp-buffer
        (call-process "md5" nil t nil fpath)
        (goto-char (point-min))
        (string-trim (nth 1 (split-string (buffer-string) "="))))
    (with-temp-buffer
      (call-process "md5sum" nil t nil fpath)
      (goto-char (point-min))
      (let ((hash (buffer-substring-no-properties
                   (point) (line-end-position))))
        (car (split-string hash))))))
xenodium commented 3 months ago

Is there a particular reason why an external md5 executable is used

It's hashing to cache thumbnails. Loading thumbnails is pretty slow on large files. AFAIK, elisp hashing will require loading the file into a buffer, which I'm also trying to avoid. Did you have something else in mind?

xenodium commented 3 months ago

elisp hashing will require loading the file into an Emacs buffer

Actually, doesn't look like it. https://github.com/emacs-mirror/emacs/blob/4871ccd41670debd47238ec0d3bbdef064e517e6/lib/md5.c

I've just merged #7 Thank you!

fuzzbomb commented 3 months ago

That's what was used before fa5b89d

It was using `(call-process "md5" ...)" to use the external executable.

I meant Emacs ' built-in (md5). It doesn't use elisp; it's implemented in C.

Maybe something like this? Work for ya?

I thought of doing something like that, but using the built-in function was simpler. Plus we'd still have Windows to deal with.

fuzzbomb commented 3 months ago

Oh, we both commented within a few seconds of each other. Thanks!

Loading thumbnails is pretty slow on large files.

I thought it was making a hash of the fpath string, rather than the actual media file?

xenodium commented 3 months ago

Actual file to ensure fresh thumbnail. Modifying the media thumbnail and keeping the same file name would yield the stale thumbnail otherwise, no?

edit: typo

xenodium commented 3 months ago

Actual file to ensure fresh thumbnail. Modifying the media thumbnail and keeping the same file name would yield the stale thumbnail otherwise, no?

@fuzzbomb Keeping the elisp one and buffer refreshes forces a thumbnail generation. Should be a good tradeoff. Thanks.