Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Layout Change: Queue View Widget #485

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

Conversation

SO9010
Copy link
Contributor

@SO9010 SO9010 commented Jun 30, 2024

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:

  • Fix sizing on the left and right panel
  • A sidebar on the right which hides with depending on the window size
  • 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
  • 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:
    image

Current look of this PR:

2024-06-30.13-13-11.mp4

@jacksongoode
Copy link
Collaborator

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

@jacksongoode
Copy link
Collaborator

jacksongoode commented Jul 2, 2024

image

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
Copy link
Collaborator

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

@SO9010
Copy link
Contributor Author

SO9010 commented Jul 10, 2024

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
Copy link
Contributor Author

SO9010 commented Jul 12, 2024

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!

image

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 ??

@SO9010
Copy link
Contributor Author

SO9010 commented Aug 8, 2024

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

@jacksongoode
Copy link
Collaborator

jacksongoode commented Aug 9, 2024

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.

image

To reproduce:

  1. Open
  2. Queue a song from context 1
  3. Play a random song in a different context 2
  4. 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
Copy link
Contributor Author

SO9010 commented Aug 24, 2024

@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
Copy link
Collaborator

Checking it out now! Thanks!

@jacksongoode
Copy link
Collaborator

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
Copy link
Collaborator

jacksongoode commented Aug 25, 2024

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

So I see two issues here:

  1. 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.

  2. 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
Copy link
Contributor Author

SO9010 commented Aug 25, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

SO9010 commented Sep 19, 2024

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?

@SO9010 SO9010 marked this pull request as ready for review September 19, 2024 14:09
@jacksongoode
Copy link
Collaborator

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
Copy link
Contributor Author

SO9010 commented Sep 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants