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

Add feature for adding track URI directly to the queue #216

Merged
merged 9 commits into from
Jan 14, 2017

Conversation

jcass77
Copy link
Member

@jcass77 jcass77 commented Dec 11, 2016

Adds popup menu options for adding a track at any point in the queue, as well as two new buttons on the queue page for doing the same.

skitched-20161211-150754

Fixes #75.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 41.037% when pulling 910ad3f on jcass77:enhance/75_add_track_uri_to_queue into c264bc5 on pimusicbox:develop.

@kingosticks
Copy link
Member

Before getting into the code (which I've not read through yet) can we discuss what direction we want going forward. There is some overlap between this stuff and and 'Streams' page functionality which could be modified to do something similar. This PR provides more flexibility (e.g. adding after a particular song) but I think we need to be careful about what extra buttons and context-menu options we start adding; it's so easy to get carried away and end up with a wall of options forcing users to have to think too much.

A UI to support track insertion is also required for playlist modification, so do we want to think now about a unified interface for things like this? I'm talking more about UI consistency rather than code re-use (although that'd be nice too).

Of course, we can remove things later but we might save ourselves some work now. What are your thoughts?

@jcass77
Copy link
Member Author

jcass77 commented Dec 16, 2016

Thanks for the feedback @kingosticks. My response is a bit long so I'll break it into sections:

Current features:

  1. I think this PR could be the first step towards getting rid of the Streams page (as discussed in Ability to add youtube, mp3s or other streams to queue instead of playing immediately. #75). The only feature that that page provides that is not available elsewhere at this stage is the 'Get currently playing' button. This is maybe also something that we would want to add to the context menu so that users can get the URI for any track whenever it is displayed on screen (in other words a 'Copy' entry like you would see in most other application context menus). If we agree on that then we can create an issue to track moving that feature.

  2. Playlists support has probably been left behind a little recently, but that is what Feature: Manage playlists #164 is for. I've been putting it off in favor of working on the smaller 'power user' features specifically because code re-use is so difficult and it seems every new feature becomes a bit of a copy and paste exercise. The codebase needs to be refactored to make reuse easier and the only way I would feel comfortable with a wholesale refactor myself is if we have proper test coverage for everything. We're not there yet and a lot of the javascript still needs to be modularized to make testing possible.

Direction going forward:

As far as the direction of the MMW project is concerned: I think there are a bunch of really good looking and easy to use frontends out there, with new ones starting up all the time. There are simple frontends for older devices, custom-built ones for touch screens, frontends that focus on mobile, frontends that aim to enhance the overall music experience by pulling in additional content, frontends built to integrate closely with Spotify, and really attractive frontends built around the aesthetics of the UI.

I like MMW because it works just as well on mobile as it does on the desktop, and most of the Mopidy features that I care about are only a swipe or a click away. My feeling is that the niche that we should be aiming for is to offer a flexible, full featured, generic web frontend that allows experienced users to get the most out of their Mopidy server. I suspect that a lot of people were introduced to Mopidy as a result of the MusicBox project (I know I was), and if there will be more releases of MusicBox in the future then the features for editing settings and managing the system will be really important too in order to lower the barrier of entry.

Unified interface:

We could probably get away with no more than four buttons on-screen in the top right corner at any given time (these split into groups of two on small screens), and use the context menus for the balance. This works well for giving the user access to the most commonly used features all of the time while keeping the UI relatively uncluttered by tucking the other options away in an off-screen menu. There will be a limit to the number of entries that the context menu can contain before the array of options become bewildering but I don't think we've reached that point yet.

@woutervanwijk did great work coming up with the UI in the first place, which is what attracted me to MMW initially. Perhaps he can chime in with a few thoughts as well...

@jcass77
Copy link
Member Author

jcass77 commented Jan 6, 2017

@kingosticks, and further thoughts on this?

@kingosticks
Copy link
Member

kingosticks commented Jan 6, 2017

I think it's important we maintain the goal of being simple to use, to this end I really don't want to clutter the screen (or context menu) with options.

I'm not convinced it's necessary for there to be two buttons since you have to deal with a popup anyway. I'd be more inclined to have just the one '+' button, and then have a radio button (dropdown/whatever) to specify where it should be added. Ultimately the popup could have a few options, if one of these was to "get now playing" then the same popup could maybe be reused for simple playlist editing. (Another nail in the coffin for the 'Streams' page).

We can still merge this right now and work on refining it later.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 40.761% when pulling 7892218 on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@jcass77
Copy link
Member Author

jcass77 commented Jan 6, 2017

@kingosticks I think I understand what you are recommending here. I've pushed 45dd2d6 which removes one of the buttons on the main queue page, and adds more options to the popup dialog box instead:

skitched-20170106-212242

The dialog box is generated dynamically based on whether a track is currently playing, and whether the user launched the dialog from the context menus or the queue page - we can refine this further if you think it is still too busy.

We should be able to re-use a lot of this for editing playlists as well.

@kingosticks
Copy link
Member

So fast! This looks just like I was thinking, awesome.

When you add from the context menu, you then get three options in the drop down, but selecting "Play Next" does the wrong thing and adds it after the selected track (same as the 1st option).

By the way, do you use a Mac? (Cancel button on the left of the Add button).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 40.572% when pulling faf146b on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@jcass77 jcass77 force-pushed the enhance/75_add_track_uri_to_queue branch from faf146b to dd9d72e Compare January 7, 2017 16:14
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 40.572% when pulling dd9d72e on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@jcass77 jcass77 force-pushed the enhance/75_add_track_uri_to_queue branch from ab831ad to a9c998c Compare January 7, 2017 16:32
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 40.572% when pulling a9c998c on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 40.572% when pulling a9c998c on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@jcass77
Copy link
Member Author

jcass77 commented Jan 7, 2017

When you add from the context menu, you then get three options in the drop down, but selecting "Play Next" does the wrong thing and adds it after the selected track (same as the 1st option).

Oops, that is probably my queue to update the test cases. I've done that and pushed a fix rebased on upstream/develop.

I've also added a 'Show Track Info' option to the context menus, primarily to allow the user to retrieve the URI for any arbitrary track that is shown in the player (works well for adding search results to the queue at a specific location):

skitched-20170107-183604

The URI should already be highlighted when the popup opens to make copying the the clipboard easier (does not seem as if there is a way to do this in mobile devices however).

skitched-20170107-183734

By the way, do you use a Mac? (Cancel button on the left of the Add button).

Yes I do use Mac, and started to standardize the layouts of all of the popup dialogs some time back to try and get some sort of guideline established - discussed here: #195 (comment)

@kingosticks
Copy link
Member

This is great and I think that Track info is a worthy entry in the context menu, very useful.

Did you notice how the popup width resizes as you select the different drop down menu items? It's a bit visually odd, particularly if you have a really long track name. I guess the context popup does the same thing but it's less noticeable. Not a big deal, LGTM as is.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 40.385% when pulling f3bc3e9 on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@jcass77
Copy link
Member Author

jcass77 commented Jan 8, 2017

I've pushed some new commits which sets a minimum width for all popup dialogs on desktop devices. This won't prevent the resizing effect in extreme cases where a track name is unusually long, but it does prevent the dialog from resizing for the static default options.

I also added a mobile device check that prevents the keyboard from popping up the moment that the user opens the 'Show Track Info' dialog.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 40.385% when pulling 3a55453 on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 40.385% when pulling 4f9e370 on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 40.385% when pulling dfffcbf on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@jcass77
Copy link
Member Author

jcass77 commented Jan 14, 2017

I've made a few more improvements to the layout of the 'Show Track Info...' popup dialog.

Also fixed and issue with track duration not showing up in the popup.

I'll merge this later today if there are no further inputs...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 40.652% when pulling 5020815 on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 40.652% when pulling 45d6e67 on jcass77:enhance/75_add_track_uri_to_queue into f3a45d0 on pimusicbox:develop.

@jcass77 jcass77 merged commit 37b267d into pimusicbox:develop Jan 14, 2017
@jcass77 jcass77 deleted the enhance/75_add_track_uri_to_queue branch February 5, 2017 01:31
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.

3 participants