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

Added Random view #1061

Closed
wants to merge 8 commits into from
Closed

Added Random view #1061

wants to merge 8 commits into from

Conversation

rstefko
Copy link
Contributor

@rstefko rstefko commented Apr 2, 2023

See #1052 for details.

@paulijar
Copy link
Collaborator

Thanks for the PR and sorry for the long delay in giving any feedback. I tested this briefly a week ago but didn't have the time to write my impressions. Here are some thoughts:

  1. I can see that this gives at least some added value. However, more work is needed before I could consider merging it.
  2. The thing bothering me the most is how the view always re-randomizes its content upon view switch. This is not in line with the rest of the app and not very good user experience: view navigation back and forth should not be a destructive operation. An idea: maybe the first randomization could happen when the view is first entered and a new randomization would be available via a context menu item on the navigation pane.
  3. The All tracks view has lot of complexity which would be totally unnecessary here. The whole "bucket" concept and using the in-view-observer directive have been invented to be able to handle lists of tens of thousands of tracks. When there can be only 300 tracks in the view, the only thing needed is a single track-list directive.
  4. In the Subsonic API and at least on the DSub client, the "random songs" feature allows the user to optionally define start year, end year, and/or genre. Having that kind of ability here would bring some genuine added value and make it more obvious, why this view should exist in addition to the shuffle feature.
  5. Please rebase your PR. I will not merge a complex mess of merge commits, I like my history nice and linear. Also, it would probably make your life easier to make the PR out of a separate feature branch instead of using the master branch of your fork for this.

@paulijar
Copy link
Collaborator

In case you are interested in implementing the optional filters mentioned in the point 4, the UI flow could work similarly as when selecting "Internet radio" > "Add manually". That is, the filter selections and confirmation button could be shown in the sidebar.

@paulijar
Copy link
Collaborator

@rstefko Are you planning to work further on this feature? It's also possible that I will pick up from where you left and do some finalization and polishing. But I'm not certain when would that happen and don't want to step on your toes. There will probably be an app release v1.9.0 during or after July with some new features, and also this one could maybe be released there.

@rstefko
Copy link
Contributor Author

rstefko commented May 29, 2023

@paulijar I am still testing it. I have pushed my latest changes. It looks like the sort by play count is finally fixing the initial problem #1052. It would be great if you could finish it. Thanks Roman

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

paulijar added a commit that referenced this pull request Jul 14, 2023
@paulijar
Copy link
Collaborator

I just merged a "Smart playlist" feature branch which I have been building on top of the commits from this PR. One of its features is to generate a playlist containing random not recently played tracks but it can do also many other things.

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