Conversation
13f3611 to
240a2b1
Compare
There was a problem hiding this comment.
Thank you so much for doing this. Apologies for the delay in review, I've been busy IRL.
When trying this I got the following error messages spamming my log and the widget wouldn't show anything:
Error in Glance App Widget
java.lang.IllegalArgumentException: RemoteViews for widget update exceeds maximum bitmap memory usage (used: 22240000, max: 15552000)
Once I commented out the bitmap images of the items it displayed.
So I've added some suggestions to make bitmaps optional to start with. Obviously it wont' fix the crash I saw but at least items without images will be rendered now.
I think overall it's a great start.
Before merging I think it's important to ensure all items are displayed. If it's easier, just disable images to start with maybe? Up to you.
And it needs to be resizable, because I think that is an overall guideline requirement by Google these days?
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| fun getCurrentWidgetFeedListItems(): Flow<PagingData<FeedListItem>> = | ||
| combine( | ||
| currentWidgetFeedAndTag, | ||
| feedListFilter, | ||
| search, | ||
| ) { feedAndTag, feedListFilter, search -> | ||
| val (feedId, tag) = feedAndTag | ||
| FeedListArgs( | ||
| feedId = feedId, | ||
| tag = tag, | ||
| minReadTime = Instant.EPOCH, | ||
| newestFirst = true, | ||
| filter = feedListFilter, | ||
| search = search, | ||
| ) | ||
| }.flatMapLatest { | ||
| feedItemStore.getPagedFeedItemsRaw( | ||
| feedId = it.feedId, | ||
| tag = it.tag, | ||
| minReadTime = it.minReadTime, | ||
| newestFirst = it.newestFirst, | ||
| filter = it.filter, | ||
| search = it.search, | ||
| ) | ||
| } | ||
|
|
||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| fun getWidgetFeedListItems(): Flow<PagingData<FeedListItem>> = | ||
| combine( | ||
| currentFeedAndTag, | ||
| minReadTime, | ||
| currentSorting, | ||
| feedListFilter, | ||
| search, | ||
| ) { feedAndTag, minReadTime, currentSorting, feedListFilter, search -> | ||
| val (feedId, tag) = feedAndTag | ||
| FeedListArgs( | ||
| feedId = feedId, | ||
| tag = tag, | ||
| minReadTime = | ||
| when (feedId) { | ||
| ID_SAVED_ARTICLES -> Instant.EPOCH | ||
| else -> minReadTime | ||
| }, | ||
| newestFirst = currentSorting == SortingOptions.NEWEST_FIRST, | ||
| filter = feedListFilter, | ||
| search = search, | ||
| ) | ||
| }.flatMapLatest { | ||
| feedItemStore.getPagedFeedItemsRaw( | ||
| feedId = it.feedId, | ||
| tag = it.tag, | ||
| minReadTime = it.minReadTime, | ||
| newestFirst = it.newestFirst, | ||
| filter = it.filter, | ||
| search = it.search, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Correct me if I'm wrong, but there is no search functionality for the widget right?
So no need to include the search query?
| @OptIn(ExperimentalCoroutinesApi::class) | |
| fun getCurrentWidgetFeedListItems(): Flow<PagingData<FeedListItem>> = | |
| combine( | |
| currentWidgetFeedAndTag, | |
| feedListFilter, | |
| search, | |
| ) { feedAndTag, feedListFilter, search -> | |
| val (feedId, tag) = feedAndTag | |
| FeedListArgs( | |
| feedId = feedId, | |
| tag = tag, | |
| minReadTime = Instant.EPOCH, | |
| newestFirst = true, | |
| filter = feedListFilter, | |
| search = search, | |
| ) | |
| }.flatMapLatest { | |
| feedItemStore.getPagedFeedItemsRaw( | |
| feedId = it.feedId, | |
| tag = it.tag, | |
| minReadTime = it.minReadTime, | |
| newestFirst = it.newestFirst, | |
| filter = it.filter, | |
| search = it.search, | |
| ) | |
| } | |
| @OptIn(ExperimentalCoroutinesApi::class) | |
| fun getWidgetFeedListItems(): Flow<PagingData<FeedListItem>> = | |
| combine( | |
| currentFeedAndTag, | |
| minReadTime, | |
| currentSorting, | |
| feedListFilter, | |
| search, | |
| ) { feedAndTag, minReadTime, currentSorting, feedListFilter, search -> | |
| val (feedId, tag) = feedAndTag | |
| FeedListArgs( | |
| feedId = feedId, | |
| tag = tag, | |
| minReadTime = | |
| when (feedId) { | |
| ID_SAVED_ARTICLES -> Instant.EPOCH | |
| else -> minReadTime | |
| }, | |
| newestFirst = currentSorting == SortingOptions.NEWEST_FIRST, | |
| filter = feedListFilter, | |
| search = search, | |
| ) | |
| }.flatMapLatest { | |
| feedItemStore.getPagedFeedItemsRaw( | |
| feedId = it.feedId, | |
| tag = it.tag, | |
| minReadTime = it.minReadTime, | |
| newestFirst = it.newestFirst, | |
| filter = it.filter, | |
| search = it.search, | |
| ) | |
| } | |
| @OptIn(ExperimentalCoroutinesApi::class) | |
| fun getCurrentWidgetFeedListItems(): Flow<PagingData<FeedListItem>> = | |
| combine( | |
| currentWidgetFeedAndTag, | |
| feedListFilter, | |
| ) { feedAndTag, feedListFilter -> | |
| val (feedId, tag) = feedAndTag | |
| FeedListArgs( | |
| feedId = feedId, | |
| tag = tag, | |
| minReadTime = Instant.EPOCH, | |
| newestFirst = true, | |
| filter = feedListFilter, | |
| search = "", | |
| ) | |
| }.flatMapLatest { | |
| feedItemStore.getPagedFeedItemsRaw( | |
| feedId = it.feedId, | |
| tag = it.tag, | |
| minReadTime = it.minReadTime, | |
| newestFirst = it.newestFirst, | |
| filter = it.filter, | |
| search = it.search, | |
| ) | |
| } | |
| @OptIn(ExperimentalCoroutinesApi::class) | |
| fun getWidgetFeedListItems(): Flow<PagingData<FeedListItem>> = | |
| combine( | |
| currentFeedAndTag, | |
| minReadTime, | |
| currentSorting, | |
| feedListFilter, | |
| ) { feedAndTag, minReadTime, currentSorting, feedListFilter -> | |
| val (feedId, tag) = feedAndTag | |
| FeedListArgs( | |
| feedId = feedId, | |
| tag = tag, | |
| minReadTime = | |
| when (feedId) { | |
| ID_SAVED_ARTICLES -> Instant.EPOCH | |
| else -> minReadTime | |
| }, | |
| newestFirst = currentSorting == SortingOptions.NEWEST_FIRST, | |
| filter = feedListFilter, | |
| search = "", | |
| ) | |
| }.flatMapLatest { | |
| feedItemStore.getPagedFeedItemsRaw( | |
| feedId = it.feedId, | |
| tag = it.tag, | |
| minReadTime = it.minReadTime, | |
| newestFirst = it.newestFirst, | |
| filter = it.filter, | |
| search = it.search, | |
| ) | |
| } | |
|
@spacecowboy thanks so much for taking a look! I've applied your patch feedback. For the memory crash, I've tried a couple things:
That said, I haven't run into the crash in my local testing. Could you either see if you can still reproduce, or perhaps pass along a repro OPML export? I've kept the patch applying your feedback separate for now, assuming we will squash and merge eventually |
The feed I tried with was Feeder's default feed for the app's changelog which automatically gets added on fresh installs: https://news.nononsenseapps.com/
Yes it will get squashed on merge. You forgot to remove the I will try the new version soon |
|
Whoops, looks like Screen_recording_20260301_151249.mp4 |
|
I'll take another look. @MatthewTighe do you feel it is ready now from your perspective and testing? |
|
It seems to work nicely now, and the resizing works smoothly 👍 One thing I noted is that the margins are un-even. There is a margin on the left but not on the right. But this is not a blocker by any means. This can be fixed later. |
|
Ha, I am not the most skilled when it comes to UX 😅 I often have to rely on the professionals for that piece of sign-off. Happy to take a look if you'd like. If it's not a blocker, is there anything else you need from me before this is merged? |
|
Then I think we should just merge. Not letting perfect be the enemy of good. Thanks for the contribution @MatthewTighe ! |
|
🙌 awesome, thanks for helping me get it over the finish line! looking forward to using it in the wild 😁 |
This PR adds a newsfeed widget using Jetpack Glance for #384. It:
There are a couple areas that could potentially use more polish, but I wanted to get this up to at least start a conversation as to whether it is worth waiting for them.
Areas of potential improvement:
LocalContext, it would need to provided by something like Robolectric, which I did not think was worth importing for that alone. there's otherwise very little logic here, but happy to eventually add tests for the Repository etc if preferredLMK your thoughts on whether any of these are worth baking longer - happy to keep trucking on this but it's obviously been slow going. Assume we will just squash these commits as well?
Screen_recording_20260106_222913.webm
Fixes #384