-
Notifications
You must be signed in to change notification settings - Fork 182
Fix youtube activity #166
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
base: development
Are you sure you want to change the base?
Fix youtube activity #166
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,11 +7,15 @@ import android.view.* | |
| import android.view.animation.DecelerateInterpolator | ||
| import androidx.core.content.ContextCompat | ||
| import androidx.fragment.app.Fragment | ||
| import androidx.lifecycle.lifecycleScope | ||
| import androidx.preference.PreferenceManager | ||
| import kotlinx.coroutines.launch | ||
| import tech.salroid.filmy.R | ||
| import tech.salroid.filmy.data.local.model.TrailerData | ||
| import tech.salroid.filmy.databinding.AllTrailerLayoutBinding | ||
| import tech.salroid.filmy.ui.adapters.MovieTrailersAdapter | ||
| import tech.salroid.filmy.ui.full.YoutubePlayerActivity.Companion.IS_SHORT | ||
| import tech.salroid.filmy.utility.isYoutubeShortVideo | ||
| import tech.salroid.filmy.utility.themeSystemBars | ||
| import kotlin.math.hypot | ||
| import androidx.core.graphics.toColorInt | ||
|
|
@@ -130,10 +134,14 @@ class AllTrailersFragment : Fragment() { | |
| } | ||
|
|
||
| private fun playTrailerOnYoutube(trailerId: String, trailerTitle: String?) { | ||
| Intent(activity, YoutubePlayerActivity::class.java).run { | ||
| putExtra(YoutubePlayerActivity.VIDEO_ID, trailerId) | ||
| putExtra(YoutubePlayerActivity.VIDEO_TITLE, trailerTitle) | ||
| startActivity(this) | ||
| lifecycleScope.launch { | ||
| val isShort = isYoutubeShortVideo(trailerId) | ||
| Intent(activity, YoutubePlayerActivity::class.java).run { | ||
| putExtra(YoutubePlayerActivity.VIDEO_ID, trailerId) | ||
| putExtra(YoutubePlayerActivity.VIDEO_TITLE, trailerTitle) | ||
| putExtra(IS_SHORT, isShort) | ||
| startActivity(this) | ||
| } | ||
| } | ||
|
Comment on lines
+137
to
145
|
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,15 +2,18 @@ package tech.salroid.filmy.ui.full | |||||
|
|
||||||
| import android.annotation.SuppressLint | ||||||
| import android.content.pm.ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE | ||||||
| import android.content.pm.ActivityInfo.SCREEN_ORIENTATION_PORTRAIT | ||||||
| import android.content.pm.ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED | ||||||
| import android.graphics.Color | ||||||
| import android.os.Bundle | ||||||
| import android.view.Gravity | ||||||
| import android.view.HapticFeedbackConstants.VIRTUAL_KEY | ||||||
| import android.view.View | ||||||
| import android.webkit.WebChromeClient | ||||||
| import android.webkit.WebResourceRequest | ||||||
| import android.webkit.WebView | ||||||
| import android.webkit.WebViewClient | ||||||
| import android.widget.FrameLayout | ||||||
| import androidx.appcompat.app.AppCompatActivity | ||||||
| import androidx.core.view.WindowCompat | ||||||
| import androidx.core.view.WindowInsetsCompat | ||||||
|
|
@@ -25,22 +28,28 @@ class YoutubePlayerActivity : AppCompatActivity() { | |||||
|
|
||||||
| private var videoId: String? = null | ||||||
| private var videoTitle: String? = null | ||||||
| private var isShort: Boolean = false | ||||||
| private var html5VideoView: View? = null | ||||||
| private var customViewCallback: WebChromeClient.CustomViewCallback? = null | ||||||
|
|
||||||
| companion object { | ||||||
| const val VIDEO_ID = "video_id" | ||||||
| const val VIDEO_TITLE = "video_title" | ||||||
| private const val YT_BASE_URL = "https://www.youtube.com" | ||||||
| const val IS_SHORT = "is_short" | ||||||
| private const val APP_BASE_URL = "https://app.filmy.tech" | ||||||
|
||||||
| private const val APP_BASE_URL = "https://app.filmy.tech" | |
| private const val APP_BASE_URL = "https://www.youtube.com" |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The aspect ratio calculation for shorts appears to be incorrect. YouTube Shorts are vertical videos with a 9:16 aspect ratio, but this code calculates videoHeight = screenWidth * 16 / 9, which gives a 16:9 (landscape) aspect ratio. This would result in the video being displayed in the wrong dimensions for portrait short videos. The calculation should be videoHeight = screenWidth * 16 / 9 for landscape videos, but for shorts it should match the device height or use a different calculation appropriate for vertical content.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,9 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package tech.salroid.filmy.utility | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import kotlinx.coroutines.Dispatchers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import kotlinx.coroutines.withContext | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import okhttp3.OkHttpClient | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import okhttp3.Request | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Generates the HTML string required to embed a YouTube video using an iframe. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -25,10 +29,10 @@ fun getYouTubeIframeHTML(youtubeVideoId: String): String { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'controls': 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'fs': 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'rel': 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'modestbranding': 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'iv_load_policy': 3, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'showinfo': 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'playsinline': 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'playsinline': 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'origin': 'https://app.filmy.tech', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'enablejsapi': 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| events: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'onReady': function(event) { event.target.playVideo(); } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -43,4 +47,23 @@ fun getYouTubeIframeHTML(youtubeVideoId: String): String { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </body> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </html> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """.trimIndent() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private val client = OkHttpClient() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | |
| * Checks whether the given YouTube video is likely a YouTube Short. | |
| * | |
| * This function performs an HTTP request to the standard watch URL for the | |
| * provided video ID and inspects the response to infer if it corresponds to | |
| * a Shorts video. | |
| * | |
| * This is a suspending function that executes the network request on | |
| * [Dispatchers.IO]. | |
| * | |
| * @param videoId The unique identifier of the YouTube video to inspect. | |
| * @return `true` if the video appears to be a YouTube Short, `false` otherwise | |
| * or if the request fails. | |
| */ |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OkHttpClient is created as a module-level private property, which means a new instance is created for each process. While this works, it doesn't follow the recommended practice of sharing a single OkHttpClient instance across the app. The codebase already has a centralized OkHttpClient provided through dependency injection in MoviesModule.kt (line 24). Consider using dependency injection to provide this OkHttpClient instance or reusing the existing injected client to maintain consistency and improve resource efficiency (connection pooling, thread pools, etc.).
| private val client = OkHttpClient() | |
| suspend fun isYoutubeShortVideo(videoId: String): Boolean = | |
| suspend fun isYoutubeShortVideo(videoId: String, client: OkHttpClient): Boolean = |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HTTP request to YouTube is made over HTTPS, but there's no validation of the SSL certificate or additional security measures in the OkHttpClient configuration. While this is generally acceptable for fetching public content, it's worth noting that the OkHttpClient could be configured with certificate pinning or other security measures if needed, especially since the app appears to handle sensitive user data (accounts, favorites, etc.). This is more of a consideration than a critical issue.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach of scraping YouTube's HTML page to detect shorts is fragile and may break if YouTube changes their HTML structure. YouTube can change their page markup at any time without notice, which would cause this detection to fail silently (always returning false). Consider using YouTube's official Data API v3 which provides a more reliable and stable way to get video information, or document this as a known limitation with fallback behavior.
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isYoutubeShortVideo function lacks error handling for network failures and exceptions. If the network call throws an exception (e.g., timeout, network unavailable, malformed URL), it will crash or propagate the exception to the caller. This could cause the app to fail when trying to play a YouTube video. Consider wrapping the network call in a try-catch block and returning a default value (false) in case of errors, so the video playback can proceed with the default non-short handling.
| val url = "https://www.youtube.com/watch?v=$videoId" | |
| val request = Request.Builder().url(url).build() | |
| client.newCall(request).execute().use { response -> | |
| if (!response.isSuccessful) return@withContext false | |
| val body = response.body?.string() ?: return@withContext false | |
| val shortsPattern = Regex("""href=["'](https://www\.youtube\.com/shorts/[^"']+)["']""") | |
| val match = shortsPattern.find(body) | |
| return@withContext match != null | |
| try { | |
| val url = "https://www.youtube.com/watch?v=$videoId" | |
| val request = Request.Builder().url(url).build() | |
| client.newCall(request).execute().use { response -> | |
| if (!response.isSuccessful) { | |
| return@use false | |
| } | |
| val body = response.body?.string() ?: return@use false | |
| val shortsPattern = Regex("""href=["'](https://www\.youtube\.com/shorts/[^"']+)["']""") | |
| val match = shortsPattern.find(body) | |
| match != null | |
| } | |
| } catch (e: Exception) { | |
| false |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isYoutubeShortVideo function makes an HTTP request to fetch the entire YouTube page HTML just to check if it's a short video. This is inefficient and wasteful, as it downloads potentially large HTML content for a simple check. Additionally, there's no caching mechanism, so repeated calls for the same video ID will make redundant network requests. Consider using YouTube's Data API v3 instead (the app already has a YOUTUBE_API_KEY configured), or at least implement caching to avoid repeated network calls for the same video ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The network call to
isYoutubeShortVideois launched in a coroutine without error handling. If the network call fails or throws an exception, the coroutine will crash and the Intent with the activity will never be started, leaving the user with no feedback. The user would click on the trailer and nothing would happen. Consider wrapping the call in a try-catch block and providing fallback behavior (e.g., launch the video with isShort=false as default) or showing an error message to the user.