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

Random album access version 1 #555

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

Conversation

Octoton
Copy link
Contributor

@Octoton Octoton commented Dec 30, 2021

First part of the perpetual queue implementation following queue revamp. For this version only album (hardcoded) are shufflelabe and there is no settings to focus the random search on specific thing (to lighten the PR size)

When activated a next album will be randomly choosen and display at the end of the queue, you can ask for another one with some choice or stop the perpetual queue:
Screenshot_20211230-212009

There is more than one way to start a new perpetual queue:
Screenshot_20211230-212004
Screenshot_20211230-211955
Screenshot_20211230-211931

On other screen than the album one the classic shuffle song is shown (if we add other shuffling like by artist for example, we can change the same way than the album tab the artist tab):
Screenshot_20211230-211926

There is a settings to remember or not if the perpetual queue was activated or not:
Screenshot_20211230-212340

@Octoton
Copy link
Contributor Author

Octoton commented Dec 30, 2021

@soncaokim there is the first of two PR to implement a random album shuffling with easy way to have other type in the futur

@soncaokim
Copy link
Collaborator

Hey, sorry I did not have time nor courage to answer you.

I just have an extreme quick look, and havent noticed anything blocking. The concept of DynQueue appears in core classes, so this should be kept minimal.

Is this stable enough for me testing? Or I should stay within the safe distance for now?

@Octoton
Copy link
Contributor Author

Octoton commented Jan 4, 2022

@soncaokim I did this mounth ago and I haven't have problem with it ever since (code it just after static queue, but got side track on other stuff. Well better late than never i guess)
There is a next PR with ensure no constant repeat and less random album each time, but this PR should work nonetheless.

@soncaokim
Copy link
Collaborator

  • Can you please solve the (minor) merge conflict?
  • The changes on core *Fragment and *Activity classes bother me a bit, since they need to be aware of the underlying queue implementation to do refresh/reload. I dont have any concrete counter proposition though.
  • If one wants to propose a new dynqueue idea, the only change on core classes would be MusicService.restoreQueuesAndPositionIfNecessary correct ?

@Octoton Octoton force-pushed the random_album_access_version_1 branch from 3404471 to dd30e6f Compare January 5, 2022 20:45
@Octoton
Copy link
Contributor Author

Octoton commented Jan 5, 2022

  • Done (had to move the staticQueue correction you did in Fix crash on restoring queue after song removal #547, see new commit)
  • Me too :), but I don't see any really clean way to do this
  • More like MusicService.setQueueToDynamicQueue, restoreQueuesAndPositionIfNecessary is there to restore an already created queue. But yes, the idea is that when you change the QueueLoader associated with the dynamic queue, everything change (this will be more clear with the next PR).

@soncaokim
Copy link
Collaborator

  • I'm under the impression that when this is enabled, the very first song of each album is not queued/not played.

  • i dont think that the "stop perpetual queue" should be colored red, ie not an destructive action as the case of song deletion.

Screenshot_20220106-224224-544

@Octoton
Copy link
Contributor Author

Octoton commented Jan 7, 2022

  • I'm under the impression that when this is enabled, the very first song of each album is not queued/not played.

  • i dont think that the "stop perpetual queue" should be colored red, ie not an destructive action as the case of song deletion.

This is strange because for me the first song is always played, could you put the step by step way that you used to see this behavior ?

More than destructive action, the red button inside a menu is there to quickly warn user with quick thumb that the action will be an hassle to revert => when you stop the perpetual queue by mistake and then begin a new one, next song will be different and in future implementation other stuff could be lost as it stop all dynamic services (for example in next PR, album listening history will be flush, thus you can have the an album that you just listen too that is proposed to you).
In contrast to this, when you remove a song (only action that is close but not red) from a queue by mistake you can just add it back.

Copy link
Collaborator

@soncaokim soncaokim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

@soncaokim
Copy link
Collaborator

@Octoton Can you please fix the merge conflict? Afterwards I think we can merge this, @AdrienPoupa?

@Octoton
Copy link
Contributor Author

Octoton commented May 2, 2022

Hi, sort if forgot about this to be honest 😅
Will try to do this in the week

Octoton added 17 commits May 3, 2022 21:40
Forbid dynamic queue to update every time queue is modified
First trad implementation
Toast if no album are found next
correction on repeat mode in dynamic mode
…mage transition from albumDetailActivity back to previous activity wasn't working
@Octoton Octoton force-pushed the random_album_access_version_1 branch from 6e38267 to 19fbb2e Compare May 3, 2022 21:00
@Octoton
Copy link
Contributor Author

Octoton commented May 3, 2022

@soncaokim there, it should be ok to merge now

@Octoton Octoton force-pushed the random_album_access_version_1 branch from 19fbb2e to 6e48905 Compare May 3, 2022 21:22
@Octoton
Copy link
Contributor Author

Octoton commented Jun 17, 2022

?

@stale
Copy link

stale bot commented Oct 15, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 15, 2022
@Octoton
Copy link
Contributor Author

Octoton commented Oct 16, 2022

?

@stale stale bot removed the wontfix label Oct 16, 2022
@soncaokim
Copy link
Collaborator

Hi @Octoton ,

Given there is no response/activity from @AdrienPoupa for a while, and there is no release since ~1y, I'm considering forking this repo. While there is no guarantee that I can commit more time than right now on it, at least I can have the control on the release process and eventually see what is blocking.

The ideal setup would be to turn this repo to a team repo (and not a personal one), but I dont know if Adrien is okay or if he even follow this anymore.

S.

@Octoton
Copy link
Contributor Author

Octoton commented Oct 17, 2022

Seems like a great way to keep this project alive

@soncaokim
Copy link
Collaborator

Given there is no response/activity from @AdrienPoupa for a while, and there is no release since ~1y, I'm considering forking this repo. While there is no guarantee that I can commit more time than right now on it, at least I can have the control on the release process and eventually see what is blocking.

Hi @Octoton, please have a look at this forked repo and this board where I've outlined steps to reach a v2 release.

Everything is still draft, and moving. And there are works to do before getting to a release-able state.

I've setup CI via Github Actions (which reuses the same CircleCI signing key, so that the development binaries can be upgraded seamlessly).

If you think this is a good target, is that possible to port this PR to the new repo? There should be some merging conflict to solve, but I'll try to assist there. I think this is a good time window to merge our PR in, before I start large-scale change on the code (renaming, linting, refactoring).

@Octoton
Copy link
Contributor Author

Octoton commented Jan 16, 2023

Given there is no response/activity from @AdrienPoupa for a while, and there is no release since ~1y, I'm considering forking this repo. While there is no guarantee that I can commit more time than right now on it, at least I can have the control on the release process and eventually see what is blocking.

Hi @Octoton, please have a look at this forked repo and this board where I've outlined steps to reach a v2 release.

Everything is still draft, and moving. And there are works to do before getting to a release-able state.

I've setup CI via Github Actions (which reuses the same CircleCI signing key, so that the development binaries can be upgraded seamlessly).

If you think this is a good target, is that possible to port this PR to the new repo? There should be some merging conflict to solve, but I'll try to assist there. I think this is a good time window to merge our PR in, before I start large-scale change on the code (renaming, linting, refactoring).

Will look into this in the week 👍

@AdrienPoupa
Copy link
Member

AdrienPoupa commented Jan 25, 2023

Hi @soncaokim, sorry for the inactivity, and thanks for all the work you have done.

I would like to help you by making working on Vinyl as easy as possible.

I thought of setuping actions so a new app can be published to the Play Store from this repo, what do you think about it? Possibly using something like https://github.com/marketplace/actions/upload-android-release-to-play-store

I will investigate this, but if you have pointers or other ideas on how I can help, please let me know!

@soncaokim
Copy link
Collaborator

Hi @soncaokim, sorry for the inactivity, and thanks for all the work you have done. I would like to help you setup actions so a new app can be published to the Play Store from this repo, I will investigate this, but if you have pointers on how to do it I would gladly help!

Hey there, long time no see! 😀

Well, as I stated above, I've started a fork and aim to build a team repo instead of personal repo, ie having the infra to let (trusted) people backing up each others. If we have a team of few trusted people, that would improve the current situation.

I would like also to have a "branding" continuity, so would like to ask you to reuse/base off the Vinyl app icon, and to name the new app Vinyl2 (or Vinyl Remastered/Reissued).
Would that be OK for you?

And if you think this can be a good common target, then please come join me.

Son

@soncaokim
Copy link
Collaborator

About the automation: this topic will be definitely an important focus for the new repo. I've already had a CI based on GithubActions running, with more coming.

@AdrienPoupa
Copy link
Member

You are of course free to fork this as you see fit, but I would think it would be more beneficial for the project and its user base to keep using this repository to avoid segregating the user base / downloads.

I am open to any idea you may have to reduce the bus factor (even though I am not planning to be run over in the near future ;))

I am not opposed to transferring the repo to an organization, if you feel this is a step in the right direction, but I feel it's important to keep the user base / stars / downloads for the continuity of the project.

@soncaokim
Copy link
Collaborator

I am not opposed to transferring the repo to an organization, if you feel this is a step in the right direction, but I feel it's important to keep the user base / stars / downloads for the continuity of the project.

That'll be a nice move if achievable. I'm afraid that this will imply sharing some secrets as well (release key, etc)

@soncaokim
Copy link
Collaborator

I am open to any idea you may have to reduce the bus factor (even though I am not planning to be run over in the near future ;))

Wha, what happened? If you mean it literally, then "bon retablissement"

@AdrienPoupa
Copy link
Member

AdrienPoupa commented Jan 25, 2023

I am open to any idea you may have to reduce the bus factor (even though I am not planning to be run over in the near future ;))

Wha, what happened? If you mean it literally, then "bon retablissement"

Nothing haha, I just meant it is better to have an organization so there is no single point of failure.

You will notice I created the VinylMusicPlayer organization and transferred the repo there. It has the benefits of keeping all the issues and stars, the former URL still works. I made you an admin of the organization so hopefully I am not a bottleneck anymore.

The last thing I see we still need to do it create an action to automate Play Store deployment, and possibly update FDroid metadata with the new repo URL: https://gitlab.com/fdroid/fdroiddata/-/merge_requests/12491

Let me know if you need anything else!

@soncaokim
Copy link
Collaborator

The last thing I see we still need to do it create an action to automate Play Store deployment, and possibly update FDroid metadata with the new repo URL.

Let me know if you need anything else!

Thanks, I'll have a look. If things are okay over there, I'll push a bunch of PR from my fork.
Let's continue over there, this thread is polluted with this discussion :-)

@AdrienPoupa
Copy link
Member

Let's continue the discussion here: #641

@soncaokim
Copy link
Collaborator

Hi @Octoton, do you plan to push this feature?

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

Successfully merging this pull request may close these issues.

None yet

3 participants