search

spezifisch / stmps

Subsonic Terminal Music Player S
GNU General Public License v3.0
19 stars 6 forks source link

Implements #36: adds ability to reorder the queue #37

Closed xxxserxxx closed 2 months ago

xxxserxxx commented 2 months ago

This adds the ability for the user to re-arrange items in the queue.

  1. If the music is playing and the top item changes (it's either moved down, or the one below is moved up), the music is stopped before the move. The music is not automatically restarted. This was to prevent jittering while re-arranging.
  2. Moving the top item up, or the bottom item down, is a NOP: it doesn't stop the music or change anything. It does, however, log a non-error message.
  3. The Queue help text and README have been updated.
  4. Bounds are checked in both the queue and the player struct methods; belts and suspenders.

If we think of better hotkeys, it's easy enough to can change them. They mirror vim at the moment.

P.S. I added a queue shuffle function under the S key, b/c it was easy and I mentioned it in the request ticket.

xxxserxxx commented 2 months ago

I added comments to the two new functions in page_queue.go, but not on the two new functions in player.go. This was simply because there were already comments on the surrounding functions in page_queue.go and not in player.go, and it's more an oversight than intentional. I think an effort to comment functions throughout the code would be beneficial, although it doesn't really excuse this omission. 🤷🏻

The stupid issue reference is wrong, again, and points to wildeyedskies' repos. At some point, I may just delete my repos and re-fork it from your's; that'd fix the github references in the future -- I've just been lazy. I'll manually insert a reference in the proper ticket.

xxxserxxx commented 2 months ago

@spezifisch it seems impossible to tell github to switch the upstream project, and the work-around is to delete the fork and re-fork from the new(er) project. Having github think wildeyedskies' project is where I want everything to is a PITA, so if/when you merge this PR, I'm going to do this change: delete my wildeyesskies fork and refork from yours'. Frankly, I don't know what side-effects that'll have, but since you've merged every PR except this one, I won't have anything outstanding preventing me from doing this. Oh, and I can't just fork yours and keep the fork of wildeyesskies because, even thought they're differently named, github sees the fork history and prevents a second fork.

Just FYI.

spezifisch commented 2 months ago

Thanks, that looks solid.

@spezifisch it seems impossible to tell github to switch the upstream project, and the work-around is to delete the fork and re-fork from the new(er) project. Having github think wildeyedskies' project is where I want everything to is a PITA, so if/when you merge this PR, ...

Yeah, that's really annoying behavior. I get similar issues when merging different branches that are not based on "upstream" via the Github's UI.

xxxserxxx commented 2 months ago

Hey, there's a linting error in here because I'm not checking the error return value of the calls to Stop(). We don't care what the return values are, because it's just because we're changing the order of item #0, which is the song that's currently playing, and it would make the UX more weird if we didn't. If it fails, it doesn't really impact the rest of the function execution.

So, I could do one of three things:

  1. Make the linter happy by checking the return value. It wouldn't change anything, because it's possible the server could return an error if we call Stop() and it's already stopped. Or, we could abort the operation. At the very least, we could log it, but -- again -- if we get an error we don't care about, neither should the user.
  2. Make the linter happy by changing this to _ := player.Stop(), which is stupid code IMO
  3. Ignore the linter errors, except that it causes the automated build to fail.

OTOH, I was looking through your actions and, while it seems you've been finding and fixing other things in the PRs, I didn't see you make a change for this one. Is it possible we're using different linters?

spezifisch commented 2 months ago

So, I could do one of three things:

I would choose 2., at least it's simple and I care about the CI build. (Notifications are kind of getting out of hand here.) Ideally I would like to have a proper logging (or I guess it's more "tracing" here) framework, so we could get access to these kind of debug messages when investigating bugs.

PS: I'm sorry I just noticed that you created a discussion and an accompanying issue. Let's better continue talking in the issue instead of a merged/closed PR.