Adding 1440p and 2160p resolutions to the Default resolutions menus

[NixedSec]

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 1440p and 2160p (2K and 4K) to the Default resolutions lists as requested in Ability to select default 2K/4K resolution #2941.

Before/After Screenshots/Screen Record

Fixes the following issue(s)

• Fixes Ability to select default 2K/4K resolution #2941

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

• I read the contribution guidelines.

[NixedSec]

Sorry for the extra commits, I added 1440p60 and 2160p60, then made a change as so the resolution lists update upon the switch interaction for high resolutions. I just synced the main repo to my fork, didn't realise that would create a new commit, im new to this.

[NixedSec]

Just edited code based on the feedback of SonarCloud.

[Stypox]

Thank you! The code can be improved in various ways

[NixedSec]

Thank you very much for the feedback. The code is looking much more concise.

[Stypox]

Thank you. Actually, I noticed a problem: the "limit mobile data" and "default resolution" menus actually use a different string for when no resolution is selected, i.e. "no limit" and "best resolution". Your method was putting "best resolution" everywhere. Also, I didn't like much the fact that resolutions were copied all over the place, so I made them dynamically generated and called it a day. I made these changes in ba0ab8f, what do you think? :-)

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[NixedSec]

Ah, I completely missed that, thanks for catching it. I saw the replicated sets of resolutions and didn't want to multiply them for different ones, then missed that entirely. Thank you for your patience. It looks good, thank you.

[Stypox]

@litetex could you review, too?

[litetex]

I see some minor problems:

• I'm not sure if we need that feature in the first place... No many people have phones (display & GPU) or internet connections that allow displaying quality higher than 1080p60 but when looking in the future it may be useful to have such an option.
• Please don't use the dev branch for doing a PR, create a new branch with a meaningful name next time
• There are a few formatting problems
• I also think that the code can be simplified on a few places

I would change the following:

diff --git a/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java b/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java
index b9be78207..39fe6c71b 100644
--- a/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java
+++ b/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java
@@ -16,11 +16,9 @@ import com.google.android.material.snackbar.Snackbar;
 import org.schabi.newpipe.R;
 import org.schabi.newpipe.util.PermissionHelper;
 
-import java.util.ArrayList;
-import java.util.Collection;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.stream.Collectors;
+import java.util.function.IntFunction;
 import java.util.stream.Stream;
 
 public class VideoAudioSettingsFragment extends BasePreferenceFragment {
@@ -29,26 +27,23 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
             List.of("1080p60", "1080p", "720p60", "720p", "480p", "360p", "240p", "144p");
     private static final List<String> HIGHER_RESOLUTIONS =
             List.of("2160p60", "2160p", "1440p60", "1440p");
-    private static final List<String> ALL_RESOLUTIONS =
-            Stream.of(HIGHER_RESOLUTIONS, NORMAL_RESOLUTIONS)
-                    .flatMap(Collection::stream)
-                    .collect(Collectors.toList());
 
     private SharedPreferences.OnSharedPreferenceChangeListener listener;
+
     private ListPreference defaultResolution;
     private ListPreference defaultPopupResolution;
     private ListPreference limitMobileDataUsage;
 
-
     @Override
     public void onCreatePreferences(final Bundle savedInstanceState, final String rootKey) {
         addPreferencesFromResourceRegistry();
 
         updateSeekOptions();
+
         defaultResolution = findPreference(getString(R.string.default_resolution_key));
         defaultPopupResolution = findPreference(getString(R.string.default_popup_resolution_key));
         limitMobileDataUsage = findPreference(getString(R.string.limit_mobile_data_usage_key));
-        updateResolutions();
+        updateAvailableResolutions();
 
         listener = (sharedPreferences, s) -> {
 
@@ -72,8 +67,8 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
             } else if (s.equals(getString(R.string.use_inexact_seek_key))) {
                 updateSeekOptions();
             } else if (s.equals(getString(R.string.show_higher_resolutions_key))) {
-                updateResolutions();
-        }
+                updateAvailableResolutions();
+            }
         };
     }
 
@@ -128,57 +123,55 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
             toast.show();
         }
     }
+    
+    private void updateAvailableResolutions() {
+        final boolean showHigherResolutions = getPreferenceManager().getSharedPreferences()
+                .getBoolean(getString(R.string.show_higher_resolutions_key), false);
 
+        updateAvailableResolutions(defaultResolution, showHigherResolutions,
+                R.string.best_resolution_key,
+                R.string.best_resolution);
 
-    /**
-     * Update resolution options. Defaults to 1080p60 and removes higher options when the "Show
-     * higher resolutions" switch is disabled.
-     */
-    private void updateResolutions() {
-        final boolean showHigherResolutions = getPreferenceManager().getSharedPreferences()
-            .getBoolean(getString(R.string.show_higher_resolutions_key), false);
+        updateAvailableResolutions(defaultPopupResolution, showHigherResolutions,
+                R.string.best_resolution_key,
+                R.string.best_resolution);
 
-        updateResolutions(defaultResolution, showHigherResolutions,
-                R.string.best_resolution_key, R.string.best_resolution);
-        updateResolutions(defaultPopupResolution, showHigherResolutions,
-                R.string.best_resolution_key, R.string.best_resolution);
-        updateResolutions(limitMobileDataUsage, showHigherResolutions,
+        updateAvailableResolutions(limitMobileDataUsage, showHigherResolutions,
                 R.string.limit_mobile_data_usage_none_key,
                 R.string.limit_data_usage_none_description);
     }
 
-    private void updateResolutions(final ListPreference preference,
-                                   final boolean showHigherResolutions,
-                                   @StringRes final int noResolutionValue,
-                                   @StringRes final int noResolutionDescription) {
-        final List<String> resolutionValues = new ArrayList<>();
-        final List<String> resolutionDescriptions = new ArrayList<>();
-
-        // add in the first place the "no resolution chosen" option (i.e. "best" or "no limit")
-        resolutionValues.add(getString(noResolutionValue));
-        resolutionDescriptions.add(getString(noResolutionDescription));
-
-        if (showHigherResolutions) {
-            // set the whole arrays
-            resolutionValues.addAll(ALL_RESOLUTIONS);
-            resolutionDescriptions.addAll(ALL_RESOLUTIONS);
-        } else {
-            // reset the current value to the biggest non-higher resolution if needed
-            if (HIGHER_RESOLUTIONS.contains(preference.getValue())) {
-                preference.setValue(NORMAL_RESOLUTIONS.get(0));
-            }
-
-            // only keep non-higher resolutions
-            resolutionValues.addAll(NORMAL_RESOLUTIONS);
-            resolutionDescriptions.addAll(NORMAL_RESOLUTIONS);
+    /**
+     * Updates the available resolution options for the specified preference.
+     *
+     * @param preference              the preference
+     * @param showHigherResolutions   <code>true</code> to only show {@link #NORMAL_RESOLUTIONS}
+     * @param noResolutionValue       the translation key for the first resolution value
+     * @param noResolutionDescription the translation key for the first resolution description
+     */
+    private void updateAvailableResolutions(final ListPreference preference,
+                                            final boolean showHigherResolutions,
+                                            @StringRes final int noResolutionValue,
+                                            @StringRes final int noResolutionDescription) {
+        final IntFunction<String[]> entryBuilder = firstResolutionKey ->
+                Stream.concat(
+                        Stream.of(getString(firstResolutionKey)),
+                        Stream.concat(
+                                showHigherResolutions
+                                        ? HIGHER_RESOLUTIONS.stream()
+                                        : Stream.empty(),
+                                NORMAL_RESOLUTIONS.stream()
+                        )).toArray(String[]::new);
+
+        preference.setEntryValues(entryBuilder.apply(noResolutionValue));
+        preference.setEntries(entryBuilder.apply(noResolutionDescription));
+
+        // reset the current value to the biggest non-higher resolution if needed
+        if (!showHigherResolutions && HIGHER_RESOLUTIONS.contains(preference.getValue())) {
+            preference.setValue(NORMAL_RESOLUTIONS.get(0));
         }
-
-        // finally set the computed arrays to the preference
-        preference.setEntryValues(resolutionValues.toArray(new String[0]));
-        preference.setEntries(resolutionDescriptions.toArray(new String[0]));
     }
 
-
     @Override
     public void onResume() {
         super.onResume();

@Stypox
However it might be better to do a larger refactoring where we could also add something like "automatic resolution" in the future:

Draft code changes

diff --git a/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java b/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java
index b9be78207..04a11312e 100644
--- a/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java
+++ b/app/src/main/java/org/schabi/newpipe/settings/VideoAudioSettingsFragment.java
@@ -16,8 +16,6 @@ import com.google.android.material.snackbar.Snackbar;
 import org.schabi.newpipe.R;
 import org.schabi.newpipe.util.PermissionHelper;
 
-import java.util.ArrayList;
-import java.util.Collection;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -29,26 +27,23 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
             List.of("1080p60", "1080p", "720p60", "720p", "480p", "360p", "240p", "144p");
     private static final List<String> HIGHER_RESOLUTIONS =
             List.of("2160p60", "2160p", "1440p60", "1440p");
-    private static final List<String> ALL_RESOLUTIONS =
-            Stream.of(HIGHER_RESOLUTIONS, NORMAL_RESOLUTIONS)
-                    .flatMap(Collection::stream)
-                    .collect(Collectors.toList());
 
     private SharedPreferences.OnSharedPreferenceChangeListener listener;
+
     private ListPreference defaultResolution;
     private ListPreference defaultPopupResolution;
     private ListPreference limitMobileDataUsage;
 
-
     @Override
     public void onCreatePreferences(final Bundle savedInstanceState, final String rootKey) {
         addPreferencesFromResourceRegistry();
 
         updateSeekOptions();
+
         defaultResolution = findPreference(getString(R.string.default_resolution_key));
         defaultPopupResolution = findPreference(getString(R.string.default_popup_resolution_key));
         limitMobileDataUsage = findPreference(getString(R.string.limit_mobile_data_usage_key));
-        updateResolutions();
+        updateAvailableResolutions();
 
         listener = (sharedPreferences, s) -> {
 
@@ -63,7 +58,7 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
                         && !Settings.canDrawOverlays(getContext())) {
 
                     Snackbar.make(getListView(), R.string.permission_display_over_apps,
-                            Snackbar.LENGTH_INDEFINITE)
+                                    Snackbar.LENGTH_INDEFINITE)
                             .setAction(R.string.settings, view ->
                                     PermissionHelper.checkSystemAlertWindowPermission(getContext()))
                             .show();
@@ -72,8 +67,8 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
             } else if (s.equals(getString(R.string.use_inexact_seek_key))) {
                 updateSeekOptions();
             } else if (s.equals(getString(R.string.show_higher_resolutions_key))) {
-                updateResolutions();
-        }
+                updateAvailableResolutions();
+            }
         };
     }
 
@@ -131,53 +126,80 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
 
 
     /**
-     * Update resolution options. Defaults to 1080p60 and removes higher options when the "Show
+     * Updates all available resolution options. Defaults to 1080p60 and removes higher options
+     * when the "Show
      * higher resolutions" switch is disabled.
      */
-    private void updateResolutions() {
+    private void updateAvailableResolutions() {
         final boolean showHigherResolutions = getPreferenceManager().getSharedPreferences()
-            .getBoolean(getString(R.string.show_higher_resolutions_key), false);
-
-        updateResolutions(defaultResolution, showHigherResolutions,
-                R.string.best_resolution_key, R.string.best_resolution);
-        updateResolutions(defaultPopupResolution, showHigherResolutions,
-                R.string.best_resolution_key, R.string.best_resolution);
-        updateResolutions(limitMobileDataUsage, showHigherResolutions,
-                R.string.limit_mobile_data_usage_none_key,
-                R.string.limit_data_usage_none_description);
+                .getBoolean(getString(R.string.show_higher_resolutions_key), false);
+
+        updateAvailableResolutions(defaultResolution, showHigherResolutions,
+                Stream.concat(
+                        buildTranslatedResolutionEntry(
+                                R.string.best_resolution_key,
+                                R.string.best_resolution),
+                        buildFixedResolutionEntries(showHigherResolutions)));
+
+        updateAvailableResolutions(defaultPopupResolution, showHigherResolutions,
+                Stream.concat(
+                        buildTranslatedResolutionEntry(
+                                R.string.best_resolution_key,
+                                R.string.best_resolution),
+                        buildFixedResolutionEntries(showHigherResolutions)));
+
+        updateAvailableResolutions(limitMobileDataUsage, showHigherResolutions,
+                Stream.concat(
+                        buildTranslatedResolutionEntry(
+                                R.string.limit_mobile_data_usage_none_key,
+                                R.string.limit_data_usage_none_description),
+                        buildFixedResolutionEntries(showHigherResolutions)));
     }
 
-    private void updateResolutions(final ListPreference preference,
-                                   final boolean showHigherResolutions,
-                                   @StringRes final int noResolutionValue,
-                                   @StringRes final int noResolutionDescription) {
-        final List<String> resolutionValues = new ArrayList<>();
-        final List<String> resolutionDescriptions = new ArrayList<>();
-
-        // add in the first place the "no resolution chosen" option (i.e. "best" or "no limit")
-        resolutionValues.add(getString(noResolutionValue));
-        resolutionDescriptions.add(getString(noResolutionDescription));
-
-        if (showHigherResolutions) {
-            // set the whole arrays
-            resolutionValues.addAll(ALL_RESOLUTIONS);
-            resolutionDescriptions.addAll(ALL_RESOLUTIONS);
-        } else {
-            // reset the current value to the biggest non-higher resolution if needed
-            if (HIGHER_RESOLUTIONS.contains(preference.getValue())) {
-                preference.setValue(NORMAL_RESOLUTIONS.get(0));
-            }
-
-            // only keep non-higher resolutions
-            resolutionValues.addAll(NORMAL_RESOLUTIONS);
-            resolutionDescriptions.addAll(NORMAL_RESOLUTIONS);
-        }
+    private Stream<ResolutionEntry> buildTranslatedResolutionEntry(
+            @StringRes final int noResolutionValue,
+            @StringRes final int noResolutionDescription) {
+        return Stream.of(new ResolutionEntry(
+                getString(noResolutionValue),
+                getString(noResolutionDescription)));
+    }
 
-        // finally set the computed arrays to the preference
-        preference.setEntryValues(resolutionValues.toArray(new String[0]));
-        preference.setEntries(resolutionDescriptions.toArray(new String[0]));
+    private Stream<ResolutionEntry> buildFixedResolutionEntries(
+            final boolean showHigherResolutions) {
+        return Stream.concat(
+                        showHigherResolutions
+                                ? HIGHER_RESOLUTIONS.stream()
+                                : Stream.empty(),
+                        NORMAL_RESOLUTIONS.stream())
+                .map(ResolutionEntry::new);
     }
 
+    /**
+     * Updates the available resolution options for the specified preference.
+     * <p>
+     * If the currently set resolution is out of range it's set to a correct value.
+     * </p>
+     *
+     * @param preference            the preference
+     * @param showHigherResolutions <code>false</code> to only show {@link #NORMAL_RESOLUTIONS}
+     * @param resEntries            The {@link ResolutionEntry ResolutionEntries} to add
+     */
+    private void updateAvailableResolutions(final ListPreference preference,
+                                            final boolean showHigherResolutions,
+                                            final Stream<ResolutionEntry> resEntries) {
+        final List<ResolutionEntry> resEntryList = resEntries.collect(Collectors.toList());
+        preference.setEntryValues(resEntryList.stream()
+                .map(ResolutionEntry::getValue)
+                .toArray(String[]::new));
+        preference.setEntries(resEntryList.stream()
+                .map(ResolutionEntry::getDescription)
+                .toArray(String[]::new));
+
+        // reset the current value to the biggest non-higher resolution if needed
+        if (!showHigherResolutions && HIGHER_RESOLUTIONS.contains(preference.getValue())) {
+            preference.setValue(NORMAL_RESOLUTIONS.get(0));
+        }
+    }
 
     @Override
     public void onResume() {
@@ -192,4 +214,32 @@ public class VideoAudioSettingsFragment extends BasePreferenceFragment {
         getPreferenceManager().getSharedPreferences()
                 .unregisterOnSharedPreferenceChangeListener(listener);
     }
+
+    static class ResolutionEntry {
+        /**
+         * The value will be persisted with the preference framework.
+         */
+        private final String value;
+        /**
+         * The description will be displayed on the UI.
+         */
+        private final String description;
+
+        ResolutionEntry(final String valueAndDescription) {
+            this(valueAndDescription, valueAndDescription);
+        }
+
+        ResolutionEntry(final String value, final String description) {
+            this.value = value;
+            this.description = description;
+        }
+
+        public String getValue() {
+            return value;
+        }
+
+        public String getDescription() {
+            return description;
+        }
+    }
 }

[Stypox]

@NixedSec do you wish to continue working on this PR?

[SameenAhnaf]

Closing due to no reply from author