Migrate video description and about channel fragments to Jetpack Compose#11489
Migrate video description and about channel fragments to Jetpack Compose#11489Isira-Seneviratne wants to merge 27 commits intoTeamNewPipe:refactorfrom
Conversation
Isira-Seneviratne
force-pushed
the
Video-description-compose
branch
from
7788a20 to
de6285b
Compare
github-actions
bot
added
size/giant
Isira-Seneviratne
force-pushed
the
Video-description-compose
branch
from
875e3fa to
45aa445
Compare
Isira-Seneviratne
changed the title
Isira-Seneviratne
force-pushed
the
Video-description-compose
branch
from
954770b to
387a4d5
Compare
AudricV
left a comment
AudricV
left a comment
There was a problem hiding this comment.
Thank you! According to your what you provided, this finally fixes the scrolling issue when selecting text 🎉
In my opinion, the tag chips component isn't the right one. An ElevatedSuggestionChip or something similar would probably look a lot better and similar to the current one used in XML.
Like other metadata items and in the current design, the Tags text should be upper-case.
You should not talk about a video description but a stream description, there are audio-only supported services. So your new VideoDescriptionSection composable and its corresponding package should be renamed :)
| onTextLayout: (TextLayoutResult) -> Unit = {}, | ||
| style: TextStyle = LocalTextStyle.current | ||
| ) { | ||
| // TODO: Handle links and hashtags, Markdown. |
There was a problem hiding this comment.
This must be solved before the PR can be merged.
Isira-Seneviratne
force-pushed
the
Video-description-compose
branch
from
edf3782 to
8f9faf3
Compare
Isira-Seneviratne
force-pushed
the
Video-description-compose
branch
from
8d93272 to
0d12cfc
Compare
ShareASmile
added
rewrite
# Conflicts: # app/build.gradle # app/src/main/java/org/schabi/newpipe/fragments/detail/DescriptionFragment.java # app/src/main/java/org/schabi/newpipe/fragments/list/channel/ChannelAboutFragment.java # app/src/main/java/org/schabi/newpipe/ui/components/items/ItemList.kt # app/src/main/java/org/schabi/newpipe/ui/components/items/stream/StreamMenu.kt
# Conflicts: # app/src/main/java/org/schabi/newpipe/ktx/Bundle.kt
|
I suggest three things on this PR:
|
|
|
I got a weird behaviour while testing which results in the comments reloading:
Further swiping and changing back to the comments tab cause further reloads of the tab. I am currently unable to capture a screenrecording. I'll try this later if you need it. |
|
@TobiGr yeah that's because comments are not cached anymore... This was a leftover from the comments PR. It's normal for Android tabs to get destroyed at a distance of 2 from the current position. |
|
Rewriting the video details fragment using Compose should fix it; alternatively, in the short term it could be migrated to using a view model that could be shared with the comment fragment. |
|
Isira-Seneviratne
force-pushed
the
Video-description-compose
branch
from
80d71ac to
ed2c8e9
Compare
# Conflicts: # app/src/main/java/org/schabi/newpipe/fragments/detail/DescriptionFragment.java # app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java # app/src/main/java/org/schabi/newpipe/fragments/list/channel/ChannelAboutFragment.java
Isira-Seneviratne
force-pushed
the
Video-description-compose
branch
from
ed2c8e9 to
c3cf3d8
Compare
Isira-Seneviratne
added
the
ready for review
| for (tag in sortedTags) { | ||
| ElevatedSuggestionChip( | ||
| onClick = { | ||
| NavigationHelper.openSearchFragment( |
There was a problem hiding this comment.
In the interests of keeping composables "dumb", only responsible for the UI and not business logic, I suggest decoupling the Navigation logic from the composable.
Add an onClick lambda and basically buggble this up to the Fragment/Screen level.
onTagClick: (tag: String) -> Unit,
...
onClick = {
onTagClick(tag)
},
In the future when fragments are replaced with compose navigation, this should make it easier to implement the compose navigation.
| ) = content { | ||
| AppTheme { | ||
| Surface(color = MaterialTheme.colorScheme.background) { | ||
| StreamDescriptionSection(requireArguments().serializable(KEY_INFO)!!) |
There was a problem hiding this comment.
Instead of forcing this to be non-null, you could wrap it with a let. That way if the arguments happen to be null, the code renders a blank surface instead of crashing.
|
|
||
| if (hasDescription) { | ||
| item { | ||
| val description = parseDescription(streamInfo.description, streamInfo.serviceId) |
There was a problem hiding this comment.
ParseDescription has to perform some work in order to generate the annotated string. It is remembered, so hopefully it won't cause any unnecessary recompositions. However since it has to do do some work, it would be best to have an AboutChannelViewModel and DescriptionViewModel where streaminfo/channelInfo could be transformed off the UI thread and then the transformed static data could be passed to the composables.
It may require the introduction of a loading state.
I was heading in that direction here:
https://github.com/cameocoder/NewPipe/blob/channel-about-refactor/app/src/main/java/org/schabi/newpipe/fragments/list/channel/ChannelAboutViewModel.kt
7 participants
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Screen_recording_20240831_154300.mp4
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Due diligence