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

Improvement: Reformat non-embedded to embedded youtube video link #970

Merged
merged 1 commit into from
Jan 20, 2024

Conversation

wutschel
Copy link
Collaborator

@wutschel wutschel commented Jan 8, 2024

Description

The implemented solution will identify multiple variants of youtube.com and youtu.be links (http, https, www or not), extract the video id and create a fresh FullHD embedded youtube link from this. This allows to show all such links with in-app preview.

Summary for release notes

Improvement: Reformat non-embedded to embedded youtube video link

XBMC Remote/ShowInfoViewController.m Outdated Show resolved Hide resolved
XBMC Remote/ShowInfoViewController.m Outdated Show resolved Hide resolved
XBMC Remote/ShowInfoViewController.m Outdated Show resolved Hide resolved
Copy link
Collaborator

@kambala-decapitator kambala-decapitator left a comment

Choose a reason for hiding this comment

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

in general looks good now, but I'd suggest also to do the following:

  • use NSURL as ivar instead of NSString: you always create NSURL to pass to NSURLComponents, and in the end you need NSURL to create request anyway
  • create NSURL and NSURLComponents outside of the new functions (i.e. do it in processTrailerFromString) and pass them to the functions

XBMC Remote/ShowInfoViewController.m Show resolved Hide resolved
XBMC Remote/ShowInfoViewController.m Show resolved Hide resolved
XBMC Remote/ShowInfoViewController.m Show resolved Hide resolved
XBMC Remote/ShowInfoViewController.m Outdated Show resolved Hide resolved
@wutschel
Copy link
Collaborator Author

in general looks good now, but I'd suggest also to do the following:

  • use NSURL as ivar instead of NSString: you always create NSURL to pass to NSURLComponents, and in the end you need NSURL to create request anyway
  • create NSURL and NSURLComponents outside of the new functions (i.e. do it in processTrailerFromString) and pass them to the functions

Good point, done.

@wutschel
Copy link
Collaborator Author

Squashed and rebased as I understood despite the comments it was approved. Just let me know, if you still would like me to do further changes.

@kambala-decapitator
Copy link
Collaborator

create NSURL and NSURLComponents outside of the new functions

seems you missed this point, could you adjust?

@wutschel
Copy link
Collaborator Author

seems you missed this point, could you adjust?

Yep, sorry. Was only focusing the NSURL part of it. Done now as well, out of consistency for all the link check methods.

The solution will identify multiple variants of youtube.com and youtu.be links (http, https, www or not), extract the video id and create a fresh FullHD embedded youtube link from this. This allows to show such links with in-app preview. Use NSURL for embedVideoURL ivar.
@wutschel
Copy link
Collaborator Author

Squashed and rebased, with latest fixups. More rework than expected for this initially small change, but the implementation is more straight forward now.

@kambala-decapitator kambala-decapitator merged commit 8f8c4ff into xbmc:master Jan 20, 2024
@wutschel wutschel deleted the improve_trailer branch January 20, 2024 12:45
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.

None yet

2 participants