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

implement ability to ignore back-queue for main view #7273

Closed
wants to merge 6 commits into from

Conversation

newhinton
Copy link

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

  • Add Setting (under history) to allow the main view to ignore the backstack of videos. Instead of going back to the last video, the player minimizes. This is NOT the default behavior, when no changes are made, the queue is still used.

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

@newhinton
Copy link
Author

This should probably beeing closed in favor of #6895

@MD77MD
Copy link

MD77MD commented Oct 24, 2021

This should probably beeing closed in favor of #6895

no i think this good idea... i would like to go back through the queue using the back button, however by implementing your idea, i gain the ability to minimize the player after queue is completed which is the behavior i prefer at this point.

so yes I think both PRs should be implemented as everyone of them has its unique usages

@avently
Copy link
Contributor

avently commented Nov 1, 2021

@newhinton

7d96c05

Agree

@triallax triallax added feature request Issue is related to a feature in the app queue Issue is related to queueing labels Nov 3, 2021
@Stypox
Copy link
Member

Stypox commented Nov 13, 2021

This PR and #6895 should be merged into a single PR, since we don't want to create two different settings options for the same thing. @avently could you port the back-behavior from this PR into #6895?

@newhinton
Copy link
Author

The functionality in this PR is already available in #6895. It is basically a duplicate.

@newhinton newhinton closed this Nov 13, 2021
@avently
Copy link
Contributor

avently commented Nov 14, 2021

@Stypox yep, this functionality is already available in my PR, nothing to add

@newhinton newhinton reopened this Aug 13, 2022
@opusforlife2
Copy link
Collaborator

Can this be made default for Android TV?

@newhinton
Copy link
Author

Can this be made default for Android TV?

I guess DeviceUtils.isTv(activity) can be used to override the default value, for the videofragment that is simple. For the settings, i am not sure, i will have to look into the settings more precise, because currently the defaults are set by the layoutfile itself.

@newhinton
Copy link
Author

I made it default for tv's. However, displaying the correct state in the settings is really not that great, but i dont see any other way to do it.

# Conflicts:
#	app/src/main/java/org/schabi/newpipe/settings/HistorySettingsFragment.java
#	app/src/main/res/values/strings.xml
@newhinton
Copy link
Author

@opusforlife2 @Stypox Any news on this? It's a rather simple PR and would add a highly requested feature.

@opusforlife2
Copy link
Collaborator

@newhinton I like the idea. We need for someone to have the time to review this. Meanwhile, please fix any merge conflicts.

@newhinton
Copy link
Author

I have resolved the conflict.

@bazipn
Copy link

bazipn commented Dec 4, 2022

This would be really useful for navigating TVs, do you have a debug apk?

@newhinton
Copy link
Author

app-debug.zip

@bazipn I build one.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@FriendlyNeighborhoodShane

Is this still active or not?

This functionality is important to me on both my TV and phone UI, as well as many other people evident from looking at the issues about this.

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.

Sorry for not reviewing earlier, I did start a review but never sent it 🤦
Closing as this is tricky to get right, as the comments I left highlight. We'll decide on a more user-friendly UI/UX for the player pages during the rewrite, and we'll definitely ditch the current strange behavior in favour of what most people would expect. We've not been able to do that so far, also because of how bad the code is.

@@ -782,6 +789,11 @@ public boolean onBackPressed() {
return true;
}

// when queue should be ignored, directly skip checks and let MainActivity handle everything
if (ignoreQueue) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

I think you should also restoreDefaultOrientation()

Comment on lines +294 to +297
ignoreQueue = prefs.getBoolean(
getString(R.string.enable_ignore_backstack_key),
DeviceUtils.isTv(activity)
);
Copy link
Member

Choose a reason for hiding this comment

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

What if the user changes preference while the fragment is open? You need to update this variable in the shared preference change listener.

Comment on lines +164 to +166

private boolean ignoreQueue;

Copy link
Member

Choose a reason for hiding this comment

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

Rename the variable and remove spaces

Suggested change
private boolean ignoreQueue;
private boolean ignoreBackstack;

@Stypox Stypox closed this Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app queue Issue is related to queueing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An option to disable the video history/queue control with the "back" button
8 participants