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

Fixed shuffle button opacity UI bug #6844

Merged
merged 5 commits into from
Nov 3, 2021

Conversation

0x416c6578
Copy link
Contributor

@0x416c6578 0x416c6578 commented Aug 4, 2021

Parameterised shuffle state into initPlayback for potentially passing the shuffle state into the player in the future

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Fixes the shuffle mode button UI bug and parameterizes the shuffle mode in initPlayback (to potentially allow passing the saved shuffle state into the player in the future)

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@0x416c6578 0x416c6578 changed the title Fixed shuffle button opacity bug Fixed shuffle button opacity UI bug Aug 4, 2021
Stypox
Stypox previously requested changes Aug 4, 2021
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Are you sure this doesn't reset the queue shuffle mode when switching players?

app/src/main/java/org/schabi/newpipe/player/Player.java Outdated Show resolved Hide resolved
@triallax triallax added the bug Issue is related to a bug label Aug 4, 2021
@TobiGr TobiGr added GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) labels Aug 29, 2021
@0x416c6578 0x416c6578 closed this Sep 21, 2021
@0x416c6578 0x416c6578 deleted the shuffle-mode-ui-fix branch September 21, 2021 09:12
@Stypox
Copy link
Member

Stypox commented Sep 22, 2021

@0x416c6578 have you done anything new? Should this be reviewed? Sorry for not noticing earlier, I didn't see the notification

@0x416c6578 0x416c6578 restored the shuffle-mode-ui-fix branch September 22, 2021 07:46
@0x416c6578
Copy link
Contributor Author

0x416c6578 commented Sep 22, 2021

I just realised I assumed it had been fixed by someone else since it wasn't a problem in the app on my phone, but then I remembered I am using builds from my own fork with the above fix lol. The only change I ended up making was adding the line onShuffleModeEnabledChanged(false); to handleIntent.

@0x416c6578 0x416c6578 reopened this Sep 22, 2021
@Stypox
Copy link
Member

Stypox commented Sep 27, 2021

Thank you, that's good. Could you verify this in the multiple conditions that initPlayback is called?

Are you sure this doesn't reset the queue shuffle mode when switching players?

Then this can be merged

@litetex litetex added the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Oct 17, 2021
@litetex
Copy link
Member

litetex commented Oct 17, 2021

Reminder ping @0x416c6578

@0x416c6578
Copy link
Contributor Author

0x416c6578 commented Oct 18, 2021

sorry, was a bit snowed down with uni work,
it seems to keep shuffle mode when going to and from background mode, however i think that in the future some sort of shuffle state flag will need to be added to better keep track of the shuffle mode (the starts of which was removed in 5513178).
when I have some time i'll work on a PR to do what was mentioned in #6844 (comment)

@github-actions github-actions bot removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Oct 18, 2021
@litetex
Copy link
Member

litetex commented Oct 18, 2021

@0x416c6578

Ty for the feedback.
Changing the state of the PR to a draft then.
You can change it back once it's ready for review.

@litetex litetex marked this pull request as draft October 18, 2021 18:01
@0x416c6578
Copy link
Contributor Author

0x416c6578 commented Oct 18, 2021

for reference the changes in this PR are working, so this could be merged. i'll just open another one with the aforementioned changes when i have some time :/ (unless you just want all the changes in one PR)

@litetex litetex self-assigned this Oct 23, 2021
Parameterised shuffle state into initPlayback for potentially passing the shuffle state into the player in the future
No need for doing the heavier method ``onShuffleModeEnabledChanged(false);``
@litetex
Copy link
Member

litetex commented Oct 23, 2021

Rebased the branch and rewrote the fix so it uses setShuffleButton(binding.shuffleButton, simpleExoPlayer.getShuffleModeEnabled()); instead.
Why?
We just want to fix the UI so no need to call the heavier onShuffleModeEnabledChanged which triggers additional events and un-shuffles the queue.

@litetex litetex marked this pull request as ready for review October 23, 2021 14:49
@litetex litetex removed their assignment Oct 23, 2021
@litetex litetex marked this pull request as draft October 23, 2021 15:02
@litetex litetex self-assigned this Oct 23, 2021
@litetex
Copy link
Member

litetex commented Oct 23, 2021

While testing I found out that the icon in the notification is broken and doesn't update:
grafik

Was also affecting the playqueue-activity.

Investigating...

The backup-list has to be created at all cost (even when current list size <= 2). Otherwise it's not possible to enter shuffle-mode (as ``isShuffled()`` always returns false)!
@litetex
Copy link
Member

litetex commented Oct 23, 2021

Fixed the problem:
The backup-list which determines if the playqueue is shuffled or not was not created when <= 2 items are in the current playlist. This made caused the notification-toggle to fail.

@litetex litetex marked this pull request as ready for review October 23, 2021 15:40
@litetex litetex removed their assignment Oct 23, 2021
litetex
litetex previously approved these changes Oct 23, 2021
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

Rewrote the fix as mentioned and tested it with a few scenarios.
Approved from my side.
If anyone else could test this PR that would be great.

@tsiflimagas
Copy link
Contributor

I'm not sure if by "anyone else" you meant some other dev or any user, but I tested this and seems to be working fine.

@litetex litetex dismissed Stypox’s stale review November 2, 2021 22:29

Dismissed stale review

@litetex litetex merged commit 2e862b4 into TeamNewPipe:dev Nov 3, 2021
This was referenced Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queue shuffle button highlighted when queue is not shuffled
6 participants