Layout Change: Queue View Widget

SO9010 commented 4 months ago

This is addressing #476 on creating a new layout and this is a draft PR so that @jacksongoode is able to collaborate with me.

Features:

[x] Fix sizing on the left and right panel
[x] A sidebar on the right which hides with depending on the window size
[x] A new divider for the left and right sides as that was brought in by the split view which I changed to be a flex row so then the left and right bars don't take up too much space
[x] Queue handling functions
- Clear queue
- Remove from queue

Questions:

Is it worth aligning some of the dividers for aesthetic purposes?
Should we have a function which skips to the place in the user queue where the user clicks, I would advocate for this?
What should we do after the song has been played from the queue, should it be removed from the view, or should we implement some sort of auto scroll?
I think we should have an option to disable this queue somewhere, maybe in settings or maybe in the top right of the title bar like here:

Current look of this PR:

https://github.com/jpochyla/psst/assets/77629938/e1d097dc-89ab-4cdb-8585-583db7b69625

jacksongoode commented 4 months ago

I'll have a look at this tonight! Very excited!

jacksongoode commented 4 months ago

Just made a few cleanups just to get consistency, however, it might be better to use reuse objects rather than recreate them. There's some real annoying issue where that horizontal separator is 1px too low. Feel free to revert or change.

jacksongoode commented 4 months ago

@SO9010 Hey Sam, just curious if you've had the time to look over the visual changes so far?

SO9010 commented 4 months ago

Hey @jacksongoode, sadly I don't have access to my computer! Your changes look great to me. That bug where the separator is off by 1px is very annoying! I will take a look when can and try to maybe hack a fix for it. But it still looks a ton better than with just a small separator bar.

Also, I'm thinking of implementing some sort of hover function to the close button to make it dimmer when not needed as it seems a bit intrusive and bulky right now.

I think the Ul is looking really good! I'm happy to take any suggestions. :)

SO9010 commented 4 months ago

Okay so an update. I have been working on this today and have come up with a solution for the off by one pixel issue and that was to fix the height. Additionally I added the ability to add dividers between the left and right sides! I also aligned the bottom player with the left controller section!

However, I am a bit stuck on how I get the position of the song in the queue to get removed after the button has been hit! Any suggestions @jacksongoode ??

jacksongoode commented 4 months ago

@SO9010 Just noticed this little overlap with the widgets of the section selectors overlaying the border.

I think this is a result of removing the splitter bar method from the split change solid_bar() https://github.com/linebender/druid/blob/4fe393927f8e59baacb53527a665980a157a0ad4/druid/src/widget/split.rs#L141

SO9010 commented 4 months ago

I have fixed the overlapping issue by reintroducing the split widget between the left sidebar and the main section. Additionally, I have created a new section called the right_bar. Now, we have both a left_bar and a right_bar. This change ensures that the queue widget won't leave space behind as it doesn't dynamically resize, which still leaves space even when there is no widget there.

I have a question now, how should we deal with it when a song has been played from the queue? Should we remove it from the widget, or should we have it, so the user is able to essentially skip backwards?

SO9010 commented 4 months ago

I have made it so the user can essentially skip backwards in the queue, as that feels quite fluid.

SO9010 commented 4 months ago

Hi @jacksongoode,

Could you please take a look at the program? I'd like to outline a few things:

The reason for not reusing the widget for the tracks is that we need to have an iterator method. I found this a bit complex to do, so I think it's best to just leave it as a new widget, you are welcome to change this of course.

There is currently an issue that is very similar to issue #477. I need some help with this because I can't figure out how to single out the position of a track in the list view, as right now it just removes the first song with the same ID, but we could remove it using the index.

I'm thinking that we need some sort of indication for which song from the queue is currently playing. What do you think?

I have made it so that the queue hides if there are no items added by the user, which helps maintain the aesthetics of the program nicely.

There is also an issue of what happens when a user clicks on the queue widget before playing anything from a playlist, which I still need to solve!

Here is a video of how it currently looks:

https://github.com/user-attachments/assets/e938f076-a278-4587-bb55-4165090e2098

jacksongoode commented 4 months ago

Hey @SO9010, I was just looking at the changes with the new commits you've added, I think we should probably follow how Spotify handles queued songs the same way. For example, if you cue a bunch of songs and pick the last song in the queue list, all of the other songs before that song should get removed.

SO9010 commented 4 months ago

Yes makes sense!

jacksongoode commented 4 months ago

There is currently an issue that is very similar to issue https://github.com/jpochyla/psst/issues/477. I need some help with this because I can't figure out how to single out the position of a track in the list view, as right now it just removes the first song with the same ID, but we could remove it using the index.

I do think we should probably fix this as it's own issue. The way's ID's should exist should contain their index in the list.

SO9010 commented 4 months ago

Ok makes sense, I think I'm starting to understand the issue more so will be able to work on it after this has merged!

jacksongoode commented 4 months ago

https://github.com/jpochyla/psst/pull/499 This should fix the duplicate ID track issue. You can use the same method to target the index of the queue list.

jacksongoode commented 4 months ago

Now that https://github.com/jpochyla/psst/pull/499 is merged, we can rebase and the fix should be here.

SO9010 commented 4 months ago

I had to implement it slightly differently because I was unable to reuse the normal view widget, which has to take an iterable track. The queue works through a vector of QueueEntry's. However, I have started implementing a similar way of collecting the song index, which is working very well :)

Edit: I may have completely miss interpreted you

SO9010 commented 4 months ago

Okay, here's a pleasing update: the end is near! I have implemented the remove by index, which has worked very well and removed the big issue, and now I have implemented the method which skips to a certain part of the

There are very few things I have to do:

[ ] Remove first song after played
[ ] Trigger audio playback after queue is clicked
[x] Settings option to hide queue

Edit: added check boxes

SO9010 commented 4 months ago

@jacksongoode, how should we do it with clearing the queue? I have a button to the right of the title, but it looks wrong. What do you suggest?

jacksongoode commented 4 months ago

One thing I was thinking is that currently the queue items don't have context menus (right click) like the rest of the tracks widgets.

I think we could add context entries to these queue widgets with one of the context items being "Clear all from Queue". The other context items could be "Removed from Queue" or "Add to Playlist" along with the other ones that we can confidently say would apply to queue items as well (I think that would be most).

This way we can hide lesser used items like that in the menu. What do you think? I still think the remove icon for each track is nice, though it would be great if only appeared on over over the track.

jacksongoode commented 4 months ago

I think the last two things are:

Click to play from queue
When a track from the queue gets played, remove it

Then we can go over and clean up!

SO9010 commented 4 months ago

Yes, but I have quickly added a few bits to the context menu!

SO9010 commented 3 months ago

There is, in fact, another issue!! When the user clicks on the first item in the queue, it doesn't do anything!

SO9010 commented 3 months ago

Alright, @jacksongoode, I have a question! Do you think we should do something slightly different to how it currently functions and follow Spotify more? Currently we have two places where it shows the currently playing song, at the top of the queue, and in the playcontrol section, but this doesn't need to be. We could remove it as it gets played, as the user will see it going to the now-playing section at the bottom with all the controls.

jacksongoode commented 3 months ago

Right, I noticed that! Yeah I do think we should pop the queued item the moment it gets played - that's how Spotify does it. So when a new queued item gets promoted to play, we immediately remove it from the queue list.

SO9010 commented 3 months ago

Ok, so progress again today; I have managed to make it so the first song gets popped after it played. Now, all that is needed is to initiate playback from the queue!

jacksongoode commented 3 months ago

With these recent changes, it seems like the queue that gets added to the sidebar, even after being exhausted, does not reflect the internal queue. It doesn't seem like the internal queue gets removed at the same time. If you change context it does seem to stay removed however...

So:

Add queue
Play a random song
Stay in context of playlist
Play through whole queue
Click another song
Issue: Songs that follow will be the whole queue again

SO9010 commented 3 months ago

Okay so could you possibly check this out for me :P thank you

jacksongoode commented 3 months ago

It seems like there might be a bug around how the items in a queue get triggered when they change from a selected item into the queue. I get a really long spinning wheel and the track plays, but I think that indicates the method that triggered the playback might be wrong.

To reproduce:

Open
Queue a song from context 1
Play a random song in a different context 2
Click next or finish song, queued item will have spinning wheel

I imagine the click on queue to play action might still need work? I can produce a panic when I click on some queued items, nothing plays but it puts the player in a bad state :)

SO9010 commented 3 months ago

@jacksongoode This should be good now. Could you please check it for me?

I found the issue. It came from the fact I was deleting the first song from the queue when it was being played, but it needs this to know where the playback origin is, so I added a display vector for this.

jacksongoode commented 3 months ago

Checking it out now! Thanks!

jacksongoode commented 3 months ago

Ugh unfortunately I cant login since my config was delete and Spotify has revoked the user/pass method. So I'll need to make a PR to address that.

jacksongoode commented 3 months ago

Ok I'm halfway done with that auth PR, just enough to get a good token.

So I see two issues here:

Queuing a song and then click on that item at the top of the queue should just pop the song and play it. Currently only clicking on items after the first will trigger them to play.
Clicking pops wrong song:
- Queue 3 songs
- Play a random song to start playback
- Skip 3 songs
- Queue 3 more songs
- Click 2nd queued item from top
- See that song played is not the clicked track (and it seems some queue order is also scrambled?)

SO9010 commented 3 months ago

Hello, thank you so much for checking it out! These issues should be relatively easy to sort out! :) I'll let you know when I've sorted that out. Sometimes these bugs slip as I've been looking at the same thing for a while :)

jacksongoode commented 2 months ago

I think this PR may need to be rebased but I could be wrong - I recently merged a fix to the builds and the oauth implementation.

SO9010 commented 2 months ago

Ok, so I've had another look at this; in my opinion, it is almost done; there is an issue where when removing the song that is currently playing, it removes it if someone clicks on the song that is about to play then it registers as the song that was queued:

// TODO: this falsely removes the song if you click on a song from the playlist that is also in the queue, not sure how to solve this? if !data.displayed_added_queue.is_empty() && data.playback.now_playing.as_mut().is_some_and(|np| { np.origin.to_string() == PlaybackOrigin::Queue.to_string() && np.item.id() == data.displayed_added_queue[0].item.id() }) { data.displayed_added_queue.remove(0); }

So @jacksongoode is there any way you can think that will help this?

jacksongoode commented 2 months ago

it removes it if someone clicks on the song that is about to play then it registers as the song that was queued:

Oh I think the solution here is the context. Queued items should have some position or label that makes it clear that on clicking on items that are present in the queue should remove those queued items? It seems like a design bug if clicking anywhere else has any interaction with the queue's poping off the stack.

SO9010 commented 2 months ago

Yes I agree but annoyingly there will probably need to be some big refractoring needed as currently the way that the origin of a currently playing song is done through this function:

pub fn queued_entry(&self, item_id: ItemId) -> Option<QueueEntry> {

        if let Some(queued) = self
            .added_queue
            .iter()
            .find(|queued| queued.item.id() == item_id)
            .cloned()
        {
            Some(queued)
        } else if let Some(queued) = self
            .playback
            .queue
            .iter()
            .find(|queued| queued.item.id() == item_id)
            .cloned()
        {
            return Some(queued);
        } else {
            None
        }
    }

I added the top bit, which forces it to assume that if the items are in the added queue when that song ID is playing, it must be from the queue. However, this is where the issue comes in. I think I could possibly change this but this is quite core logic so id like to avoid completely changing this function.

I've been looking at the idea of having a variable which says whether it is playing from the queue, much like I have done with the actual queue code. But I'm struggling with the implementation.

Is there maybe a way to get a function to parse through a value in things like this section:

fn handle_command(&mut self, cmd: PlayerCommand) {
        match cmd {
            PlayerCommand::LoadQueue { items, position } => self.load_queue(items, position),
            PlayerCommand::LoadAndPlay { item } => self.load_and_play(item),
            PlayerCommand::Preload { item } => self.preload(item),
            PlayerCommand::Pause => self.pause(),
            PlayerCommand::Resume => self.resume(),
            PlayerCommand::PauseOrResume => self.pause_or_resume(),
            PlayerCommand::Previous => self.previous(),
            PlayerCommand::Next => self.next(),
            PlayerCommand::Stop => self.stop(),
            PlayerCommand::Seek { position } => self.seek(position),
            PlayerCommand::Configure { config } => self.configure(config),
            PlayerCommand::SetQueueBehavior { behavior } => self.queue.set_behaviour(behavior),
            PlayerCommand::SkipToPlaceInQueue { item } => self.queue.skip_to_place_in_queue(item),
            PlayerCommand::ClearQueue => self.queue.clear_user_items(),
            PlayerCommand::AddToQueue { item } => self.queue.add(item),
            PlayerCommand::RemoveFromQueue { item } => self.queue.remove(item),
            PlayerCommand::SetVolume { volume } => self.set_volume(volume),
        }
    }

As then for example we could have a player command GetIfPlayingUserItem, then from there, this would all be streamlined and could even be placed in the Appdata section to be more easily accessible.

jacksongoode commented 1 month ago

I think this needs to be on a branch? It seem like there's some weird conflict I'm getting by this being on your master branch? Generally every PR should be on a branch with its name.

SO9010 commented 1 month ago

So, before I think this is possible, I think we need to find a way to give the playback origin without just having it detected after it's been played.

jpochyla / psst