Make UI behavior for playback information display more consistent #8180

Trust04zh · 2022-04-12T12:54:53Z

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

~~Make Resume playback the dependency of Positions in lists.~~ This change has been put on hold, see this comment.
Regain the function to control if playback information for videos in lists should display for option Positions in lists.
Use Resume playback to control if playback information in VideoDetailFragment should display (positionView and detailPositionView, for UI), and remove this function for Positions in lists.
Animate without delay in case that positionView and detailPositionView should disappear. (this fixes Instant inconsistent display for playback information in VideoDetailFragment #8176)

Before/After Screenshots/Screen Record

The first half of the two videos explain how the option Positions in lists should control the display of playback information in the list (change 2), and the second half explain how to eliminate the instant inconsistency in playback information display (change 4).

Before:

before.mp4

After:

after.mp4

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

I read the contribution guidelines.

litetex

Thank you for the PR.

Had a quick review:

Please add some documentation/comments that explain (e.g. on complex if blocks) why stuff is happening
Some code snippets look like they are copy-pasted. Consider using abstraction.
Please don't use settings-logic inside a UI class (#Layering)

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

app/src/main/java/org/schabi/newpipe/views/AnimatedProgressBar.java

Trust04zh · 2022-04-15T17:50:40Z

Thank @litetex for commenting! This really helps me on improvement. I add some response here.

Please add some documentation/comments that explain (e.g. on complex if blocks) why stuff is happening

Gonna do that.

Some code snippets look like they are copy-pasted. Consider using abstraction.

yes, in class LocalPlaylistStreamItemHolder, LocalStatisticStreamItemHolder, StreamMiniInfoItemHolder there are copy-pasted code, while you can see that the previous code that surrounding my changes (some are with high similarity) didn't use abstraction, so I just keep the style as previous code. I can also try to do abstraction for my changes and previous code, please reply if you think this would be better.

Please don't use settings-logic inside a UI class (#Layering)

I will find an appropriate place for it (maybe a helper class).

Trust04zh · 2022-04-18T17:39:56Z

The updates

add a util class dependentPreferenceHelper to retrive option value (enabled or not) with complex dependencies.
add just one comment, I think the code is clear to read now.
use primitive boolean instead of Boolean for added methods.
removed the debug log info since it can not be reached.

sonarcloud · 2022-04-18T17:43:13Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
10.3% Duplication

Trust04zh · 2022-06-25T15:39:35Z

Just a remind that this pr has been shelved for some time, it may be better if someone would like to review this if the issues related to this pr should be solved and my solution being considered to be acceptable, or I may forget about my codes XD. Anyway I hope my pr not to bother you, so you can also just take this pr as a reference (if helpful) when solving related issues in the future.

Stypox

Thank you for the PR! I agree with litetex, the code in the various stream item holders should be deduplicated. Maybe you could add a common method that takes as parameter the itemProgressView, and the data needed to take decisions and update the UI.
Since this and other requested changes require knowledge about the codebase, if you need help or want me to do this, just ask ;-)

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

app/src/main/java/org/schabi/newpipe/util/DependentPreferenceHelper.java

app/src/main/java/org/schabi/newpipe/local/holder/LocalStatisticStreamItemHolder.java

opusforlife2 · 2022-08-23T23:25:37Z

@Trust04zh Hey, will you be incorporating the review at some point?

Trust04zh · 2022-08-25T13:53:10Z

@opusforlife2 yes, I will check the review and update the code in the recent two or three days.

sonarcloud · 2022-08-29T03:10:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

Trust04zh · 2022-08-29T03:35:47Z

Thank you for the reviews @Stypox! I agree with them and updated the code accordingly. There is no functinoal change in the new commits.

As some code changes are removed in new commits, the duplication in item holder classes reduces but still exists.

Maybe you could add a common method that takes as parameter the itemProgressView, and the data needed to take decisions and update the UI.

I'm not sure about how to make such code abstraction, since the three classes are not simply extended from the same superclass and the updateState methods have different signature. I would be appreciate if you could do this.

Stypox

Sorry for the long time passed... I hope you still remember something ;-)

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

app/src/main/java/org/schabi/newpipe/info_list/holder/StreamMiniInfoItemHolder.java

app/src/main/java/org/schabi/newpipe/local/holder/LocalPlaylistStreamItemHolder.java

app/src/main/java/org/schabi/newpipe/local/holder/LocalStatisticStreamItemHolder.java

Trust04zh · 2022-12-04T15:09:16Z

@Stypox Thanks for your reviewing and sorry that I just found the email reminding me for your reply, the code is updated.

sonarcloud · 2022-12-04T15:21:09Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
8.6% Duplication

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

app/src/main/java/org/schabi/newpipe/info_list/holder/StreamMiniInfoItemHolder.java

app/src/main/java/org/schabi/newpipe/local/holder/LocalPlaylistStreamItemHolder.java

app/src/main/java/org/schabi/newpipe/local/holder/LocalStatisticStreamItemHolder.java

app/src/main/java/org/schabi/newpipe/views/AnimatedProgressBar.java

Trust04zh · 2023-01-20T15:42:54Z

@Stypox Thanks for your reviewing! The code mentioned are updated.

Stypox · 2023-01-20T17:16:10Z

@Trust04zh there are some unresolved points. You can use the "Resolve conversation" button to mark off things you have already done, in order not to forget anything. #8180 (comment) #8180 (comment)

Trust04zh · 2023-01-24T06:27:59Z

@Stypox Now they are marked as resolve. Sorry I took a few days to reply, I just took a vacation.

Stypox · 2023-01-28T21:10:44Z

Mmmh, you didn't push any new commit though, so I guess you misunderstood me: the things I did not mark as done myself were the ones that still needed to be done ;-)

I unresolved the two mentioned comments again. It's just two small things.

Trust04zh · 2023-01-30T09:05:40Z

@Stypox updated now, thank you for the reminder, I did forget to deal with them 😟

…ations The following is the list of all commits squashed together: Regain function for option `Positions in lists` use option `Resume playback` to control display of progress info in VideoDetailFragment, remove this (extra) function from option `Positions in lists`. remove extra check for live streams, live streams updates just as non-live streams. fix TeamNewPipe#8176 by eliminating exit delay Regain function for option `Positions in lists` update code with developer's comments apply static import to methods in util class DependentPreferenceHelper Regain function for option `Positions in lists` use option `Resume playback` to control display of progress info in VideoDetailFragment, remove this (extra) function from option `Positions in lists`. remove extra check for live streams, live streams updates just as non-live streams. fix behavior for displaying progress bar when autoplay off but video resume on not to retrieve unnecessary states when position in lists disabled fix mistake in code simplify conditional logic update doc comment and remove unused method Fix not showing duration if position indicators disabled Positions in lists only depends on watch history

Stypox

I made some changes:

rebased and squashed commits
Now positions in lists only depends on watch history. Was there a specific reason for them to depend on "Resume playback"? One might want to see progress on items without resuming playback. 3ee3ddb
Fix not showing duration if position indicators disabled. A condition was not in the correct place. a3699ca

Do these changes look good to you @Trust04zh?

sonarcloud · 2023-02-07T08:58:15Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
4.3% Duplication

Trust04zh · 2023-02-08T02:50:49Z

@Stypox yes, they are good to me.

Now positions in lists only depends on watch history. Was there a specific reason for them to depend on "Resume playback"? One might want to see progress on items without resuming playback.

In my understanding, displaying progress just tells playback information vaguely, and if users do not want to resume playback, it does not quite make sense. There was no clear discussion for this point in the previous issue, and the change is fine to me.

opusforlife2 · 2023-03-02T20:08:03Z

Thanks again, @Trust04zh!

opusforlife2 · 2023-04-07T17:49:41Z

@Trust04zh Could I bother you to update the PR description at the top with the latest changes? It's outdated, and difficult to understand exactly what the final state of things is.

I think the videos are outdated as well now.

Trust04zh · 2023-04-09T03:39:35Z

@opusforlife2

I have updated the description of this pr based on the final changes, including:

Indicating that Resume playback did not become a dependency of Positions in lists.
Adding an explanation of what changes are reflected in the two videos.

I think that the description now clarifies things, and I'm willing to update if there are still unclear points for you.

opusforlife2 · 2023-04-09T13:11:25Z

Thanks!

Sorry, I don't understand point 4 at all. I was basically asking for changes that are visible to the user, not what code changes have been done. Point 2 is clear to me, from that perspective. Not sure what part of the videos is addressing point 4.

Trust04zh · 2023-04-09T13:31:44Z

Change 4 is for fixing #8176. In the "before" video, around 38 to 39 second, you can see the inconsistent behavior that the playback information becomes wrong for an instant.

I added a note for change 4 to indicate that it is for fixing #8176.

opusforlife2 · 2023-04-09T13:34:20Z

Ah. Got it. Thanks!

litetex reviewed Apr 15, 2022

View reviewed changes

Stypox force-pushed the dev branch from a81dbe2 to dc7fce8 Compare June 24, 2022 16:53

Stypox requested changes Jul 13, 2022

View reviewed changes

opusforlife2 added GUI Issue is related to the graphical user interface waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. labels Aug 23, 2022

github-actions bot removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Aug 25, 2022

opusforlife2 requested a review from Stypox August 31, 2022 19:51

Stypox requested changes Nov 22, 2022

View reviewed changes

Stypox requested changes Dec 31, 2022

View reviewed changes

Stypox force-pushed the fix-4053-8176 branch from 3ee3ddb to e947e86 Compare February 7, 2023 08:50

Stypox approved these changes Feb 7, 2023

View reviewed changes

Stypox added the history Anything to do with previously watched stuff label Feb 7, 2023

Stypox merged commit 097c236 into TeamNewPipe:dev Feb 26, 2023

Stypox mentioned this pull request Mar 1, 2023

Release v0.25.1 (please TEST!) #9870

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make UI behavior for playback information display more consistent #8180

Make UI behavior for playback information display more consistent #8180

Trust04zh commented Apr 12, 2022 •

edited

Loading

litetex left a comment

Trust04zh commented Apr 15, 2022 •

edited

Loading

Trust04zh commented Apr 18, 2022

sonarcloud bot commented Apr 18, 2022

Trust04zh commented Jun 25, 2022

Stypox left a comment

opusforlife2 commented Aug 23, 2022

Trust04zh commented Aug 25, 2022

sonarcloud bot commented Aug 29, 2022

Trust04zh commented Aug 29, 2022

Stypox left a comment

Trust04zh commented Dec 4, 2022

sonarcloud bot commented Dec 4, 2022

Trust04zh commented Jan 20, 2023

Stypox commented Jan 20, 2023

Trust04zh commented Jan 24, 2023 •

edited

Loading

Stypox commented Jan 28, 2023 •

edited

Loading

Trust04zh commented Jan 30, 2023

Stypox left a comment

sonarcloud bot commented Feb 7, 2023

Trust04zh commented Feb 8, 2023

opusforlife2 commented Mar 2, 2023

opusforlife2 commented Apr 7, 2023

Trust04zh commented Apr 9, 2023

opusforlife2 commented Apr 9, 2023

Trust04zh commented Apr 9, 2023 •

edited

Loading

opusforlife2 commented Apr 9, 2023

Make UI behavior for playback information display more consistent #8180

Make UI behavior for playback information display more consistent #8180

Conversation

Trust04zh commented Apr 12, 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

litetex left a comment

Choose a reason for hiding this comment

Trust04zh commented Apr 15, 2022 • edited Loading

Trust04zh commented Apr 18, 2022

sonarcloud bot commented Apr 18, 2022

Trust04zh commented Jun 25, 2022

Stypox left a comment

Choose a reason for hiding this comment

opusforlife2 commented Aug 23, 2022

Trust04zh commented Aug 25, 2022

sonarcloud bot commented Aug 29, 2022

Trust04zh commented Aug 29, 2022

Stypox left a comment

Choose a reason for hiding this comment

Trust04zh commented Dec 4, 2022

sonarcloud bot commented Dec 4, 2022

Trust04zh commented Jan 20, 2023

Stypox commented Jan 20, 2023

Trust04zh commented Jan 24, 2023 • edited Loading

Stypox commented Jan 28, 2023 • edited Loading

Trust04zh commented Jan 30, 2023

Stypox left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 7, 2023

Trust04zh commented Feb 8, 2023

opusforlife2 commented Mar 2, 2023

opusforlife2 commented Apr 7, 2023

Trust04zh commented Apr 9, 2023

opusforlife2 commented Apr 9, 2023

Trust04zh commented Apr 9, 2023 • edited Loading

opusforlife2 commented Apr 9, 2023

Trust04zh commented Apr 12, 2022 •

edited

Loading

Trust04zh commented Apr 15, 2022 •

edited

Loading

Trust04zh commented Jan 24, 2023 •

edited

Loading

Stypox commented Jan 28, 2023 •

edited

Loading

Trust04zh commented Apr 9, 2023 •

edited

Loading