Improve playback parameter dialog's UI #7989

litetex · 2022-03-04T21:27:21Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Backstory

Noticed Change pitch by semitones #7565 lately
Took a look at changes inside there/PlaybackParameterDialog
This PR

Description of the changes in your PR

Refactored the code
- Removed a ton of duplicated stuff
- Use viewbinding
- Use Icepick instead of saving everything manually
Tuned UI
- UI fit's now - without a scroller - onto a 5in smartphone (Pixel 3a) in fullscreen mode
- Removed "Adjust pitch by musical semitones" checkbox and replaced it by a tab like component that can be expanded on demand
- Semitones now work with "hook"-mode
- Mark the currently selected steps/pitchmode inside the UI
Other
- Used more containers in the UI so that we don't have to do everything inside the code

Before/After Screenshots/Screen Record

Before:
After:

NPPlaybackDialogDemo1.mp4

Fixes the following issue(s)

Fixes Overhaul PlaybackParameterDialog #7960

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.

app/src/main/java/org/schabi/newpipe/util/DrawableResolver.kt

karyogamy · 2022-03-12T21:17:42Z

Nice work! By the way, would it be possible to collapse the 2 checkboxes into one line similar to Percent and Semitone?
If the text is too long to fit, maybe we can just call them Unhook and Skip Silence? Even with text and subtext it might save more space.

litetex · 2022-03-13T19:36:23Z

@karyogamy
I don't think that collapsing the checkboxes would be beneficial, because we don't need to save more space (already optimized for a standard 5" screen).
I might eventually do more UI optimizations in a future PR (e.g. representing the "Unhook" checkbox as a chain between speed and pitch) but the current one primarily aims at code improvements.

Stypox

Used more containers in the UI so that we don't have to do everything inside the code

This is not good for performance afaik, but if we don't migrate to ConstraintLayout (which has view groups) there is no alternative (so nothing to do)

app/src/main/java/org/schabi/newpipe/util/DrawableResolver.kt

app/src/main/java/org/schabi/newpipe/player/helper/PlayerSemitoneHelper.java

app/src/main/java/org/schabi/newpipe/player/helper/PlaybackParameterDialog.java

litetex · 2022-03-16T13:53:42Z

Thank you for the review!

Used more containers in the UI so that we don't have to do everything inside the code

This is not good for performance afaik, but if we don't migrate to ConstraintLayout (which has view groups) there is no alternative (so nothing to do)

It aren't so many new "containers" (1-3)
I think that ConstraintLayout is overrated in the first place - at least for this view:

So I quickly googled why Constraintlayout is better than Relativelayout and found these sources (from https://stackoverflow.com/questions/59628341/constraintlayout-vs-relativelayout-performance-2020):

https://developer.android.com/topic/performance/rendering/optimizing-view-hierarchies.html#cheaper

In the N release of Android, the ConstraintLayout class provides similar functionality to RelativeLayout, but at a significantly lower cost.

However it doesn't cite any sources why it performs better...
https://android-developers.googleblog.com/2017/08/understanding-performance-benefits-of.html
This article is in my opinion not representable:
- It simply removes all relative layouts when using ConstraintLayout. They don't use any view groups.
- According to their graph the performance in the "Measurement and Layout phase" increased from 0.65ms to 0.5ms (which is a performance increase in 25-30% not 40% as they stated)

TL;DR
I will not rewrite the complete layout now to ConstraintLayout because it's then 0.15ms faster. Especially regarding that the dialog/layout is rarely used in the first place.
We can create an issue that does this in the follow up, but for now I think that the current state should be enough.

Stypox · 2022-03-16T14:28:43Z

I will not rewrite the complete layout now to ConstraintLayout because it's then 0.15ms faster. Especially regarding that the dialog/layout is rarely used in the first place.
We can create an issue that does this in the follow up, but for now I think that the current state should be enough.

Yes, I do agree it shouldn't be done here, and I do agree there is no big need to spend time on it. It's just something worth noting in my opinion, since ConstraintLayouts are better not only in performance but also they are also simpler to code, cause less errors and are more powerful.

SameenAhnaf · 2022-04-03T11:36:35Z

Do we really need to click twice just to save space? A checkbox could be placed here instead?

litetex · 2022-04-09T20:01:04Z

@SameenAhnaf

How often do you change between semitones and percent?Not that often or?
If the "pitch" and "Semtiones" are displayed on a small screen and have long localizations they won't fit

Redirion · 2022-04-14T10:10:33Z

I resolved the conflict directly here on github and it resulted in that not really helpful commit message "Merge branch 'dev'". Can you clean that up please litetex? This should be ready to merge.

sonarqubecloud · 2022-04-14T10:16:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

* Removed/Renamed methods * Use ``IcePick`` * Better structuring * Keep skipSilence when rotating the device (PlayQueueActivity only)

* De-Duplicated some fields * Use a container for the pitch controls * Name pitch related elements correctly

* Add support for semitones * Fixed some minor bugs * Improved some methods

Using an expandable Tab-like component instead of a combobox

From the PR review

Nicer import 😉

Stypox

I tested and it works well. Thank you!

…changes

goyalyashpal · 2023-01-22T19:42:20Z

Mark the currently selected steps/pitchmode inside the UI

hey! is this reverted in v0.24.1 ?? I was unable to see the rectangular dashed border around currently selected playback speed selector/speed adjustment step size buttons in that version, while it's there in v0.23.3

Trail/please ignore

blame on dialog_playback_parameter.xml
<-- files in !9145
<-- linked in #8962
<-- gh issue 🔍 : adjustment step size
<-- search term found in !7728
<-- Release 0.23.0 #8230
<-- gh issue 🔍 : speed selector

Failed gh pulls 🔍 :

opusforlife2 · 2023-01-22T20:45:48Z

I can see it just fine. Maybe your OS colours are different?

goyalyashpal · 2023-01-26T16:53:38Z

I can see it just fine. Maybe your OS colours are different?

i just rechecked, and yeah, it was present there.... though it was very faint due to low resolution and small screen size of the device i saw that on.

litetex added feature request Issue is related to a feature in the app codequality Improvements to the codebase to improve the code quality labels Mar 4, 2022

litetex marked this pull request as ready for review March 4, 2022 21:29

TacoTheDank reviewed Mar 4, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/util/DrawableResolver.kt Outdated Show resolved Hide resolved

TacoTheDank reviewed Mar 4, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/util/DrawableResolver.kt Outdated Show resolved Hide resolved

TacoTheDank reviewed Mar 6, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/util/DrawableResolver.kt Outdated Show resolved Hide resolved

Stypox requested changes Mar 16, 2022

View reviewed changes

litetex mentioned this pull request Apr 9, 2022

Limit speed at live edge #8134

Closed

5 tasks

litetex added 14 commits April 16, 2022 21:21

Major refactoring of PlaybackParameterDialog

652d501

* Removed/Renamed methods * Use ``IcePick`` * Better structuring * Keep skipSilence when rotating the device (PlayQueueActivity only)

Use viewbinding

4cdf6ed

Remove redundant attributes

6e0c380

Remove invalid parameters

e4f2028

Rework dialog

a4c083e

* De-Duplicated some fields * Use a container for the pitch controls * Name pitch related elements correctly

Shrunk dialog a bit

7d42e50

Fine tuned dialog (no scrollers in fullscreen on 5in phone)

dae5aa3

Reworked/Implemented PlaybackParameterDialog functionallity

762cdc8

* Add support for semitones * Fixed some minor bugs * Improved some methods

Fine tuned dialog

321cf8b

Code improvements regarding stepSize

621b38c

Reworked switching to semitones

4b06536

Using an expandable Tab-like component instead of a combobox

Added some doc and abstracted more methods

2060288

Removed unused strings

1b8c517

Use better Kotlin syntax

44dada9

From the PR review

litetex added 4 commits April 16, 2022 21:21

Update DrawableResolver.kt

b9190ed

Nicer import 😉

Refactored code

0f551ba

Merged DrawableResolver into ThemeHelper

1dc1463

Fix merge conflicts

a311519

litetex force-pushed the refactor-playback-parameter-dialog branch from 0c1b282 to a311519 Compare April 16, 2022 19:24

Stypox approved these changes Apr 30, 2022

View reviewed changes

Stypox merged commit 7646c68 into TeamNewPipe:dev Apr 30, 2022

Stypox changed the title ~~Rewrote PlaybackParameterDialog~~ Improve playback parameter dialog's UI Apr 30, 2022

KosOrKosm added a commit to KosOrKosm/NewPipe that referenced this pull request Apr 30, 2022

PlaybackParameterDialog: merge live tempo limit with TeamNewPipe#7989 …

ed2641e

…changes

litetex deleted the refactor-playback-parameter-dialog branch May 1, 2022 12:34

Stypox mentioned this pull request Jun 24, 2022

Release v0.23.1 (please TEST!) #8548

Closed

3 tasks

Fs00 mentioned this pull request Jul 26, 2022

Test issue Fs00/gh4a#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve playback parameter dialog's UI #7989

Improve playback parameter dialog's UI #7989

litetex commented Mar 4, 2022 •

edited

Loading

karyogamy commented Mar 12, 2022

litetex commented Mar 13, 2022

Stypox left a comment •

edited

Loading

litetex commented Mar 16, 2022 •

edited

Loading

Stypox commented Mar 16, 2022

SameenAhnaf commented Apr 3, 2022 •

edited by github-actions bot

Loading

litetex commented Apr 9, 2022

Redirion commented Apr 14, 2022

sonarqubecloud bot commented Apr 14, 2022

Stypox left a comment

goyalyashpal commented Jan 22, 2023 •

edited

Loading

opusforlife2 commented Jan 22, 2023

goyalyashpal commented Jan 26, 2023

Improve playback parameter dialog's UI #7989

Improve playback parameter dialog's UI #7989

Conversation

litetex commented Mar 4, 2022 • edited Loading

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Fixes the following issue(s)

APK testing

Due diligence

karyogamy commented Mar 12, 2022

litetex commented Mar 13, 2022

Stypox left a comment • edited Loading

Choose a reason for hiding this comment

litetex commented Mar 16, 2022 • edited Loading

Stypox commented Mar 16, 2022

SameenAhnaf commented Apr 3, 2022 • edited by github-actions bot Loading

litetex commented Apr 9, 2022

Redirion commented Apr 14, 2022

sonarqubecloud bot commented Apr 14, 2022

Stypox left a comment

Choose a reason for hiding this comment

goyalyashpal commented Jan 22, 2023 • edited Loading

opusforlife2 commented Jan 22, 2023

goyalyashpal commented Jan 26, 2023

litetex commented Mar 4, 2022 •

edited

Loading

Stypox left a comment •

edited

Loading

litetex commented Mar 16, 2022 •

edited

Loading

SameenAhnaf commented Apr 3, 2022 •

edited by github-actions bot

Loading

goyalyashpal commented Jan 22, 2023 •

edited

Loading