[Patch] Add: warning when no tag writer exists

[Grant Shoshin Shangreaux]

Hello EMMS!

My FSF copyright assignment has been finalized, so I am now free to
contribute. The following patches are related to issues I had with the
tag editor allowing me to edit tags and save them without actually
writing to the file. This is easy to recreate with an Opus file, which
does not have a tag writer configured for EMMS. The second patch here
makes it so that EMMS will warn the user that there is no tag writer
configured for this file type. Should the user want to edit tags anyway,
it can be overridden with C-u. This should work for both single and
multiple file edits. In the case of multiple files, tracks with a tag
writer will open in the tag editor while the other tracks are filtered
out and we echo a warning.

I'm happy to make any adjustments needed, and I am still working on some
of the other improvements talked about in another thread. The first
patch here is just some clean up that seemed useful.

Also, please let me know if there's a proper way to send these patches
to the list. I'm pretty new this flow.

Thank you!
Grant Shangreaux

>From 9ec30dabd899c593199cbbfca33c14fe252b21ff Mon Sep 17 00:00:00 2001
From: Grant Shangreaux <grant@unabridgedsoftware.com>
Date: Sun, 7 Feb 2021 15:30:34 -0600
Subject: [PATCH 1/2] Clean: emms-track-file-p replaces eq file type checks

There were about 10 checks throughout the code checking if a track is a
file type. File type tracks are specific enough for a predicate function.
---
 doc/emms.texinfo      | 2 +-
 emms-cache.el         | 2 +-
 emms-cue.el           | 2 +-
 emms-info-libtag.el   | 2 +-
 emms-info-metaflac.el | 2 +-
 emms-info-mp3info.el  | 2 +-
 emms-info-ogginfo.el  | 2 +-
 emms-info-opusinfo.el | 2 +-
 emms-info-tinytag.el  | 2 +-
 emms-player-mpd.el    | 2 +-
 emms-tag-editor.el    | 2 +-
 emms.el               | 4 ++++
 12 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/doc/emms.texinfo b/doc/emms.texinfo
index 77c27b2..d0e4570 100644
--- a/doc/emms.texinfo
+++ b/doc/emms.texinfo
@@ -1669,7 +1669,7 @@ from @file{emms-player-simple.el}
 @lisp
 (defun emms-mpg321-remote-playablep (track)
        "Return non-nil when we can play this track."
-       (and (eq 'file (emms-track-type track))
+       (and (emms-track-file-p track)
 @end lisp
 @noindent
 
diff --git a/emms-cache.el b/emms-cache.el
index 3e0d056..35b851c 100644
--- a/emms-cache.el
+++ b/emms-cache.el
@@ -163,7 +163,7 @@ regardless of whether they have been modified or not."
   (interactive "P")
   (let (removed)
     (maphash (lambda (path track)
-               (when (eq (emms-track-get track 'type) 'file)
+               (when (emms-track-file-p track)
                  ;; if no longer here, remove
                  (if (not (file-exists-p path))
                      (progn
diff --git a/emms-cue.el b/emms-cue.el
index 3828ee6..880d2d8 100644
--- a/emms-cue.el
+++ b/emms-cue.el
@@ -93,7 +93,7 @@ When PREVIOUS-P is t, get previous track info instead."
 (defun emms-info-cueinfo (track)
   "Add track information to TRACK.
 This is a useful element for `emms-info-functions'."
-  (when (and (eq 'file (emms-track-type track))
+  (when (and (emms-track-file-p track)
              (string-match "\\.\\(ape\\|flac\\)\\'" (emms-track-name track)))
     (let ((cue (concat (file-name-sans-extension (emms-track-name track))
                        ".cue")))
diff --git a/emms-info-libtag.el b/emms-info-libtag.el
index 4269aba..fb9c5dd 100644
--- a/emms-info-libtag.el
+++ b/emms-info-libtag.el
@@ -81,7 +81,7 @@ Case is irrelevant."
   :type '(string))
 
 (defun emms-info-libtag (track)
-  (when (and (eq 'file (emms-track-type track))
+  (when (and (emms-track-file-p track)
              (let ((case-fold-search t))
                (string-match
                 emms-info-libtag-known-extensions
diff --git a/emms-info-metaflac.el b/emms-info-metaflac.el
index 79131cb..f03925f 100644
--- a/emms-info-metaflac.el
+++ b/emms-info-metaflac.el
@@ -70,7 +70,7 @@ external metaflac program"
 (defun emms-info-metaflac (track)
   "Get the FLAC tag of file TRACK, using `emms-info-metaflac-program'
 and return an emms-info structure representing it."
-  (when (and (eq 'file (emms-track-type track))
+  (when (and (emms-track-file-p track)
              (string-match "\\.\\(flac\\|FLAC\\)\\'" (emms-track-name track)))
     (with-temp-buffer
       (when (zerop
diff --git a/emms-info-mp3info.el b/emms-info-mp3info.el
index 06624d5..5ebc964 100644
--- a/emms-info-mp3info.el
+++ b/emms-info-mp3info.el
@@ -72,7 +72,7 @@ This should be a list of info-flag=value lines."
 (defun emms-info-mp3info (track)
   "Add track information to TRACK.
 This is a useful element for `emms-info-functions'."
-  (when (and (eq 'file (emms-track-type track))
+  (when (and (emms-track-file-p track)
              (string-match "\\.[Mm][Pp]3\\'" (emms-track-name track)))
     (with-temp-buffer
       (when (zerop
diff --git a/emms-info-ogginfo.el b/emms-info-ogginfo.el
index fbe64ec..68950ff 100644
--- a/emms-info-ogginfo.el
+++ b/emms-info-ogginfo.el
@@ -44,7 +44,7 @@ program"
 (defun emms-info-ogginfo (track)
   "Add track information to TRACK.
 This is a useful element for `emms-info-functions'."
-  (when (and (eq 'file (emms-track-type track))
+  (when (and (emms-track-file-p track)
              (string-match "\\.[Oo][Gg][Gg]\\'" (emms-track-name track)))
 
     (with-temp-buffer
diff --git a/emms-info-opusinfo.el b/emms-info-opusinfo.el
index af1a9b8..7f28387 100644
--- a/emms-info-opusinfo.el
+++ b/emms-info-opusinfo.el
@@ -43,7 +43,7 @@ program"
 (defun emms-info-opusinfo (track)
   "Add track information to TRACK.
 This is a useful element for `emms-info-functions'."
-  (when (and (eq 'file (emms-track-type track))
+  (when (and (emms-track-file-p track)
              (or (string-match "\\.[Oo][Gg][Gg]\\'" (emms-track-name track))
                  (string-match "\\.[Oo][Pp][Uu][Ss]\\'" (emms-track-name 
track))))
 
diff --git a/emms-info-tinytag.el b/emms-info-tinytag.el
index 32895f0..28a72de 100644
--- a/emms-info-tinytag.el
+++ b/emms-info-tinytag.el
@@ -80,7 +80,7 @@ Case is irrelevant."
 
 (defun emms-info-tinytag (track)
   "Set tags for TRACK using tinytag."
-  (when (and (eq 'file (emms-track-type track))
+  (when (and (emms-track-file-p track)
             (let ((case-fold-search t))
               (string-match
                emms-info-tinytag-known-extensions
diff --git a/emms-player-mpd.el b/emms-player-mpd.el
index 39f23d5..e7a61a0 100644
--- a/emms-player-mpd.el
+++ b/emms-player-mpd.el
@@ -1158,7 +1158,7 @@ info from MusicPD.
 This is a useful addition to `emms-info-functions'."
   (if info
       (emms-info-mpd-process track info)
-    (when (and (eq 'file (emms-track-type track))
+    (when (and (emms-track-file-p track)
                (not (string-match "\\`http://"; (emms-track-name track))))
       (let ((file (emms-player-mpd-get-mpd-filename (emms-track-name track))))
         (when (or emms-player-mpd-music-directory
diff --git a/emms-tag-editor.el b/emms-tag-editor.el
index c00a4fb..045a516 100644
--- a/emms-tag-editor.el
+++ b/emms-tag-editor.el
@@ -617,7 +617,7 @@ With prefix argument, bury the tag edit buffer."
                 old (emms-track-get track 'orig-track))
           ;; rename local file
           (when (and (emms-track-get track 'newname)
-                     (eq (emms-track-get track 'type) 'file)
+                     (emms-track-file-p track)
                      (file-writable-p (emms-track-name track))
                      (y-or-n-p (format "Rename %s to %s? "
                                        (emms-track-name track)
diff --git a/emms.el b/emms.el
index 8c950b1..6965e2a 100644
--- a/emms.el
+++ b/emms.el
@@ -676,6 +676,10 @@ before the error is signaled."
   "Return the type of TRACK."
   (emms-track-get track 'type))
 
+(defun emms-track-file-p (track)
+  "True if TRACK is a file type"
+  (eq 'file (emms-track-type track)))
+
 (defun emms-track-name (track)
   "Return the name of TRACK."
   (emms-track-get track 'name))
-- 
2.20.1

>From ae5bea3703072580ace8a9e10d6ffbb6b09c39e4 Mon Sep 17 00:00:00 2001
From: Grant Shangreaux <grant@unabridgedsoftware.com>
Date: Sat, 6 Feb 2021 11:39:20 -0600
Subject: [PATCH 2/2] Add: EMMS warning if no tagging program for audio file
 type

Warn the user when there is no tag writing program available to EMMS
for modifying the actual audio files. Current behavior allows the tags
to be edited, saved to the EMMS cache db, and then silently skips past
the part where the metadata would be applied to the file itself.

The changes here prevent the user from even opening the editor for a
file that has no tag writing program configured. This can be
overridden with the prefix arg when user wants to edit the tags
anyway, knowing they will only be saved in the emms-cache-db and not
written to the source audio files.
---
 emms-tag-editor.el | 65 +++++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 21 deletions(-)

diff --git a/emms-tag-editor.el b/emms-tag-editor.el
index 045a516..5b6e447 100644
--- a/emms-tag-editor.el
+++ b/emms-tag-editor.el
@@ -271,27 +271,52 @@ This string is suitable for inserting into the tags 
buffer."
     (goto-char (point-min))
     (emms-tag-editor-display-log-buffer-maybe)))
 
-(defun emms-tag-editor-edit-track (track)
-  "Edit the track at point, or TRACK."
+(defun emms-tag-editor--tagfile-function (track)
+  "Return value of `emms-tag-editor-tagfile-functions' for TRACK, or nil."
+  (assoc (file-name-extension (emms-track-get track 'name))
+         emms-tag-editor-tagfile-functions))
+
+(defun emms-tag-editor--track-editable-p (track)
+  "Return t if TRACK is not a file, or has a tagfile function defined."
+  (or (not (emms-track-file-p track))
+      (emms-tag-editor--tagfile-function track)))
+
+(defun emms-tag-editor-edit-track (track &optional edit-anyway)
+  "Edit the track at point, or TRACK.
+If EDIT-ANYWAY is true or TRACK is not a file type, it will be loaded
+in the tag editor. Otherwise, if EMMS does not have a program configured
+to actually write tags to the audio file, do not open the tag data in
+the editor."
   (interactive (list (emms-tag-editor-track-at)))
-  (if (null track)
-      (message "No track at point!")
-    (emms-tag-editor-insert-tracks (list track))))
-
-(defun emms-tag-editor-edit-marked-tracks ()
-  "Edit all tracks marked in the current buffer."
+  (cond
+   ((null track) (message "No track at point!"))
+   ((or (emms-tag-editor--track-editable-p track) edit-anyway)
+    (emms-tag-editor-insert-tracks (list track)))
+   (t (message "EMMS has no tag writing program configured for this file 
type!"))))
+
+(defun emms-tag-editor-edit-marked-tracks (&optional edit-anyway)
+  "Edit all tracks marked in the current buffer.
+If EDIT-ANYWAY is nil, filter out any file tracks that do not have a
+tagfile function defined."
   (interactive)
-  (let ((tracks (emms-mark-mapcar-marked-track 'emms-tag-editor-track-at t)))
+  (let* ((tracks (emms-mark-mapcar-marked-track 'emms-tag-editor-track-at t))
+         (funcs (mapcar #'emms-tag-editor--tagfile-function tracks)))
+    (when (seq-some #'null funcs)
+      (unless edit-anyway
+        (setq tracks (seq-filter #'emms-tag-editor--track-editable-p tracks))
+        (message "Skipped file tracks without a tag writing program 
configured.")))
     (if (null tracks)
-        (message "No track marked!")
+        (message "No writable track marked!")
       (emms-tag-editor-insert-tracks tracks))))
 
-(defun emms-tag-editor-edit ()
-  "Edit tags of either the track at point or all marked tracks."
-  (interactive)
+(defun emms-tag-editor-edit (&optional arg)
+  "Edit tags of either the track at point or all marked tracks.
+With a prefix argument, edits tags even if there is no external
+program for writing tags to the specified track or tracks."
+  (interactive "P")
   (if (emms-mark-has-markedp)
-      (emms-tag-editor-edit-marked-tracks)
-    (emms-tag-editor-edit-track (emms-tag-editor-track-at))))
+      (emms-tag-editor-edit-marked-tracks arg)
+    (emms-tag-editor-edit-track (emms-tag-editor-track-at) arg)))
 
 (defvar emms-tag-editor-mode-map
   (let ((map (make-sparse-keymap)))
@@ -646,10 +671,9 @@ With prefix argument, bury the tag edit buffer."
             (when (setq val (emms-track-get track (car tag)))
             (emms-track-set old (car tag) val)))
           ;; use external program to change tags in the file
-          (when (and (eq (emms-track-get track 'type) 'file)
+          (when (and (emms-track-file-p track)
                      (file-writable-p (emms-track-name track))
-                     (setq func (assoc (file-name-extension filename)
-                                       emms-tag-editor-tagfile-functions)))
+                     (setq func (emms-tag-editor--tagfile-function track)))
             (setq exit
                   (if (functionp (cdr func))
                       (funcall (cdr func) track)
@@ -755,7 +779,7 @@ value.
 If DONT-APPLY is non-nil the changes won't be applied directly.
 Then it's the callers job to apply them afterwards with
 `emms-tag-editor-apply'."
-  (if (eq (emms-track-get track 'type) 'file)
+  (if (emms-track-file-p track)
       (let* ((old-file (emms-track-name track))
              (path     (file-name-directory old-file))
              (suffix   (file-name-extension old-file))
@@ -829,7 +853,7 @@ A pipe is defined like below:
 
 (defun emms-tag-editor-track-pipe (track pipe-name)
   "Run command of pipe nameed PIPE-NAME to TRACK."
-  (if (eq (emms-track-get track 'type) 'file)
+  (if (emms-track-file-p track)
       (let* ((coding-system-for-read 'utf-8)
              (command (emms-tag-editor-pipe-get pipe-name :command))
              (arguments (emms-tag-editor-pipe-get pipe-name :arguments)))
@@ -877,6 +901,5 @@ A pipe is defined like below:
       (dolist (track tracks)
         (emms-tag-editor-track-pipe track pipe-name)))))
 
-
 (provide 'emms-tag-editor)
 ;;; Emms-tag-editor.el ends here
-- 
2.20.1