pkgs/mpdpopm: Make the rating centered around 0 (i.e. a i8 instead of u8)

This allows us to correctly track "negative" ratings, when the user
specifies `rating decr` multiple times.

5 files changed, 11 insertions, 404 deletions

diff --git a/pkgs/by-name/mp/mpdpopm/src/bin/mpdpopm.rs b/pkgs/by-name/mp/mpdpopm/src/bin/mpdpopm.rs
index 04760f18..82a354d6 100644
--- a/pkgs/by-name/mp/mpdpopm/src/bin/mpdpopm.rs
+++ b/pkgs/by-name/mp/mpdpopm/src/bin/mpdpopm.rs
@@ -160,7 +160,7 @@ async fn get_ratings(
     tracks: Option<Vec<String>>,
     with_uri: bool,
 ) -> Result<()> {
-    let mut ratings: Vec<(String, u8)> = Vec::new();
+    let mut ratings: Vec<(String, i8)> = Vec::new();
 
     for file in map_tracks(client, tracks).await? {
         let rating = rating_count::get(client, &file)
@@ -185,7 +185,7 @@ async fn get_ratings(
 }
 
 /// Rate a track
-async fn set_rating(client: &mut Client, rating: u8, arg: Option<String>) -> Result<()> {
+async fn set_rating(client: &mut Client, rating: i8, arg: Option<String>) -> Result<()> {
     let is_current = arg.is_none();
     let file = provide_file(client, arg).await?;
 
@@ -472,7 +472,7 @@ enum RatingCommand {
     /// With a second argument, rate that song at the first argument. Ratings
     /// may be expressed a an integer between 0 & 255, inclusive.
     #[clap(verbatim_doc_comment)]
-    Set { rating: u8, track: Option<String> },
+    Set { rating: i8, track: Option<String> },
 
     /// increment the rating for one track
     ///
diff --git a/pkgs/by-name/mp/mpdpopm/src/lib.rs b/pkgs/by-name/mp/mpdpopm/src/lib.rs
index 26645228..e4579db2 100644
--- a/pkgs/by-name/mp/mpdpopm/src/lib.rs
+++ b/pkgs/by-name/mp/mpdpopm/src/lib.rs
@@ -37,7 +37,6 @@ pub mod config;
 pub mod filters_ast;
 pub mod messages;
 pub mod playcounts;
-pub mod ratings;
 pub mod storage;
 pub mod vars;
 
diff --git a/pkgs/by-name/mp/mpdpopm/src/messages.rs b/pkgs/by-name/mp/mpdpopm/src/messages.rs
index ae356f34..c7c295c8 100644
--- a/pkgs/by-name/mp/mpdpopm/src/messages.rs
+++ b/pkgs/by-name/mp/mpdpopm/src/messages.rs
@@ -52,8 +52,6 @@ use crate::{
     clients::{Client, IdleClient, PlayerStatus},
     filters::ExpressionParser,
     filters_ast::{FilterStickerNames, evaluate},
-    ratings::{RatedTrack, RatingRequest},
-    storage::{self, last_played, play_count, rating_count},
 };
 
 use backtrace::Backtrace;
@@ -61,10 +59,8 @@ use boolinator::Boolinator;
 use tracing::debug;
 
 use std::collections::VecDeque;
-use std::convert::TryFrom;
 use std::path::PathBuf;
 
-
 #[derive(Debug)]
 pub enum Error {
     BadPath {
@@ -347,15 +343,7 @@ impl MessageProcessor {
         state: &PlayerStatus,
         stickers: &FilterStickerNames<'a>,
     ) -> Result<()> {
-        if let Some(stripped) = msg.strip_prefix("rate ") {
-            self.rate(stripped, client, state).await
-        } else if let Some(stripped) = msg.strip_prefix("inc-rate ") {
-            self.inc_rate(stripped, client, state).await
-        } else if let Some(stripped) = msg.strip_prefix("setpc ") {
-            self.setpc(stripped, client, state).await
-        } else if let Some(stripped) = msg.strip_prefix("setlp ") {
-            self.setlp(stripped, client, state).await
-        } else if let Some(stripped) = msg.strip_prefix("findadd ") {
+        if let Some(stripped) = msg.strip_prefix("findadd ") {
             self.findadd(stripped.to_string(), client, stickers, state)
                 .await
         } else if let Some(stripped) = msg.strip_prefix("searchadd ") {
@@ -366,189 +354,6 @@ impl MessageProcessor {
         }
     }
 
-    /// Handle rating message: "RATING( TRACK)?"
-    async fn rate(&self, msg: &str, client: &mut Client, state: &PlayerStatus) -> Result<()> {
-        let req = RatingRequest::try_from(msg).map_err(|err| Error::Ratings {
-            source: storage::Error::Rating {
-                source: err,
-                back: Backtrace::new(),
-            },
-            back: Backtrace::new(),
-        })?;
-
-        let pathb = match req.track {
-            RatedTrack::Current => match state {
-                PlayerStatus::Stopped => {
-                    return Err(Error::PlayerStopped {});
-                }
-                PlayerStatus::Play(curr) | PlayerStatus::Pause(curr) => curr.file.clone(),
-            },
-            RatedTrack::File(p) => p,
-            RatedTrack::Relative(_i) => {
-                return Err(Error::NotImplemented {
-                    feature: String::from("Relative track position"),
-                });
-            }
-        };
-        let path: &str = pathb
-            .to_str()
-            .ok_or_else(|| Error::BadPath { pth: pathb.clone() })?;
-
-        debug!("Setting a rating of {} for `{}'.", req.rating, path);
-
-        rating_count::set(client, path, req.rating)
-            .await
-            .map_err(|err| Error::Ratings {
-                source: err,
-                back: Backtrace::new(),
-            })?;
-
-        Ok(())
-    }
-
-    /// Handle inc-rating message: "( TRACK)?"
-    async fn inc_rate(&self, msg: &str, client: &mut Client, state: &PlayerStatus) -> Result<()> {
-        let pathb = if msg.is_empty() {
-            // We rate the current track
-            match state {
-                PlayerStatus::Stopped => {
-                    return Err(Error::PlayerStopped {});
-                }
-                PlayerStatus::Play(curr) | PlayerStatus::Pause(curr) => curr.file.clone(),
-            }
-        } else {
-            PathBuf::from(msg)
-        };
-
-        let path: &str = pathb
-            .to_str()
-            .ok_or_else(|| Error::BadPath { pth: pathb.clone() })?;
-
-        let rating = rating_count::get(client, path)
-            .await
-            .map_err(|err| Error::Ratings {
-                source: err,
-                back: Backtrace::new(),
-            })?
-            .unwrap_or(0)
-            .saturating_add(1);
-
-        debug!(
-            "Incrementing a rating for `{}' (new value: {}).",
-            path, rating
-        );
-
-        rating_count::set(client, path, rating)
-            .await
-            .map_err(|err| Error::Ratings {
-                source: err,
-                back: Backtrace::new(),
-            })?;
-
-        Ok(())
-    }
-
-    /// Handle `setpc': "PC( TRACK)?"
-    async fn setpc(&self, msg: &str, client: &mut Client, state: &PlayerStatus) -> Result<()> {
-        let text = msg.trim();
-        let (pc, track) = match text.find(char::is_whitespace) {
-            Some(idx) => (
-                text[..idx]
-                    .parse::<usize>()
-                    .map_err(|err| Error::ExpectedInt {
-                        source: err,
-                        text: String::from(text),
-                        back: Backtrace::new(),
-                    })?,
-                &text[idx + 1..],
-            ),
-            None => (
-                text.parse::<usize>().map_err(|err| Error::ExpectedInt {
-                    source: err,
-                    text: String::from(text),
-                    back: Backtrace::new(),
-                })?,
-                "",
-            ),
-        };
-        let file = if track.is_empty() {
-            match state {
-                PlayerStatus::Stopped => {
-                    return Err(Error::PlayerStopped {});
-                }
-                PlayerStatus::Play(curr) | PlayerStatus::Pause(curr) => curr
-                    .file
-                    .to_str()
-                    .ok_or_else(|| Error::BadPath {
-                        pth: curr.file.clone(),
-                    })?
-                    .to_string(),
-            }
-        } else {
-            track.to_string()
-        };
-
-        play_count::set(client, &file, pc)
-            .await
-            .map_err(|err| Error::Playcount {
-                source: err,
-                back: Backtrace::new(),
-            })?;
-
-        Ok(())
-    }
-
-    /// Handle `setlp': "LASTPLAYED( TRACK)?"
-    async fn setlp(&self, msg: &str, client: &mut Client, state: &PlayerStatus) -> Result<()> {
-        let text = msg.trim();
-        let (lp, track) = match text.find(char::is_whitespace) {
-            Some(idx) => (
-                text[..idx]
-                    .parse::<u64>()
-                    .map_err(|err| Error::ExpectedInt {
-                        source: err,
-                        text: String::from(text),
-                        back: Backtrace::new(),
-                    })?,
-                &text[idx + 1..],
-            ),
-            None => (
-                text.parse::<u64>().map_err(|err| Error::ExpectedInt {
-                    source: err,
-                    text: String::from(text),
-                    back: Backtrace::new(),
-                })?,
-                "",
-            ),
-        };
-
-        let file = if track.is_empty() {
-            match state {
-                PlayerStatus::Stopped => {
-                    return Err(Error::PlayerStopped {});
-                }
-                PlayerStatus::Play(curr) | PlayerStatus::Pause(curr) => curr
-                    .file
-                    .to_str()
-                    .ok_or_else(|| Error::BadPath {
-                        pth: curr.file.clone(),
-                    })?
-                    .to_string(),
-            }
-        } else {
-            track.to_string()
-        };
-
-        last_played::set(client, &file, lp)
-            .await
-            .map_err(|err| Error::Playcount {
-                source: err,
-                back: Backtrace::new(),
-            })?;
-
-        Ok(())
-    }
-
     /// Handle `findadd': "FILTER [sort TYPE] [window START:END]"
     async fn findadd<'a>(
         &self,
diff --git a/pkgs/by-name/mp/mpdpopm/src/ratings.rs b/pkgs/by-name/mp/mpdpopm/src/ratings.rs
deleted file mode 100644
index 739d3827..00000000
--- a/pkgs/by-name/mp/mpdpopm/src/ratings.rs
+++ /dev/null
@@ -1,195 +0,0 @@
-// Copyright (C) 2020-2025 Michael herstine <sp1ff@pobox.com>
-//
-// This file is part of mpdpopm.
-//
-// mpdpopm is free software: you can redistribute it and/or modify it under the terms of the GNU
-// General Public License as published by the Free Software Foundation, either version 3 of the
-// License, or (at your option) any later version.
-//
-// mpdpopm is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even
-// the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General
-// Public License for more details.
-//
-// You should have received a copy of the GNU General Public License along with mpdpopm.  If not,
-// see <http://www.gnu.org/licenses/>.
-
-//! Logic for rating MPD tracks.
-//!
-//! # Introduction
-//!
-//! This module contains types implementing a basic rating functionality for
-//! [MPD](http://www.musicpd.org).
-//!
-//! # Discussion
-//!
-//! Rating messages to the relevant channel take the form `RATING( TRACK)?` (the two components can
-//! be separated by any whitespace). The rating can be given by an integer between 0 & 255
-//! (inclusive) represented in base ten, or as one-to-five asterisks (i.e. `\*{1,5}`). In the latter
-//! case, the rating will be mapped to 1-255 as per Winamp's
-//! [convention](http://forums.winamp.com/showpost.php?p=2903240&postcount=94):
-//!
-//!   - 224-255: 5 stars when READ with windows explorer, writes 255
-//!   - 160-223: 4 stars when READ with windows explorer, writes 196
-//!   - 096-159: 3 stars when READ with windows explorer, writes 128
-//!   - 032-095: 2 stars when READ with windows explorer, writes 64
-//!   - 001-031: 1 stars when READ with windows explorer, writes 1
-//!
-//! NB a rating of zero means "not rated".
-//!
-//! Everything after the first whitepace, if present, is taken to be the track to be rated (i.e.
-//! the track may contain whitespace). If omitted, the rating is taken to apply to the current
-//! track.
-
-use backtrace::Backtrace;
-
-use std::path::PathBuf;
-
-////////////////////////////////////////////////////////////////////////////////////////////////////
-//                                             Error                                              //
-////////////////////////////////////////////////////////////////////////////////////////////////////
-
-/// An enumeration of ratings errors
-#[derive(Debug)]
-pub enum Error {
-    Rating {
-        source: std::num::ParseIntError,
-        text: String,
-    },
-    PlayerStopped,
-    NotImplemented {
-        feature: String,
-    },
-    BadPath {
-        pth: PathBuf,
-        back: Backtrace,
-    },
-    Client {
-        source: crate::clients::Error,
-        back: Backtrace,
-    },
-}
-
-impl std::fmt::Display for Error {
-    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        match self {
-            Error::Rating { source, text } => write!(
-                f,
-                "Unable to interpret ``{}'' as a rating: {}",
-                text, source
-            ),
-            Error::PlayerStopped => write!(f, "Player stopped"),
-            Error::NotImplemented { feature } => write!(f, "{} not implemented", feature),
-            Error::BadPath { pth, back: _ } => write!(f, "Bad path: {:?}", pth),
-            Error::Client { source, back: _ } => write!(f, "Client error: {}", source),
-        }
-    }
-}
-
-impl std::error::Error for Error {
-    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        match &self {
-            Error::Rating { text: _, source } => Some(source),
-            Error::Client { source, back: _ } => Some(source),
-            _ => None,
-        }
-    }
-}
-
-pub type Result<T> = std::result::Result<T, Error>;
-
-////////////////////////////////////////////////////////////////////////////////////////////////////
-//                                     RatingRequest message                                      //
-////////////////////////////////////////////////////////////////////////////////////////////////////
-
-/// The track to which a rating shall be applied.
-#[derive(Debug, PartialEq)]
-pub enum RatedTrack {
-    Current,
-    File(std::path::PathBuf),
-    Relative(i8),
-}
-
-/// A request from a client to rate a track.
-#[derive(Debug)]
-pub struct RatingRequest {
-    pub rating: u8,
-    pub track: RatedTrack,
-}
-
-/// Produce a RatingRequest instance from a line of MPD output.
-impl std::convert::TryFrom<&str> for RatingRequest {
-    type Error = Error;
-
-    /// Attempt to produce a RatingRequest instance from a line of MPD response to a
-    /// "readmessages" command. After the channel line, each subsequent line will be of the form
-    /// "message: $MESSAGE"-- this method assumes that the "message: " prefix has been stripped off
-    /// (i.e. we're dealing with a single line of text containing only our custom message format).
-    ///
-    /// For ratings, we expect a message of the form: "RATING (TRACK)?".
-    fn try_from(text: &str) -> std::result::Result<Self, Self::Error> {
-        // We expect a message of the form: "RATING (TRACK)?"; let us split `text' into those two
-        // components for separate processing:
-        let text = text.trim();
-        let (rating, track) = match text.find(char::is_whitespace) {
-            Some(idx) => (&text[..idx], &text[idx + 1..]),
-            None => (text, ""),
-        };
-
-        // Rating first-- the desired rating can be specified in a few ways...
-        let rating = if rating.is_empty() {
-            // an empty string is interpreted as zero:
-            0u8
-        } else {
-            // "*{1,5}" is interpreted as one-five stars, mapped to [0,255] as per Winamp:
-            match rating {
-                "*" => 1,
-                "**" => 64,
-                "***" => 128,
-                "****" => 196,
-                "*****" => 255,
-                // failing that, we try just interperting `rating' as an unsigned integer:
-                _ => rating.parse::<u8>().map_err(|err| Error::Rating {
-                    source: err,
-                    text: String::from(rating),
-                })?,
-            }
-        };
-
-        // Next-- track. This, too, can be given in a few ways:
-        let track = if track.is_empty() {
-            // nothing at all just means "current track"
-            RatedTrack::Current
-        } else {
-            // otherwise...
-            match text.parse::<i8>() {
-                // if we can interpret `track' as an i8, we take it as an offset...
-                Ok(i) => RatedTrack::Relative(i),
-                // else, we assume it's a path. If it's not, we'll figure that out downstream.
-                Err(_) => RatedTrack::File(std::path::PathBuf::from(&track)),
-            }
-        };
-
-        Ok(RatingRequest { rating, track })
-    }
-}
-
-#[cfg(test)]
-mod rating_request_tests {
-    use super::*;
-    use std::convert::TryFrom;
-
-    /// RatingRequest smoke tests
-    #[test]
-    fn rating_request_smoke() {
-        let req = RatingRequest::try_from("*** foo bar splat.mp3").unwrap();
-        assert_eq!(req.rating, 128);
-        assert_eq!(
-            req.track,
-            RatedTrack::File(PathBuf::from("foo bar splat.mp3"))
-        );
-        let req = RatingRequest::try_from("255").unwrap();
-        assert_eq!(req.rating, 255);
-        assert_eq!(req.track, RatedTrack::Current);
-        let _req = RatingRequest::try_from("******").unwrap_err();
-    }
-}
diff --git a/pkgs/by-name/mp/mpdpopm/src/storage/mod.rs b/pkgs/by-name/mp/mpdpopm/src/storage/mod.rs
index 325b633a..d64f17c1 100644
--- a/pkgs/by-name/mp/mpdpopm/src/storage/mod.rs
+++ b/pkgs/by-name/mp/mpdpopm/src/storage/mod.rs
@@ -16,10 +16,6 @@ pub enum Error {
         source: crate::clients::Error,
         back: Backtrace,
     },
-    Rating {
-        source: crate::ratings::Error,
-        back: Backtrace,
-    },
 }
 
 impl std::fmt::Display for Error {
@@ -31,7 +27,6 @@ impl std::fmt::Display for Error {
                 write!(f, "Couldn't get system time: {}", source)
             }
             Error::Client { source, back: _ } => write!(f, "Client error: {}", source),
-            Error::Rating { source, back: _ } => write!(f, "Rating error: {}", source),
         }
     }
 }
@@ -93,7 +88,10 @@ pub mod play_count {
         #[tokio::test]
         async fn pc_smoke() {
             let mock = Box::new(Mock::new(&[
-                ("sticker get song a unwoundstack.com:playcount", "sticker: unwoundstack.com:playcount=11\nOK\n"),
+                (
+                    "sticker get song a unwoundstack.com:playcount",
+                    "sticker: unwoundstack.com:playcount=11\nOK\n",
+                ),
                 (
                     "sticker get song a unwoundstack.com:playcount",
                     "ACK [50@0] {sticker} no such sticker\n",
@@ -188,9 +186,9 @@ pub mod rating_count {
     pub const STICKER: &str = "unwoundstack.com:ratings_count";
 
     /// Retrieve the rating count for a track
-    pub async fn get(client: &mut Client, file: &str) -> Result<Option<u8>> {
+    pub async fn get(client: &mut Client, file: &str) -> Result<Option<i8>> {
         client
-            .get_sticker::<u8>(file, STICKER)
+            .get_sticker::<i8>(file, STICKER)
             .await
             .map_err(|err| Error::Client {
                 source: err,
@@ -199,7 +197,7 @@ pub mod rating_count {
     }
 
     /// Set the rating count for a track
-    pub async fn set(client: &mut Client, file: &str, rating_count: u8) -> Result<()> {
+    pub async fn set(client: &mut Client, file: &str, rating_count: i8) -> Result<()> {
         client
             .set_sticker(file, STICKER, &format!("{}", rating_count))
             .await