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

comments support #1650

Merged
merged 38 commits into from
Feb 21, 2019
Merged

comments support #1650

merged 38 commits into from
Feb 21, 2019

Conversation

yausername
Copy link
Contributor

@yausername yausername commented Sep 3, 2018

The code is crappy and needs lot of work.

68747470733a2f2f692e696d6775722e636f6d2f62497a784b73702e6a7067

@TobiGr
Copy link
Contributor

TobiGr commented Sep 11, 2018

@yausername Please rebase your changes in this and in the extractor repo so we can start to test and review.

campbellers
campbellers previously approved these changes Sep 15, 2018
@theScrabi
Copy link
Member

I will start to reviw this soon.

@yausername
Copy link
Contributor Author

requires TeamNewPipe/NewPipeExtractor#101

@theScrabi
Copy link
Member

@yausername there are several changes that have to be made in the backend first: https://github.com/TeamNewPipe/NewPipeExtractor/pull/101/files

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Hey @yausername,
I quickly tested your changes and what I saw looks really good 👍 Unfortunately, I do not have time for code reviews at the moment.

  • There should be a content setting which allows the user to disable comments.
  • Loading the video detail view seems to be way slower than usual. Do you fetch the comments asynchronously? I am not aware of any code you did here or in the extractor, so this is something @theScrabi might discuss with you further.
  • I am not sure if this is a good idea, but maybe we should make it easier to switch between the "comments" and "next video" tabs. Do you guys think we should detect horizontal swipes?

In case we are able to fetch nested comments one day, we need to discuss a proper design for that.

@yausername
Copy link
Contributor Author

Hi @TobiGr,
Thanks for the feedback.

@yausername
Copy link
Contributor Author

@TobiGr I have made some change for

  • adding content setting to disable comments.
  • switch tabs with horizontal swipe - viewpager
  • Changed from parallaxscrollview to coordinate layout.

Please test and give feedback if/when you have time

Thanks

@TobiGr
Copy link
Contributor

TobiGr commented Oct 11, 2018

@yausername Thanks for taking a look at my suggestions. I am sorry for the late response, but I rarely find spare time for larger PRs at the moment.

Yes I am fetching comments asynchronously. Did you check on the latest code 803b8ea because the video detail seems to load just as quick for me.

Maybe it was my wifi. It is kinda unstable and slow...

I really like your changes so far. Comment support will be an significant improvement for NewPipe 👍 There are still some things to do from UI and UX side, but I guess someone will find time to take a look at the code soon.

Below you can see two screenshots. The left one was taken with NewPipe 0.14.2 and the right one at your latest commit. As you can see (I hope so, the divider is hard to see at my screenshots), we separate the "next video" from the other recommended videos. It might be good if you keep the next video and separate it from the others.

When comments are disabled, there is no "next video" heading. But to be honest, I am not sure if we need one. @theScrabi What do you think?
We might also discuss what to do with the black divider which usually divides the tab headings and their content. In my opinion, we can keep it, but there should be more horizontal space around it.

@yausername
Copy link
Contributor Author

@TobiGr Thanks for taking time for the feedback. I will look into your suggestions.

@theScrabi
Copy link
Member

theScrabi commented Oct 12, 2018

@yausername
There is another thing. I don't like the huge tab 'commends' and 'downloads' as they take away a loot of screenspace.
I'd rather like to have something like floating indicator dots:

@avently
Copy link
Contributor

avently commented Oct 12, 2018

@yausername thanks, man! This is what I wanted long time

@theScrabi
Copy link
Member

@TobiGr you have ben granded write access for frontend as well as backend, please feel free to review and merge PRs (especially the smaller ones). You would help me a lot, and help to make releases go faster :)

@avently
Copy link
Contributor

avently commented Feb 17, 2019

Finally.
@TobiGr, as your first task, please, merge this PR:) with your help this project will get new life

@TobiGr
Copy link
Contributor

TobiGr commented Feb 19, 2019

@theScrabi Thanks, I'll take a look at the extractor once I finished my exam on monday.

@theScrabi
Copy link
Member

Merged the extractor though :). I had time today.

@theScrabi
Copy link
Member

Code does not look bad to me. However could someone else please give another final review?

@theScrabi
Copy link
Member

Please also think about QA.
Therefore here is a version build and signed with my keys:
NewPipe-v0.15.1_with_commends.apk.zip

Copy link
Member

@theScrabi theScrabi left a comment

Choose a reason for hiding this comment

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

screenshot_20190219-180434_newpipe

Padding in settings should be removed.

@justanidea
Copy link
Contributor

This works well on my device. Anyways, would be possible to set the default pannel on the next videos? I use it more often than comments, and i think its going to be really annoying to swipe at each video
A big TY anyways

@avently
Copy link
Contributor

avently commented Feb 19, 2019

Absolutely agree with panels reordering

@yausername
Copy link
Contributor Author

screenshot_20190219-180434_newpipe

Padding in settings should be removed.

Done

@theScrabi
Copy link
Member

@justanidea maybe the last state of the panel could be saved.
You guys are welcome to help :)

@theScrabi
Copy link
Member

F*** it, it worx.

@theScrabi theScrabi merged commit e69c45a into TeamNewPipe:dev Feb 21, 2019
@theScrabi
Copy link
Member

theScrabi commented Feb 21, 2019

@yausername thanks a lot, great work 👍

@cryptono
Copy link

@theScrabi, when is the next apk release going to be published? Can't wait for the comment support! 👍

@theScrabi
Copy link
Member

Lets say next thing i do in newpipe is another release ;)

@theScrabi
Copy link
Member

@yausername I encountered the first bug, would you please take a look at it?

@yausername
Copy link
Contributor Author

@yausername I encountered the first bug, would you please take a look at it?

Sure, I'll take a look.

This was referenced Feb 24, 2019
@skylarmt
Copy link

skylarmt commented Mar 3, 2019

I just want to say thank you for the comments support, it's always been annoying when a video says there's more info in the comments. However, it's funny how at the same time NewPipe gets comments, YouTube starts shutting down comments on an entire category of videos :)

@Mhowser
Copy link

Mhowser commented Mar 3, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.