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

Directplay on Background #2198

Merged
merged 3 commits into from
Mar 12, 2019
Merged

Directplay on Background #2198

merged 3 commits into from
Mar 12, 2019

Conversation

Redirion
Copy link
Member

@Redirion Redirion commented Mar 8, 2019

closes #2187

See the issue description and discussion. (with pictures!)

Regarding difference between "Play on Background" (commit and branch name) vs. "Play in Background" (strings.xml): from developer perspective playback runs on background player while from user perspective playback runs in background. ;)

Tested on Android 7.1.2.

Copy link
Member

@theScrabi theScrabi left a comment

Choose a reason for hiding this comment

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

Code is ok, please check QA.

@Redirion
Copy link
Member Author

Redirion commented Mar 9, 2019

Code is ok, please check QA.

could you please link or explain what you mean with QA? My change is extremely small and straightforward and tested.

@theScrabi
Copy link
Member

Quality Asurance. We need to check if everything works as intended.

@Redirion
Copy link
Member Author

Ah. I thought you wanted me to check some sort of QA guideline for NewPipe. Thanks for the clarification.

@TobiGr
Copy link
Contributor

TobiGr commented Mar 12, 2019

@theScrabi IMO, we have too many longpress menus. They have a bad UI and UX , becuase

  1. You can only interact with one video/stream item at once.
  2. These overlay menus block every interaction with other elements.
  3. They don't look nice

I think we should replace the menus with some buttons/icons in the app bar. something like this (rather the second draft):
Bild3
In other words, make it possible to select multiple videos from a list and keep every item directly accessible.

Btw. Should I open a new ticket for further discussion on this?

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

My comment above has nothing to do with the original purpose of this PR, As @theScrabi already said, code looks good and I didn't find any bugs during a short test.

@TobiGr TobiGr merged commit f9e771f into TeamNewPipe:dev Mar 12, 2019
@theScrabi
Copy link
Member

@TobiGr this proposal looks god and fits better into the material design then the long press menus we have right now :) I think its a good idea.

@Redirion Redirion deleted the directOnBackground branch March 14, 2019 10:25
This was referenced Apr 28, 2019
@Stypox
Copy link
Member

Stypox commented May 30, 2019

Oh, I just came across this PR... @Redirion I saw you added the direct_on_background (= Play directly in background) string to the strings.xml... Does it have the same meaning as start_here_on_background (= Start playing in the background)? I removed it in #2368 because I thought it was a duplicate.

@Redirion
Copy link
Member Author

I thought "start here on background" means that this video would be played directly and all videos below would be queued. The option added by me would just play the video without queing others. @Stypox

@Stypox
Copy link
Member

Stypox commented May 30, 2019

It seems like that's not the meaning... start_here_on_background clears the queue and then adds the stream, just as your option does.

That name is ambiguous

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.

Extra option in addition of Enqueue when Backgrounded
4 participants