[Android] "What’s New" promo message: Make the ModalSurfaceActivity handle any remote message we support #7368
Conversation
…pport modal display.
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
anikiki
changed the title
…dling and visibility logic.
…type differentiation.
anikiki
changed the title
There was a problem hiding this comment.
Pull request overview
This PR extends the ModalSurfaceActivity to support standard remote messages in addition to the previously supported cards list messages. The changes introduce a common dismiss listener interface, update the view model to use sealed classes for different view states, and add RemoteMessageView to the modal surface layout.
Key changes:
- Created
RemoteMessageDismissListenerinterface to unify dismissal handling across message types - Refactored
ModalSurfaceViewModel.ViewStatefrom a data class to a sealed class withCardsListandMessagevariants - Removed Surface.NEW_TAB_PAGE filtering from
RemoteMessageViewModelto allow modal surface usage
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| RemoteMessageDismissListener.kt | Introduces common interface for message dismissal listeners |
| ModalSurfaceViewModel.kt | Refactors ViewState to sealed class and adds Message state handling |
| ModalSurfaceActivity.kt | Implements RemoteMessageDismissListener and adds RemoteMessageView support with toolbar |
| CardsListRemoteMessageView.kt | Updates listener type to use common RemoteMessageDismissListener interface |
| RemoteMessageView.kt | Adds dismiss listener support and triggers callback on message dismissal |
| RemoteMessageViewModel.kt | Removes Surface.NEW_TAB_PAGE filter and adds dismissed state tracking |
| activity_modal_surface.xml | Changes root to LinearLayout and adds RemoteMessageView alongside CardsListRemoteMessageView |
| ModalSurfaceViewModelTest.kt | Updates tests for sealed class ViewState structure |
| RemoteMessageViewModelTest.kt | Updates test to verify modal surface messages are displayed |
Comments suppressed due to low confidence (1)
remote-messaging/remote-messaging-impl/src/main/java/com/duckduckgo/remote/messaging/impl/newtab/RemoteMessageViewModel.kt:121
- The flow has redundant dispatcher switching with flowOn(dispatchers.io()) at line 102 and flowOn(dispatchers.main()) at line 120. Since the outer coroutine is already launched on dispatchers.io(), and the withContext at line 104 already switches to dispatchers.main() for the critical section, the flowOn operators are unnecessary and add confusion. The withContext call is sufficient to handle the dispatcher switching needed for this flow.
viewModelScope.launch(dispatchers.io()) {
remoteMessagingModel.getActiveMessages()
.flowOn(dispatchers.io())
.onEach { message ->
withContext(dispatchers.main()) {
val newMessage = message?.id != lastRemoteMessageSeen?.id
val dismissed = newMessage && message == null && lastRemoteMessageSeen != null
if (newMessage) {
lastRemoteMessageSeen = message
}
_viewState.emit(
viewState.value.copy(
message = message,
newMessage = newMessage,
dismissed = dismissed,
),
)
}
}
.flowOn(dispatchers.main())
.launchIn(viewModelScope)
| <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent"> | ||
| android:layout_height="match_parent" | ||
| android:orientation="vertical"> |
There was a problem hiding this comment.
Changing the root layout from FrameLayout to LinearLayout with orientation=vertical adds unnecessary layout complexity. Since the toolbar is initially gone and the FrameLayout takes match_parent for height, a FrameLayout root with the toolbar as the first child would achieve the same result with simpler layout hierarchy and potentially better performance.
| private fun setupToolbar() { | ||
| setSupportActionBar(binding.includeToolbar.toolbar) | ||
| supportActionBar?.setDisplayHomeAsUpEnabled(true) | ||
| } |
There was a problem hiding this comment.
The toolbar is set up unconditionally in onCreate, including setting the action bar and enabling the home/up button, but the toolbar is only shown for ViewState.Message. This means the action bar is configured even when it's not displayed (for CardsList views). While not causing a runtime error, this could lead to unexpected behavior or resource waste. Consider conditionally setting up the action bar only when the toolbar will be visible, or at least document why this setup is necessary even when hidden.
| binding.cardsListRemoteMessageView.show() | ||
| } | ||
| is ModalSurfaceViewModel.ViewState.Message -> { | ||
| // RemoteMessageView fetches the active message from RemoteMessageModel, so no messageId is needed |
There was a problem hiding this comment.
The comment states that RemoteMessageView fetches the active message from RemoteMessageModel, but this is misleading. RemoteMessageView uses RemoteMessageViewModel which fetches from RemoteMessageModel.getActiveMessages(). More importantly, the comment doesn't explain that the messageId from the activity params is not used because the view relies on the ViewModel to fetch whatever is currently active, which could potentially differ from the intended message if the active message changes between when the modal was triggered and when the view loads.
| viewModelScope.launch(dispatchers.io()) { | ||
| remoteMessagingModel.getActiveMessages() | ||
| .map { message -> | ||
| if (message?.surfaces?.contains(Surface.NEW_TAB_PAGE) == true) message else null | ||
| } | ||
| .flowOn(dispatchers.io()) |
There was a problem hiding this comment.
The removal of Surface filtering causes RemoteMessageView to display messages regardless of their intended surface. RemoteMessageView is used in two contexts: (1) New Tab Page via RemoteMessageNewTabSectionPlugin where it should only show Surface.NEW_TAB_PAGE messages, and (2) Modal Surface via ModalSurfaceActivity where it should show Surface.MODAL messages. Without filtering, a MODAL surface message could appear on the new tab page, or vice versa. The ViewModel needs to accept a surface parameter or the filtering needs to be applied based on the usage context to ensure messages are only shown on their intended surfaces.
| if (viewState.dismissed) { | ||
| listener?.onDismiss() |
There was a problem hiding this comment.
The dismiss listener is called when viewState.dismissed is true, but there's no documentation explaining when this happens or what the expected behavior is for consumers. Consider adding a KDoc comment explaining that the listener is invoked when a previously displayed message is removed from the active messages flow, so implementers understand when to expect this callback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| .map { message -> | ||
| if (message?.surfaces?.contains(Surface.NEW_TAB_PAGE) == true) message else null | ||
| } | ||
| .flowOn(dispatchers.io()) |
There was a problem hiding this comment.
Bug: Removing surface filter shows modal messages on wrong surface
The Surface.NEW_TAB_PAGE filter was removed from RemoteMessageViewModel, but this view model is still used by RemoteMessageNewTabSectionPlugin to display messages on the New Tab Page. When a message is active with only Surface.MODAL in its surfaces list, it will now incorrectly appear on the New Tab Page. The RemoteMessageNewTabSectionPlugin.isUserEnabled() method also has no surface filtering—it just checks if any message exists. This change was needed to support modal messages, but it breaks the New Tab Page behavior by showing messages that aren't intended for that surface.
Please tell me if this was useful or not with a 👍 or 👎.
| viewModelScope.launch { | ||
| _command.send(Command.DismissMessage) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Standard messages not dismissed on back press in modal
When the user presses back on a standard message (ViewState.Message) in the modal surface, the code only sends Command.DismissMessage to close the activity, but doesn't actually dismiss the message in the repository. In contrast, for ViewState.CardsList, cardsListPixelHelper.dismissCardsListMessage() is called which both fires a pixel AND calls remoteMessagingRepository.dismissMessage(). This inconsistency means standard messages will persist and reappear every time the modal surface is opened, while cards list messages are properly dismissed. The standard message handling needs to call remoteMessagingRepository.dismissMessage() or equivalent.
Please tell me if this was useful or not with a 👍 or 👎.
|
Closing this PR as we won't support other remote messages on MODAL yet. |
1 participant
Task/Issue URL: https://app.asana.com/1/137249556945/project/1200581511062568/task/1212304890091987?focus=true
Description
Added support for standard remote messages in the modal surface activity, which previously only supported cards list messages. This change allows both types of messages to be displayed in a modal context.
Key changes:
RemoteMessageDismissListenerinterface to handle message dismissal for both message typesRemoteMessageViewto support the dismiss listenerModalSurfaceViewModelto handle non-cards list messagesRemoteMessageViewto the modal surface layoutSteps to test this PR
Standard Remote Messages in Modal Surface
Dismiss Behavior
NO UI changes
Note
Modal surface can now display standard remote messages in addition to cards list, using a shared dismiss listener with updated view models and layout.
ModalSurfaceActivity) now renders eitherCardsListRemoteMessageVieworRemoteMessageView, toggling toolbar visibility accordingly.activity_modal_surface.xmlupdated to include toolbar andRemoteMessageViewcontainer.RemoteMessageView: addsRemoteMessageDismissListenerand emitsonDismisswhen message is dismissed; wiring for commands unchanged.CardsListRemoteMessageView: replaces local listener with sharedRemoteMessageDismissListenerand removes inner interface.RemoteMessageDismissListenerinterface shared across views.ModalSurfaceViewModel: refactorsViewStateto sealed classes (CardsList,Message); handles back navigation (including cards list dismiss pixel) and dismissal uniformly.RemoteMessageViewModel: addsdismissedflag inViewState; simplifies active message stream (no surface filtering) and tracks new/dismissed transitions.ViewState, dismiss behavior, and support for modal-surface messages inRemoteMessageViewModel.Written by Cursor Bugbot for commit 4aa7c92. This will update automatically on new commits. Configure here.