Skip to content

Conversation

@ktprograms
Copy link
Contributor

@ktprograms ktprograms commented Aug 22, 2021

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

  • Added menu_play_queue_item.xml as the resource to inflate the popup menu from.
  • Refactored the existing popup menu code in the background/popup player queue to use the menu xml.
  • Added the popup menu code to the main player queue.

Before/After Screenshots/Screen Record

Before After
Before.mp4
After.mp4

Fixes the following issue(s)

APK testing

https://github.com/TeamNewPipe/NewPipe/suites/3562943169/artifacts/85514049 (Link copied from the Checks tab)

Due diligence

@ktprograms
Copy link
Contributor Author

Just wondering, how does the Playback Speed Menu change it's color based on device dark mode?

@Stypox
Copy link
Member

Stypox commented Aug 24, 2021

Does the "Details" button work as expected, i.e. loading the details of another video without changing the playing stream?

@ktprograms
Copy link
Contributor Author

I think It does change the playing stream. How can I open the details without replacing the stream?

@Stypox
Copy link
Member

Stypox commented Aug 24, 2021

How can I open the details without replacing the stream?

I have no idea, probably it can't be done. I'd remove the "Details" action.

@ktprograms
Copy link
Contributor Author

Good idea. Come to think of it, Details doesn't make much sense when in the main player.

@ktprograms ktprograms force-pushed the queue-long-press-menu branch from 4df2650 to 9dd605f Compare August 24, 2021 11:22
@ktprograms
Copy link
Contributor Author

@Stypox I added a setVisible(false); on the Details menu item.

@ktprograms ktprograms force-pushed the queue-long-press-menu branch from 4526080 to ad99b54 Compare August 25, 2021 00:52
@ktprograms ktprograms force-pushed the queue-long-press-menu branch from ad99b54 to 1b05c40 Compare August 25, 2021 00:57
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.

Thanks :-)

@ktprograms
Copy link
Contributor Author

@Stypox all changes done in a4503eb

@ktprograms
Copy link
Contributor Author

@Stypox I just realised, if PlayQueueActivity checks if getPlayQueue() is null, shouldn't the popup menu in Player.java also check for null since they both are returned from the same property?

@AudricV AudricV added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Aug 26, 2021
@ktprograms ktprograms requested a review from Stypox August 28, 2021 12:46
@Stypox
Copy link
Member

Stypox commented Aug 30, 2021

@ktprograms you are right, maybe it's better to make sure the play queue is not null before checking .indexOf(item) != -1, in both places

@ktprograms
Copy link
Contributor Author

@Stypox sorry, I think I misread the code previously. The code in PlayQueueActivity checks if player is not null, not if getPlayQueue() is null:

if (player != null && player.getPlayQueue().indexOf(item) != -1) {

And since I haven't had any NPE yet, I think it can just be left as it is.

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.

I tested and it works well, thank you! :-)
Btw, you can resolve conversations when you are finished implementing them, it makes navigating the thread simpler ;-)

@Stypox Stypox merged commit 4b7c37e into TeamNewPipe:dev Aug 31, 2021
This was referenced Sep 5, 2021
@ktprograms ktprograms deleted the queue-long-press-menu branch October 10, 2021 11:46
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 player Issues related to any player (main, popup and background)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UX: consistent behavior for long press in play queue

3 participants