Migrate related items fragment to Jetpack Compose #11383
Conversation
Isira-Seneviratne
changed the title
Isira-Seneviratne
force-pushed
the
Related-items-Compose
branch
2 times, most recently
from
c6ae703 to
75e18d1
Compare
Isira-Seneviratne
force-pushed
the
Related-items-Compose
branch
from
75e18d1 to
03b47b8
Compare
|
| } | ||
|
|
||
| // Handle long clicks for stream items | ||
| // TODO: Adjust the menu display depending on where it was triggered |
There was a problem hiding this comment.
I'm a little new to this code base, so please forgive me if I'm making recommendations that go against the normal process. Typically, I'll recommend that we create a new issue to address "TODO" items since then those can get prioritized and taken care of by anyone else interested in making contributions to the code base.
If this seems like a good idea, please create an issue, and provide a link to it here so that I can review the details, then I'll consider this resolved. Otherwise, if it's common to leave "TODO" comments in the code and address them as they are found by later engineers making changes, then I won't worry about it.
Let me know your thoughts.
There was a problem hiding this comment.
Typically, I'll recommend that we create a new issue to address "TODO" items since then those can get prioritized and taken care of by anyone else interested in making contributions to the code base.
Yes, this would be a good idea for normal development. But since we are in the middle of a refactor an this code is going to be looked at again for sure, I wouldn't worry about opening issue and so, as it might just add unneeded overhead. We will need to look at all TODOs before finalizing the refactor.
| val nestedScrollModifier = Modifier.nestedScroll(rememberNestedScrollInteropConnection()) | ||
|
|
||
| if (mode == ItemViewMode.GRID) { | ||
| // TODO: Implement grid layout using LazyVerticalGrid and LazyVerticalGridScrollbar. |
There was a problem hiding this comment.
See the comment I left above about "TODO" comments in general. Please let me know your thoughts.
There was a problem hiding this comment.
I added that comment because the grid layout is not being used in this PR (the related items view will only be in the list format). I have implemented the grid layout in a separate PR.
| import org.schabi.newpipe.extractor.stream.StreamInfoItem | ||
| import org.schabi.newpipe.ui.theme.AppTheme | ||
|
|
||
| @OptIn(ExperimentalFoundationApi::class) |
There was a problem hiding this comment.
Is it possible to use an element that doesn't require the ExperimentalFoundationApi optin? I realize it may not be possible, but if possible, I believe it reduces the risk of new bugs being introduced if the underlying element is removed from the library or fundamentally changed. I realize that I'm not as familiar with this code base, so the process here may be different, so please don't hesitate to disregard this comment. I just wanted to weigh in and find out your thoughts.
There was a problem hiding this comment.
That annotation is needed because the combinedClickable modifier is currently experimental.
There was a problem hiding this comment.
I believe it reduces the risk of new bugs being introduced if the underlying element is removed from the library or fundamentally changed.
While this is true, I would argue that Jetpack Compose is still not fully stable and that using experimental things is the only way to do a few things (unless you want to reimplement them yourself, but then they're gonna be even more experimental ;-) ).
| SparseItemUtil.fetchStreamInfoAndSaveToDatabase( | ||
| context, stream.serviceId, stream.url | ||
| ) { info -> | ||
| // TODO: Use an AlertDialog composable instead. |
There was a problem hiding this comment.
See my comments above about "TODO" comments. In general, I thought it would make more sense to capture this in a new issue so that it can be prioritized and picked up by anyone interested in contributing to the code base. Please let me know your thoughts on this.
There was a problem hiding this comment.
I added that TODO because the code for the existing download dialog is fairly complex, and implementing it in Compose would be best done in a separate PR.
| fun RelatedItems(info: StreamInfo) { | ||
| val sharedPreferences = PreferenceManager.getDefaultSharedPreferences(LocalContext.current) | ||
| val key = stringResource(R.string.auto_queue_key) | ||
| // TODO: AndroidX DataStore might be a better option. |
There was a problem hiding this comment.
Unlike the other "TODO" comments I mentioned above, I think this one makes sense to leave in the code since it's a question to consider and isn't necessarily calling for a code change. I'm just commenting on this TODO to clarify that I don't think any action needs to be taken on this one, since I think it makes sense to consider at some future point when someone else is taking a look at this code and considering the question.
|
I left a few comments on the code changes. I'm pretty new to this code base, so please forgive me if my thoughts/suggestions are not in line with the normal process. I've been reviewing the contributions guidelines and seeking to stay within those rules. Otherwise, I'm going to grab the APK to test on my Android and report back if I find any issues with this build. |
|
I installed the APK for this branch on my Android and have been testing NewPipe by going through all the functionality I normally use for NewPipe as well as checked on a few pieces of functionality that I don't use every day but I believe are still important. Everything looks good. I'll keep an eye out if more changes come in so I can install the latest build to see if it's working. |
| SHOW_CHANNEL_DETAILS(R.string.show_channel_details, (fragment, item) -> { | ||
| final var activity = fragment.requireActivity(); | ||
| fetchUploaderUrlIfSparse(activity, item.getServiceId(), item.getUrl(), | ||
| item.getUploaderUrl(), url -> openChannelFragment(activity, item, url)); | ||
| }), |
There was a problem hiding this comment.
Why did you need to make this change? Did requireContext() return an invalid context or something like that?
|
|
||
| @Composable | ||
| fun ItemList( | ||
| items: List<InfoItem>, |
There was a problem hiding this comment.
It would be great if this were made data-source-agnostic. This would mean creating an UiInfoItem class that can be obtained from either an extractor's StreamInfoItem or a database's LocalInfoItem. This way we could create lists containing items from different sources, with a single UI implementation. What do you think?
There was a problem hiding this comment.
Yeah, that sounds like a good idea.
| val item = items[it] | ||
|
|
||
| if (item is StreamInfoItem) { | ||
| val isSelected = selectedStream == item |
There was a problem hiding this comment.
This should be selectedStream === item, right? Otherwise it's going to compare the fields, which might be slower (and is also not what we want).
| fun StreamListItem( | ||
| stream: StreamInfoItem, | ||
| showProgress: Boolean, | ||
| isSelected: Boolean, |
There was a problem hiding this comment.
This parameter should have a more descriptive name, such as isMenuOpen or something like that (feel free to suggest alternatives)
| val streamViewModel = viewModel<StreamViewModel>() | ||
| var progress by rememberSaveable { mutableLongStateOf(0L) } | ||
|
|
||
| LaunchedEffect(stream) { | ||
| progress = streamViewModel.getStreamState(stream)?.progressMillis ?: 0L | ||
| } |
There was a problem hiding this comment.
The view model itself should provide the progress in a StateFlow, there should be no need for this in the UI layer. Ideally I would suggest creating a @Singleton StreamStateRepository through which ALL state changes should be done (e.g. a video being watched) and that pushes events in a Flows of pairs (stream_id, stream_state). Every stream item visible in the UI would subscribe to the repository's Flow and if it receives an event with a matching stream_id updates the StateFlow, which in turn will trigger a UI recomposition of that stream item.
Btw this is the approach I thought of in a few minutes, but there may be better ones to achieve the same thing.
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Before:
After:
Fixes the following issue(s)
Relies on the following changes
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