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

PeerTube support #2201

Merged
merged 30 commits into from Dec 10, 2019
Merged

PeerTube support #2201

merged 30 commits into from Dec 10, 2019

Conversation

@yausername
Copy link
Member

yausername commented Mar 9, 2019

@yausername

This comment has been minimized.

Copy link
Member Author

yausername commented Mar 9, 2019

@theScrabi

This comment has been minimized.

Copy link
Member

theScrabi commented Mar 9, 2019

Nice ... although the orange is a little to bright for my taste :D.

@yausername

This comment has been minimized.

Copy link
Member Author

yausername commented Mar 10, 2019

made the suggested changes.

@Booteille

This comment has been minimized.

Copy link

Booteille commented Apr 1, 2019

What's the status on this issue?

@theScrabi

This comment has been minimized.

Copy link
Member

theScrabi commented Apr 1, 2019

Gets merged, its just middle of semester and cur. time is rare.

@Booteille

This comment has been minimized.

Copy link

Booteille commented Apr 1, 2019

Yeah, I can understand!

Thanks for took the time to answer!

@Serkan-devel

This comment has been minimized.

Copy link

Serkan-devel commented Jun 15, 2019

Does this PR lack anything still?

@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Jun 15, 2019

I did not have time to review the extractor yet. The front-end implementation is good now.

@TheAssassin

This comment has been minimized.

Copy link
Member

TheAssassin commented Jul 22, 2019

@Serkan-devel

This comment has been minimized.

Copy link

Serkan-devel commented Jul 23, 2019

Is it being reviewed?

@theScrabi

This comment has been minimized.

Copy link
Member

theScrabi commented Jul 23, 2019

Not yet. I may need to take a look at the backend first.

@ghost

This comment has been minimized.

Copy link

ghost commented Nov 11, 2019

Wow, that's amazing!

Although, I found some bugs:

  • Share doesn't share the video's link but api's one :

Video: https://framatube.org/videos/watch/bb19bdca-4619-4e4e-84b7-c1ee409412b2
Link I got from sharing with the app: https://framatube.org/api/v1/videos/bb19bdca-4619-4e4e-84b7-c1ee409412b2
Same behaviour when you press "open in browser"

  • Subtitles aren't supported:

video: https://framatube.org/videos/watch/9c9de5e8-0a1e-484a-b099-e80766180a6d
on this video there are a lot of subtitles (Chinese, Dutch, English, French, German, Russian, Swedish) but no one is showed on the app: subtitles_not_supported

  • Some characters aren't supported in comments (I found ' and ") :

In-app screenshot : inapp_Screenshot
Firefox :
Screenshot_20191111-093016_Firefox

@yausername

This comment has been minimized.

Copy link
Member Author

yausername commented Nov 15, 2019

@Ozyc I have fixed all three issues.
Please find the debug apk in the top comment.
Thanks

@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Nov 16, 2019

Nice. Works like a charm. We need to review the extractor PR, it has been put on hold for way to long.

yausername added 2 commits Dec 2, 2019
@yausername

This comment has been minimized.

Copy link
Member Author

yausername commented Dec 2, 2019

* Trendings are not the same as those on the current instance. Compare https://framatube.org/videos/trending and Trendings in the app, you'll see that trending videos in the app are very old : 10 months to 2 years, whereas on the website you can have videos released less than a week ago

I have fixed this issue in the new apk.

Copy link
Member

TobiGr left a comment

Nice! The way one can add a new instance looks good now :)
There are two final things which need to be done before I can merge this:
the setting order and the extractor need to be updated

app/src/main/res/xml/content_settings.xml Outdated Show resolved Hide resolved
@TobiGr TobiGr added the peertube label Dec 2, 2019
yausername added 2 commits Dec 3, 2019
@yausername

This comment has been minimized.

Copy link
Member Author

yausername commented Dec 3, 2019

the setting order and the extractor need to be updated

done

@gkeegan

This comment has been minimized.

Copy link
Contributor

gkeegan commented Dec 3, 2019

I think it should be kept into consideration for the future to add links to the sites in NewPipe, so that people can support them if they want. Maybe not for YouTube, but at least for PeerTube or the instances of similar services that support open source and free software.

This could be added for MediaCCC if they accept donations too.

@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Dec 3, 2019

I think it should be kept into consideration for the future to add links to the sites in NewPipe, so that people can support them if they want. Maybe not for YouTube, but at least for PeerTube or the instances of similar services that support open source and free software.

@gkeegan That's a good idea. Can you please open an issue for that? We should discuss where to place the donation info / buttons. There was an attempt to get some donation information of YouTube channels already.

the setting order and the extractor need to be updated

done

@yausername thanks. I'll merge this tonight.

@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Dec 3, 2019

I just did my final round of testing. Everything works fine :)
There is only one child in the rooms which makes trouble all the time: KitKat... There is a ssl exception with https://framatube.org. However, other peertube instances work fine.

DefaultKioskFragment@9d4f3918: onError() called with: exception = [javax.net.ssl.SSLHandshakeException: javax.net.ssl.SSLProtocolException: SSL handshake aborted: ssl=0xb9534dc0: Failure in SSL library, usually a protocol error
    error:1407742E:SSL routines:SSL23_GET_SERVER_HELLO:tlsv1 alert protocol version (external/openssl/ssl/s23_clnt.c:741 0x8d95e990:0x00000000)]

#2792 might solve this issue. For this reason, I'd like to proceed and merge this?
@Stypox @yausername @theScrabi @gkeegan Do you consider this as blocking? (this problem has existed before and made media.ccc unusable for KitKat users, see #2777)

@gkeegan

This comment has been minimized.

Copy link
Contributor

gkeegan commented Dec 3, 2019

I don't think it should be blocked from merge if there is a possible fix on the way. So long as a release doesn't happen before 2792 is merged (or issue is fixed by other methods), everything should be fine.

I think that discussion may need to happen in the future about what to do with KitKat as it seems problematic.

@Stypox

This comment has been minimized.

Copy link
Member

Stypox commented Dec 4, 2019

On Android 7.1 it currently works only for some instances (e.g. https://diode.zone works while https://framatube.org gives "Network Error")

@Stypox

This comment has been minimized.

Copy link
Member

Stypox commented Dec 4, 2019

And if I try to remove and re-insert Framatube it tells me "failed to validate instance"

(Also, the first letter of the error should probably be uppercase)

@yausername

This comment has been minimized.

Copy link
Member Author

yausername commented Dec 4, 2019

And if I try to remove and re-insert Framatube it tells me "failed to validate instance"

@Stypox Does it happen every time? I tried on a 7.1.1 emulator and not able to reproduce.

(Also, the first letter of the error should probably be uppercase)

fixed

@TobiGr

This comment has been minimized.

Copy link
Member

TobiGr commented Dec 6, 2019

On Android 7.1 it currently works only for some instances (e.g. https://diode.zone works while https://framatube.org gives "Network Error")

@Stypox Can you please verify that this is also the ssl certificate problem? In this case, I'd like to merge.

@Stypox

This comment has been minimized.

Copy link
Member

Stypox commented Dec 9, 2019

@yausername correct the first lowercase letter also for other strings you added

@Stypox

This comment has been minimized.

Copy link
Member

Stypox commented Dec 9, 2019

@TobiGr @yausername the issue is caused by a failed handshake, so this can be merged.

javax.net.ssl.SSLProtocolException: SSL handshake terminated: ssl=0x7dd8c31180: Failure in SSL library, usually a protocol error
error:10000410:SSL routines:OPENSSL_internal:SSLV3_ALERT_HANDSHAKE_FAILURE (external/boringssl/src/ssl/s3_pkt.c:610 0x7dd8cfdd40:0x00000001)
error:1000009a:SSL routines:OPENSSL_internal:HANDSHAKE_FAILURE_ON_CLIENT_HELLO (external/boringssl/src/ssl/s3_clnt.c:764 0x7dead5cf76:0x00000000)
@yausername

This comment has been minimized.

Copy link
Member Author

yausername commented Dec 10, 2019

@yausername correct the first lowercase letter also for other strings you added

done

@TobiGr
TobiGr approved these changes Dec 10, 2019
@TobiGr TobiGr dismissed theScrabi’s stale review Dec 10, 2019

Outdated review

Maintainence automation moved this from Waiting 4 review & testing to Waiting for merge Dec 10, 2019
@TobiGr TobiGr merged commit b2bbdc6 into TeamNewPipe:dev Dec 10, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - app/build.gradle (theScrabi) No known issues
Details
security/snyk - build.gradle (theScrabi) No known issues
Details
Maintainence automation moved this from Waiting for merge to Merged Dec 10, 2019
@B0pol

This comment has been minimized.

Copy link

B0pol commented Dec 10, 2019

Wow, finally merged, huge thanks to you !!!
by the way, I found there was a bounty for PeerTube : TeamNewPipe/NewPipeExtractor#79
Did you know ? I hope it's still available and you'll win it 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Maintainence
  
Merged
10 participants
You can’t perform that action at this time.