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

Simple playback-speed-controls improvements #7363

Conversation

litetex
Copy link
Member

@litetex litetex commented Nov 4, 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

"Why another PR for this issue?" → #4913 (comment)

Probably supersedes

In case we also want to change the order of the checkboxes...

No new layout is required, simply implement this:

    private void setDialogMode(final View view, final boolean landscapeMode) {
        final LinearLayout layoutAdditionalOptions =
                view.findViewById(R.id.additionalOptions);
        final List<CheckBox> additionalOptionsCheckboxes =
                Arrays.asList(
                        view.findViewById(R.id.unhookCheckbox),
                        view.findViewById(R.id.skipSilenceCheckbox)
                );

        layoutAdditionalOptions.setOrientation(
                landscapeMode
                        ? LinearLayout.HORIZONTAL
                        : LinearLayout.VERTICAL
        );

        for (final CheckBox checkBox : additionalOptionsCheckboxes) {
            final LinearLayout.LayoutParams layoutParams =
                    (LinearLayout.LayoutParams) checkBox.getLayoutParams();
            layoutParams.width =
                    landscapeMode
                            ? 0
                            : LinearLayout.LayoutParams.MATCH_PARENT;
            layoutParams.weight = landscapeMode ? 1 : 0;

            checkBox.setLayoutParams(layoutParams);
        }
    }

Before/After Screenshots/Screen Record

  • Before: grafik

  • After: grafik

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

* Removed dependency to @dimen/video_item_search_padding as it's unrelated
* Made the margins/paddings a bit smaller
* Put the checkboxes inside a layout
* Removed some useless attributes (maxLine)
to also save some margin/padding/etc
@litetex litetex mentioned this pull request Nov 4, 2021
3 tasks
@triallax triallax added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Nov 5, 2021
@MD77MD
Copy link

MD77MD commented Nov 6, 2021

if both check boxes where put at the same raw... to eliminate scrolling... which is the point of all the PRs

@litetex
Copy link
Member Author

litetex commented Nov 6, 2021

if both check boxes where put at the same row... to eliminate scrolling... which is the point of all the PRs

This does not work on some languages like e.g. German (where the "Unhook"-text is relatively large) or on small devices.
The scrollbars will always be there in some kind of situation.

Pi-Bouf
Pi-Bouf previously approved these changes Nov 7, 2021
@Hugov8 Hugov8 mentioned this pull request Nov 7, 2021
5 tasks
@Fs00
Copy link
Contributor

Fs00 commented Nov 7, 2021

A personal opinion on this: I would remove the "Playback Speed Controls" title rather than constrict it at the top, which doesn't look too good.
I think that the purpose of the dialog would be clear enough even without the title.

@litetex
Copy link
Member Author

litetex commented Nov 7, 2021

A personal opinion on this: I would remove the "Playback Speed Controls" title rather than constrict it at the top, which doesn't look too good. I think that the purpose of the dialog would be clear enough even without the title.

I like this idea.
It is really kind of obvious that these are the playback speed controls so I guess we can remove the title without harm :)

Update: Applied and committed (see issue-description for updated demo image)

@litetex litetex added the player Issues related to any player (main, popup and background) label Nov 7, 2021
@opusforlife2
Copy link
Collaborator

So has the need for scrolling been removed completely after removing the title?

@litetex
Copy link
Member Author

litetex commented Nov 8, 2021

So has the need for scrolling been removed completely after removing the title?

Yes. On "normal" devices like a Pixel 3a (5 inch display).

However small ones like a Nexus S (from 2011; Android 2.3.x with 4 inch display) will always have this kind of problem. It's "unfixable" there.

@MD77MD
Copy link

MD77MD commented Nov 10, 2021

i think it's a good enough now

@opusforlife2
Copy link
Collaborator

Alright. Seems like this is the best we can achieve here. 👍

Copy link
Member

@Redirion Redirion left a comment

Choose a reason for hiding this comment

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

lgtm

@Redirion Redirion merged commit 344fbff into TeamNewPipe:dev Nov 12, 2021
@litetex litetex deleted the playback-speed-ctrls-simple-landscape-improvements branch November 12, 2021 20:23
@goyalyashpal
Copy link
Contributor

goyalyashpal commented Dec 12, 2021

It is really kind of obvious that these are the playback speed controls so I guess we can remove the title without harm :)

bad bad, i dont agree :(

especially bad when getting the elder people onboard with "smartphones" and stuff

but ohkayyy....

@goyalyashpal goyalyashpal mentioned this pull request Jan 2, 2022
3 tasks
@litetex litetex mentioned this pull request Feb 21, 2022
5 tasks
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 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.

Speedmenu layout in landscape mode
8 participants