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

Merge player classes into a single one #5371

Merged
merged 5 commits into from
Jan 14, 2021
Merged

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Jan 8, 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

This PR merges the three files of the player, namely BasePlayer, VideoPlayer and VideoPlayerImpl, into only one. Having three files was totally useless since code randomly spanned across all three files without any clear distinction. Before unified player three different classes were needed to allow for extension, but now that's not needed anymore. Merging everything together also removes lines of code and makes the code simpler to read, since everything is in one place, as there are no overridden methods all over the place anymore.

  • Create different sections inside the file with big title comments, grouping together related functions. Add //region and //endregion comments before and after each section, allowing them to be folded by the IDE.
  • Use only one enum for player type, also for intents, and therefore use intvalue = type.ordinal() and PlayerType.values()[intvalue]
  • Remove some unused functions
  • Move utility functions and functions that use preferences toPlayerHelper
  • Change checkPopupPositionBounds so that it tries to clamp both x and y bounds and not only one at a time
  • Fix wrong speed indicator in queue activity (finally I understood what ought to be done)
  • Remove viewpager hiding from In Fullscreen playback, toggle play/pause with hardware space button #5331
  • Fix view types different between player.xml and player.xml (large-land), causing view binding to require conversions: see Use view binding in VideoPlayer. #5253 (comment)
  • Merge ServicePlayerActivity and BackgroundPlayerActivity into a single PlayQueueActivity and remove useless fields getTag and redraw (which were not used)

I'd like this to be merged soon, otherwise it will be a mess to rebase.

Due diligence

@triallax triallax added the player Issues related to any player (main, popup and background) label Jan 8, 2021
@opusforlife2
Copy link
Collaborator

This has been bugging you for a while, hasn't it? xD

@Redirion
Copy link
Member

Redirion commented Jan 9, 2021

as your PR also modifies the VideoDetailFragment. Can you please remove the lines


and

that's from PR #5331. So I don't have to do that in another PR and force you to rebase / force push.

@vkay94
Copy link
Contributor

vkay94 commented Jan 9, 2021

I'm in favor of this :) It's always a struggle to "not breaking" stuff because of the super-calls, especially for methods like showControls and hideControls - requires a check on all classes - plus most (even if not all) of the UI component getters can be/are removed ^^

Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

I just did a quick run down, i skipped Player since there is no direct side-by-side comparison. But if you just pulled the methods upwards there shouldn't be much to look at.
PlayerHelper i also just skimmed through for the same reason.

Only thing that i would change is making seperate commits for those points. When looked at afterwards it then shows that its intended and not accidental.

Fix wrong speed indicator in queue activity (finally I understood what ought to be done)
Remove viewpager hiding from #5331

@@ -33,61 +33,59 @@
public class PlayerGestureListener
extends BasePlayerGestureListener
implements View.OnTouchListener {
private static final String TAG = ".PlayerGestureListener";
private static final boolean DEBUG = BasePlayer.DEBUG;
private static final String TAG = PlayerGestureListener.class.getSimpleName();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this done like this in other places too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is the way to do it

@@ -129,6 +128,7 @@ public boolean onCreateOptionsMenu(final Menu m) {
getMenuInflater().inflate(R.menu.menu_play_queue, m);
getMenuInflater().inflate(getPlayerOptionMenuResource(), m);
onMaybeMuteChanged();
onPlaybackParameterChanged(player.getPlaybackParameters());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this coming from?

Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the issue with the speed on the button being wrong

@avently
Copy link
Contributor

avently commented Jan 9, 2021

Isn't it quite hard to navigate in 4k lines of code?
This seems like a logical step forward. Maybe someone will find a way to divide the file into the pieces without direct connection so they can do it's job without touching other classes

@TacoTheDank
Copy link
Member

Isn't it quite hard to navigate in 4k lines of code?
This seems like a logical step forward. Maybe someone will find a way to divide the file into the pieces without direct connection so they can do it's job without touching other classes

Paging @XiangRongLin :)

@XiangRongLin
Copy link
Collaborator

@TacoTheDank Seeing how my other refactoring&tests PR are still just lying around, I'm not going to touch this. Refactoring something big like this needs many iterations, which does not work if each iteration has a very long wait time just to get feedback.

@Stypox
Copy link
Member Author

Stypox commented Jan 12, 2021

@XiangRongLin I separated the commits as you requested, and also merged and renamed ServicePlayerActivity into PlayQueueActivity

XiangRongLin
XiangRongLin previously approved these changes Jan 13, 2021
Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

Looks ok to me. I trust you with merging those classes, since like many said 4k lines is too much to review.

@opusforlife2
Copy link
Collaborator

Seeing how my other refactoring&tests PR are still just lying around, I'm not going to touch this. Refactoring something big like this needs many iterations, which does not work if each iteration has a very long wait time just to get feedback.

Yes, this is a chronic problem because of general lack of free time on the team. Plus even when a dev gets some time to work on Newpipe, they might have their own priority regarding what to work on.

@XiangRongLin @vkay94 You've both been given write access, so you can approve and merge PRs. Whenever you have the inclination, please go ahead and review/merge PRs that meet your standards. :)

Also, @XiangRongLin could you please join the newpipe IRC channel (preferably with a Matrix account)? @vkay94 is already there.

@Redirion
Copy link
Member

Redirion commented Jan 14, 2021

player.getPlayer() or player.isPlayerNull are looking a bit unfortunate. Maybe it should be renamed to exoInstance / playbackInstance or something like that? so it would become player.getExoInstance() and player.isExoInstanceNull

@Stypox
Copy link
Member Author

Stypox commented Jan 14, 2021

I rebased onto view binding and tested again, everything seems to be working fine. @Redirion I renamed playerIsNull into exoPlayerIsNull, is that ok? I don't know, though, what you are referring to with player.getPlayer, as there is no such method anymore. @Isira-Seneviratne I was able to solve the problem with incompatible view binding types by making sure the same view types are used in player.xml and player.xml (large-land)

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
Copy link
Member

I will take the bold move to merge this. I think this should go into public testing with the new release. This will give us the opportunity to collect more broad feedback.

@Redirion Redirion merged commit c90696e into TeamNewPipe:dev Jan 14, 2021
This was referenced Jan 18, 2021
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 20, 2021
@Stypox Stypox deleted the merge-player branch August 4, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants