Conversation
Regarding this, is it possible to inject a global size reduction CSS? announcement h1 would turn into android-h1 size * 0.8 for example. |
I do just that here: https://github.com/ReVanced/revanced-manager/pull/2948/changes#diff-fde2123669c1efe0c6b6171854031f931086b2311dd0b5dd4b3b3907de42b0f1R114 |
|
The problem isn't really reducing text size, it's more of actually |
|
Is it not possible to limit the height of views and hide the scroller? I am not really versed in android so pardon my inexperience, im just comparing it to web CSS |
|
Another thing that could increase consistency with https://revanced.app/announcements would be the search bar. Is it possible to implement that? |
app/src/main/java/app/revanced/manager/ui/viewmodel/AnnouncementsViewModel.kt
Outdated
Show resolved
Hide resolved
| withContext(Dispatchers.IO) { | ||
| reVancedAPI.getAnnouncements().getOrNull() | ||
| }?.let { |
There was a problem hiding this comment.
Instead of fetching the announcements like this, instead directly fetch using the tags filter: https://api.revanced.app/v4/announcements?tag=%E2%9C%A8%20ReVanced&tag=manager
There was a problem hiding this comment.
That'd require refetching every time new tags are applied. Sounds good?
There was a problem hiding this comment.
Do you mean when the API has new tags? Yep. The /tags endpoint can be fetched every time to ensure it's up to date. It's cached by CloudFlare. The announcements are too but when you filter based on tag, sometimes there's cache misses
There was a problem hiding this comment.
I meant refetching announcements every time it's filtered with a new set of tags. Fortunately Flows support debouncing so it won't be that big of a deal.
There was a problem hiding this comment.
Also, as far as I can tell, the website also doesn't make a request when filtering.
There was a problem hiding this comment.
Since most users will only be using this to view "revanced" and "manager" tagged announcements, I think it's fine but if it's possible the moment that the user changes the tag selection can the manager switch to fetching the entire announcements and only then filtering them locally? To avoid refetching every time a tag is changed, because the user is more likely to continue changing tags
There was a problem hiding this comment.
I meant refetching announcements every time it's filtered with a new set of tags. Fortunately Flows support debouncing so it won't be that big of a deal.
Works, but cant you keep a Map<Tags, Announcements> and extend the map with cursor & count parameters? This way when you filter, it uses the cached results.
There was a problem hiding this comment.
So cache for every tag combination?
Alignment should be looked into |
Ushie
changed the title
Heights don't take text line heights into account, since they're fixed. I tried to limit the |
I don't see why it's not possible, as long as all announcements are fetched. |
|
Website currently scopes search to the loaded announcements |
Perhaps it's better to revisit this once the API provides searching capabilities? |
|
Sure, I can add a query parameter next to the existing one for content filtering. One issue would be that the contents are html so just plain text filtering wouldn't work. |
I guess it doesn't account for action buttons when it centers the icon. I can fix that, but I think it's outside of this PR's scope. |
|
It is, it's infact a deliberate design choice, keep it as is |
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive announcement system to the ReVanced Manager app. It introduces functionality to fetch, display, filter, and track read status of announcements from the ReVancedAPI. The implementation includes a list view of all announcements with filtering by tags, a detailed view for individual announcements rendered with WebViews, and notification badges for unread announcements on the dashboard.
Changes:
- Added announcement data models, repository layer, and API integration for fetching announcements and tags
- Implemented AnnouncementsViewModel for managing announcement state, filtering by tags, and tracking read status
- Created UI screens for displaying announcement lists with tag-based filtering and individual announcement details using WebView
- Updated Material3 library to version 1.5.0-alpha14 to utilize TwoRowsTopAppBar for announcement detail screens
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Upgraded Material3 library to 1.5.0-alpha14 for new topbar APIs |
| app/src/main/res/values/strings.xml | Added string resources for announcement-related UI elements |
| app/src/main/java/app/revanced/manager/ui/viewmodel/DashboardViewModel.kt | Added announcement checking on startup and unread announcement notification logic |
| app/src/main/java/app/revanced/manager/ui/viewmodel/AnnouncementsViewModel.kt | New ViewModel managing announcement data, filtering, and read state tracking |
| app/src/main/java/app/revanced/manager/ui/screen/DashboardScreen.kt | Added announcement notification card and navigation button with unread badge |
| app/src/main/java/app/revanced/manager/ui/screen/AnnouncementsScreen.kt | New screen displaying announcement list with tag filtering and unread indicators |
| app/src/main/java/app/revanced/manager/ui/screen/AnnouncementScreen.kt | New screen displaying individual announcement content using WebView |
| app/src/main/java/app/revanced/manager/ui/model/navigation/Nav.kt | Added navigation destinations for announcements screens |
| app/src/main/java/app/revanced/manager/network/dto/ReVancedAnnouncementTag.kt | DTO for announcement tag data from API |
| app/src/main/java/app/revanced/manager/network/dto/ReVancedAnnouncement.kt | DTO for announcement data from API with Parcelable support |
| app/src/main/java/app/revanced/manager/network/api/ReVancedAPI.kt | Added API endpoints for fetching announcements and tags |
| app/src/main/java/app/revanced/manager/domain/repository/AnnouncementRepository.kt | Repository implementing caching for announcements and tags data |
| app/src/main/java/app/revanced/manager/domain/manager/PreferencesManager.kt | Added preferences for read announcements and selected announcement tags |
| app/src/main/java/app/revanced/manager/di/ViewModelModule.kt | Registered AnnouncementsViewModel in DI container |
| app/src/main/java/app/revanced/manager/di/RepositoryModule.kt | Registered AnnouncementRepository in DI container |
| app/src/main/java/app/revanced/manager/MainActivity.kt | Added routing for announcements screens |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </style> | ||
| </head> | ||
| <body> | ||
| ${announcement.content} |
There was a problem hiding this comment.
Using string interpolation directly in HTML content (${announcement.content}) without sanitization creates an XSS vulnerability. If the announcement content contains malicious script tags or event handlers, they will be executed in the WebView. Consider using proper HTML escaping/sanitization before inserting the content, or use loadDataWithBaseURL with a safe base URL and implement Content Security Policy headers.
There was a problem hiding this comment.
I think WebView disables JavaScript by default. @Ushie thoughts?
There was a problem hiding this comment.
Not sure, I'm pretty sure ReVanced doesn't want to disable JS, besides I don't know how anything can escalate here
There was a problem hiding this comment.
ReVanced makes a point out of this here with the "limited character count" section
https://revanced.app/announcements/12-revanced-announcements
It seems like clients are meant to display things as is
There was a problem hiding this comment.
JS can be enabled by default. We intend remote code execution here in case we want to do something funky with js on the announcement, simply enabling js should work
There was a problem hiding this comment.
It's probably a good idea to control whether or not JS is enabled based on the announcement itself
| val announcements = withContext(Dispatchers.IO) { | ||
| announcementRepository.getAnnouncements() | ||
| } ?: return | ||
|
|
||
| val readAnnouncements = prefs.readAnnouncements.get() | ||
| if (readAnnouncements.isEmpty()) { | ||
| val announcementIds = announcements.mapTo(mutableSetOf()) { it.id.toString() } | ||
| prefs.readAnnouncements.update(announcementIds) | ||
| return | ||
| } | ||
|
|
||
| unreadAnnouncement = announcements.firstOrNull { announcement -> | ||
| val hasRelevantTag = "revanced" in announcement.tags || | ||
| "manager" in announcement.tags | ||
|
|
||
| val isUnread = announcement.id.toString() !in readAnnouncements | ||
|
|
||
| hasRelevantTag && isUnread | ||
| } | ||
|
|
There was a problem hiding this comment.
The checkForAnnouncements function silently fails when the API call returns null or throws an exception, unlike checkForManagerUpdates which uses uiSafe for error handling. Users won't be notified if announcements fail to load. Consider wrapping the API call in uiSafe or adding proper error handling to inform users when announcements cannot be retrieved.
| val announcements = withContext(Dispatchers.IO) { | |
| announcementRepository.getAnnouncements() | |
| } ?: return | |
| val readAnnouncements = prefs.readAnnouncements.get() | |
| if (readAnnouncements.isEmpty()) { | |
| val announcementIds = announcements.mapTo(mutableSetOf()) { it.id.toString() } | |
| prefs.readAnnouncements.update(announcementIds) | |
| return | |
| } | |
| unreadAnnouncement = announcements.firstOrNull { announcement -> | |
| val hasRelevantTag = "revanced" in announcement.tags || | |
| "manager" in announcement.tags | |
| val isUnread = announcement.id.toString() !in readAnnouncements | |
| hasRelevantTag && isUnread | |
| } | |
| uiSafe(app, R.string.failed_to_check_updates, "Failed to check for announcements") { | |
| val announcements = withContext(Dispatchers.IO) { | |
| announcementRepository.getAnnouncements() | |
| } ?: throw IllegalStateException("Announcements could not be retrieved") | |
| val readAnnouncements = prefs.readAnnouncements.get() | |
| if (readAnnouncements.isEmpty()) { | |
| val announcementIds = announcements.mapTo(mutableSetOf()) { it.id.toString() } | |
| prefs.readAnnouncements.update(announcementIds) | |
| return@uiSafe | |
| } | |
| unreadAnnouncement = announcements.firstOrNull { announcement -> | |
| val hasRelevantTag = "revanced" in announcement.tags || | |
| "manager" in announcement.tags | |
| val isUnread = announcement.id.toString() !in readAnnouncements | |
| hasRelevantTag && isUnread | |
| } | |
| } |
app/src/main/java/app/revanced/manager/ui/viewmodel/AnnouncementsViewModel.kt
Show resolved
Hide resolved
| private fun observeSelectedTags() { | ||
| viewModelScope.launch { | ||
| snapshotFlow { selectedTags.toList() }.collect { _ -> | ||
| saveSelectedTags() | ||
| applyTagFilter() | ||
| } | ||
| } |
There was a problem hiding this comment.
The observeSelectedTags flow will trigger immediately on initialization, causing saveSelectedTags and applyTagFilter to be called before allAnnouncements is loaded. This could lead to applying filters on null data or unnecessary preference updates. The snapshotFlow will emit immediately with the current state. Consider adding a check to ensure data is loaded before applying filters, or restructure the initialization order.
| FilterChip( | ||
| selected = selected, | ||
| onClick = { | ||
| if (selected) { | ||
| selectedTags.remove(tag) | ||
| } else { | ||
| selectedTags.add(tag) | ||
| } | ||
| }, | ||
| label = { Text(tag) } | ||
| ) |
There was a problem hiding this comment.
The FilterChip used here lacks a leadingIcon with a check mark when selected, which is inconsistent with other FilterChip usage in the codebase. Other parts of the codebase use CheckedFilterChip (see PatchesSelectorScreen.kt:152, 158) which provides a check icon for selected chips. Consider using CheckedFilterChip instead of FilterChip for consistency.
There was a problem hiding this comment.
I remember I added a checkmark. @Ushie did you remove it accidentally when merging?
There was a problem hiding this comment.
I removed it intentionally to avoid layout shifts, but I didn't know that the checkmark was already used in other places of the app
There was a problem hiding this comment.
I think I added it in patch search screen when I worked on it.
app/src/main/java/app/revanced/manager/ui/viewmodel/AnnouncementsViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/revanced/manager/ui/screen/AnnouncementsScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/revanced/manager/ui/screen/AnnouncementsScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/revanced/manager/ui/viewmodel/AnnouncementsViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/revanced/manager/ui/viewmodel/DashboardViewModel.kt
Outdated
Show resolved
Hide resolved
…Screen.kt Co-authored-by: Ax333l <main@axelen.xyz>
…ose/announcements
|
@Axelen123 since moving to a Serializable data class instead of Parcel, I've faced this crash FATAL EXCEPTION: main |
|
Serializable would make sense if Navigation3 was used. Since this is Navigation2, I think parcelables are the only (sane) solution. |
…s with invalid tag selection (if a tag gets removed from API)
|
This is complete and ready for final review for merge |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Parcelize | ||
| @Serializable | ||
| data class ReVancedAnnouncement( | ||
| val id: Long, | ||
| val author: String, | ||
| val title: String, | ||
| val content: String, | ||
| val tags: List<String>, | ||
| val attachments: List<String>, | ||
| @SerialName("created_at") | ||
| val createdAt: LocalDateTime, | ||
| @SerialName("archived_at") | ||
| val archivedAt: LocalDateTime, | ||
| val level: Int, |
There was a problem hiding this comment.
ReVancedAnnouncement is marked @Parcelize but includes kotlinx.datetime.LocalDateTime fields (createdAt/archivedAt). Unless you add a custom Parceler/@TypeParceler for LocalDateTime, Parcelize typically can't generate Parcelable implementations for this type, which will break navigation where the whole announcement is placed into savedStateHandle. Consider passing only an ID (and re-fetching), or storing timestamps/ISO strings instead of LocalDateTime, or adding an explicit parceler for LocalDateTime.
|
|
||
| private fun loadData() { | ||
| viewModelScope.launch { | ||
| if (!network.isConnected()) return@launch |
There was a problem hiding this comment.
When there’s no network, loadData() returns early without updating any state. On the screen, announcements will remain null, so the UI can get stuck showing the loading spinner indefinitely. Consider setting announcements to an empty list (or an explicit error/offline state) before returning so the UI can render a proper empty/offline message.
| if (!network.isConnected()) return@launch | |
| if (!network.isConnected()) { | |
| allAnnouncements = emptyList() | |
| announcements = emptyList() | |
| tags = emptyList() | |
| return@launch | |
| } |
| private fun FilterBottomSheet( | ||
| onDismissRequest: () -> Unit, | ||
| tags: List<String>, | ||
| selectedTags: SnapshotStateList<String>, | ||
| showArchived: Boolean, | ||
| onShowArchivedChange: (Boolean) -> Unit, | ||
| onReset: () -> Unit, | ||
| onSave: () -> Unit | ||
| ) { |
There was a problem hiding this comment.
onSave is passed into FilterBottomSheet but never used, and tag selection is already being persisted from observeSelectedTags(). Either wire up an explicit “Save/Apply” action that calls onSave, or remove the unused parameter and the call site to avoid dead code and confusion.
| fun invalidateCache() { | ||
| cachedAnnouncements = null | ||
| cachedTags = null |
There was a problem hiding this comment.
invalidateCache() mutates cachedAnnouncements/cachedTags without taking the same mutex used by getAnnouncements()/getTags(). That can create a data race if invalidateCache() is ever called concurrently with a read. Consider either making invalidateCache() suspend and wrapping it in mutex.withLock { ... }, or using atomic/volatile state for the cache fields.
| fun invalidateCache() { | |
| cachedAnnouncements = null | |
| cachedTags = null | |
| suspend fun invalidateCache() { | |
| mutex.withLock { | |
| cachedAnnouncements = null | |
| cachedTags = null | |
| } |
| var showArchived by mutableStateOf(false) | ||
|
|
||
| private var savedTags = emptySet<String>() | ||
|
|
||
| init { | ||
| loadData() | ||
| observeSelectedTags() | ||
| } |
There was a problem hiding this comment.
showArchived affects filtering in applyTagFilter(), but changing showArchived never triggers applyTagFilter() again. As a result, toggling “Show archived” in the UI likely won’t update the list until the tag selection changes or the screen reloads. Consider observing showArchived (e.g., snapshotFlow { showArchived }.distinctUntilChanged()) or invoking applyTagFilter() whenever showArchived is updated.
| vm.announcements?.let { repositories -> | ||
| if (repositories.isEmpty()) { | ||
| item { |
There was a problem hiding this comment.
The lambda variable name repositories is misleading here since the list contains announcements. Renaming it (and related occurrences) to announcements would make the code easier to follow.
| ) : Preference<Set<Long>>(dataStore, default) { | ||
| private val key = stringSetPreferencesKey(key) | ||
|
|
||
| override fun Preferences.read() = this[key]?.mapTo(mutableSetOf()) { it.toLong() } ?: default |
There was a problem hiding this comment.
LongSetPreference parses stored strings via it.toLong(). If the DataStore value is ever corrupted (or the key is reused with non-numeric values), this will throw and break preference reads across the app. Consider using toLongOrNull() and ignoring invalid entries (or falling back to default) to make this preference resilient.
| override fun Preferences.read() = this[key]?.mapTo(mutableSetOf()) { it.toLong() } ?: default | |
| override fun Preferences.read(): Set<Long> { | |
| val stringSet = this[key] ?: return default | |
| val longSet = stringSet.mapNotNullTo(mutableSetOf()) { it.toLongOrNull() } | |
| // If there were stored values but none could be parsed, treat as corruption and fall back to default. | |
| return if (stringSet.isNotEmpty() && longSet.isEmpty()) default else longSet | |
| } |
4 participants
This adds announcement pages: a list of all announcements and a singular announcement. Announcement contents use WebViews to render the text. The list of all announcements was supposed to contain small previews, but truncating WebView texts is tricky, so I left some half-finished code commented out with a TODO label.
The app keeps track of which announcements are read to display unread indicators and notification cards when latest announcements arrive. On first launch, all current announcements are automatically marked as read.
It is also possible to filter announcements based on their tag. By default only ReVanced and manager tags are selected.
This also updates the material3 library to 1.5.0 alpha in order to utilize new flexible topbar APIs.
Here are some screenshots: