[Android] "What’s New" promo message: Add the new UI elements #7256
Conversation
anikiki
changed the title
anikiki
force-pushed
the
feature/ana/android_whats_new_promo_message_add_the_new_ui_elements
branch
3 times, most recently
from
a3a769e to
92e3971
Compare
| <activity | ||
| android:name="com.duckduckgo.remote.messaging.impl.ui.ModalSurfaceActivity" | ||
| android:exported="false" /> | ||
|
|
There was a problem hiding this comment.
This is the new activity that will show the remote message on a "MODAL" surface (full screen).
| override fun onResume(owner: LifecycleOwner) { | ||
| super.onResume(owner) | ||
| appCoroutineScope.launch { | ||
| evaluate() |
There was a problem hiding this comment.
This evaluates if a remote message needs to be shown full screen. This is not the full implementation, more conditions will be added later. So far this is guarded by a FF and a check that onboarding is completed is made.
There was a problem hiding this comment.
Thanks for the comment here.
Is your plan to keep this class in :app module? because dependencies you need or your original plan?
Something to consider is if we want this to be RMF focused or a generic evaluator for fullscreen prompts that coordinates the priorities. Depending on that, it's ok to keep it here, or I would consider moving it into rmf module.
There was a problem hiding this comment.
The main issue why this is still in the app module is detecting that onboarding has completed.
app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt
Show resolved
Hide resolved
| logcat(INFO) { "RMF: messages parsed $messages ${Thread.currentThread().name}" } | ||
|
|
||
| val updatedMessages: List<RemoteMessage> = | ||
| if (!remoteMessagingFeatureToggles.self().isEnabled() || !remoteMessagingFeatureToggles.remoteMessageModalSurface().isEnabled()) { |
There was a problem hiding this comment.
Removed the TODO and added the FF. If the FF is not enabled, we keep only the remote messages that should be shown on the new tab page.
| is PlayStore -> Command.LaunchPlayStore(this.value) | ||
| is Url -> Command.SubmitUrl(this.value) | ||
| is UrlInContext -> Command.SubmitUrlInContext(this.value) | ||
| is UrlInContext -> Command.SubmitUrl(this.value) |
There was a problem hiding this comment.
UrlInContext is handled in the same way as Url for remote messages that are not shown on MODAL surface.
| import javax.inject.Inject | ||
|
|
||
| @InjectWith(ViewScope::class) | ||
| class CardsListRemoteMessageView @JvmOverloads constructor( |
There was a problem hiding this comment.
This is a custom view that displays a cards list.
| class RealCommandActionMapper @Inject constructor( | ||
| private val surveyParameterManager: SurveyParameterManager, | ||
| ) : CommandActionMapper { | ||
| override suspend fun asCommand(action: Action): Command { |
There was a problem hiding this comment.
I'm not a fan of this approach as we already have a duplication of this. Will leave it as is for now, but think of a different approach in a new PR.
|
|
||
| @InjectWith(ActivityScope::class) | ||
| @ContributeToActivityStarter(ModalSurfaceActivityFromMessageId::class) | ||
| class ModalSurfaceActivity : DuckDuckGoActivity(), CardsListRemoteMessageView.CardsListRemoteMessageListener { |
There was a problem hiding this comment.
This is the activity showing a remote message with MODAL surface.
| private fun render(viewState: ModalSurfaceViewModel.ViewState?) { | ||
| if (viewState == null) return | ||
|
|
||
| if (viewState.showCardsListView) { |
There was a problem hiding this comment.
For now this only shows a cards list view as this PR is to verify this UI.
However, in the future this will be generic and show any remote message that has the MODAL surface.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...src/main/java/com/duckduckgo/app/browser/remotemessage/RemoteMessageModalSurfaceEvaluator.kt
Show resolved
Hide resolved
| private fun render(viewState: CardsListRemoteMessageViewModel.ViewState?) { | ||
| viewState?.cardsLists?.let { | ||
| modalSurfaceAdapter.submitList(it.listItems) | ||
| binding.headerImage.setImageResource(it.placeholder.drawable(true)) |
There was a problem hiding this comment.
Bug: Hardcoded light mode ignores dark theme for placeholders
The Placeholder.drawable() function is called with a hardcoded true (light mode) instead of dynamically checking the actual theme. The drawable() function uses the isLightModeEnabled parameter to return different drawables for placeholders like VISUAL_DESIGN_UPDATE (light vs dark artwork). Despite the PR showing both light and dark mode UI screenshots, the appBuildConfig is injected but appTheme.isLightModeEnabled() is not used, causing dark mode users to always see light mode images.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
...-messaging-impl/src/main/java/com/duckduckgo/remote/messaging/impl/ui/ModalSurfaceAdapter.kt
Show resolved
Hide resolved
| ) | ||
| intent?.flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_SINGLE_TOP | ||
| applicationContext.startActivity(intent) | ||
| } |
There was a problem hiding this comment.
Bug: Modal activity shows blank screen for unsupported message types
There's a mismatch between the evaluator and the modal activity logic. The RemoteMessageModalSurfaceEvaluator launches ModalSurfaceActivity for any message with Surface.MODAL, regardless of its messageType. However, ModalSurfaceViewModel.onInitialise() only sets the view state when messageType == CARDS_LIST. For any other message type (e.g., SMALL, MEDIUM), the view state remains null, rendering a blank activity with no content and no way to dismiss except pressing the back button. The evaluator needs to also check that the message type is supported before launching the activity.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
Sending Review, moving into Testing.
| android:exported="false" | ||
| android:label="@string/appName" /> | ||
|
|
||
| <activity |
There was a problem hiding this comment.
We can declare this activity inside remote messaging impl module manifest.
| val messageId = activityParams?.messageId ?: return | ||
| val messageType = activityParams.messageType | ||
|
|
||
| if (messageType == Content.MessageType.CARDS_LIST) { |
There was a problem hiding this comment.
Since we have some logic around valid message types, I'd include some logic to handle an invalid type (not necessary here). Ideally, we would close the activity if type invalid, crash with illegalState, or handling before opening the activity.
| val viewState: Flow<ViewState?> = _viewState.asStateFlow() | ||
|
|
||
| fun onInitialise(activityParams: ModalSurfaceActivityFromMessageId?) { | ||
| val messageId = activityParams?.messageId ?: return |
There was a problem hiding this comment.
Kind of related to valid message types, I'd be more explicit with the expectations here. If this is null, activity won't render anything, which will be unexpected. So I would handle that state somehow or crash it if illegal state. Follow your preference.
| lifecycleScope.launch { | ||
| viewModel.viewState | ||
| .flowWithLifecycle(lifecycle, Lifecycle.State.STARTED) | ||
| .collectLatest { render(it) } |
There was a problem hiding this comment.
Just checking if this is intended (usage of collectLatest) which on any new emission, will cancel the previous suspend action (if there's a suspend point), and apply latest state.
| lifecycleScope.launch { | ||
| viewModel.commands | ||
| .flowWithLifecycle(lifecycle, Lifecycle.State.STARTED) | ||
| .collectLatest { processCommand(it) } |
There was a problem hiding this comment.
Ditto. just checking if is collectLatest intended here? right now doesn't seem problematic, unsure if we will have more commands later which can change the assumption.
|
|
||
| findViewTreeLifecycleOwner()?.lifecycle?.addObserver(viewModel) | ||
|
|
||
| val coroutineScope = findViewTreeLifecycleOwner()?.lifecycleScope |
There was a problem hiding this comment.
if this returns null best to return or similar, just based on the !! later.
|
|
||
| viewModelScope.launch(dispatchers.io()) { | ||
| val message = remoteMessagingRepository.getMessageById(messageId) | ||
| val cardsList = message?.content as? Content.CardsList |
There was a problem hiding this comment.
should handle the case where this is null and we are here?
| class RealCommandActionMapper @Inject constructor( | ||
| private val surveyParameterManager: SurveyParameterManager, | ||
| ) : CommandActionMapper { | ||
| override suspend fun asCommand(action: Action): Command { |
| android:id="@+id/actionButton" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="24dp" |
There was a problem hiding this comment.
NIT: use dimen references
| xmlns:tools="http://schemas.android.com/tools" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="24dp" |
There was a problem hiding this comment.
NIT: use dimen references (keylines)
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
Overall testing works fine, but found 2 issues:
-
once, after opening the app, it showed the what's new, and later it showed the new input widget screen on top of it. Pressing back on the input widget took me to the what's new. I think there's a race condition here between what's new (activity) vs widget input (activity)
-
What's new is rendered first as blank, and items take 1 second to appear. So loading is pretty async and not great.
anikiki
force-pushed
the
feature/ana/android_whats_new_promo_message_add_the_new_ui_elements
branch
from
ff64a45 to
1957d95
Compare
|
|
||
| intent.flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_SINGLE_TOP | ||
| applicationContext.startActivity(intent) | ||
| } |
There was a problem hiding this comment.
Bug: Modal message displayed repeatedly on every app resume
The RemoteMessageModalSurfaceEvaluator.onResume() launches the modal activity on every app resume when a modal message exists, but neither the evaluator nor CardsListRemoteMessageViewModel.onCloseButtonClicked() actually dismisses or marks the message as shown in the repository. The modal will display every time the user foregrounds the app until the message naturally expires. The existing RemoteMessageViewModel properly calls remoteMessagingModel.onMessageDismissed(), but the new code only sends a DismissMessage command that closes the activity without updating message state. The TODO comment on line 83 acknowledges this issue.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
| if (messageType == Content.MessageType.CARDS_LIST) { | ||
| _viewState.value = ViewState(messageId = messageId, showCardsListView = true) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Non-CardsList modal messages show blank activity screen
When a message with MODAL surface but a non-CARDS_LIST messageType is configured, ModalSurfaceViewModel.onInitialise does nothing after the early return check passes - the viewState remains null because only CARDS_LIST type sets it. The RemoteMessageModalSurfaceEvaluator launches the activity for any message with Surface.MODAL, and the RemoteMessagingConfigJsonMapper keeps all message types when the feature flag is enabled. This results in a blank activity screen with no way to dismiss it except pressing back. The method could send a dismiss command when an unsupported message type is encountered.
Please tell me if this was useful or not with a 👍 or 👎.
…l messages is now dependent on this toggle within the remote messaging module.
…ssagingFeatureToggles.
… and consistency.
…arity; improve intent handling in RemoteMessageModalSurfaceEvaluator.
…UI setup in CardsListRemoteMessageView.
anikiki
force-pushed
the
feature/ana/android_whats_new_promo_message_add_the_new_ui_elements
branch
from
f56fe00 to
a2f7db4
Compare
| viewModelScope.launch { | ||
| _command.send(Command.DismissMessage) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Modal message never dismissed, reappears on every resume
The CardsListRemoteMessageViewModel sends dismiss commands to the UI but never calls remoteMessagingRepository.dismissMessage(messageId) to mark the message as dismissed in the database. Since RemoteMessageModalSurfaceEvaluator.evaluate() calls remoteMessagingRepository.message() on every onResume, the modal will keep reappearing each time the user brings the app to foreground until the message is properly dismissed. Compare with how other message views use RealRemoteMessageModel.onMessageDismissed() which calls the repository's dismiss method.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
| conflatedCommandJob += viewModel.commands | ||
| .onEach { processCommand(it) } | ||
| .launchIn(coroutineScope!!) | ||
| } |
There was a problem hiding this comment.
Bug: View initializes before messageId set, content never loads
The CardsListRemoteMessageView.onAttachedToWindow() is called during layout inflation (when setContentView runs), before the Activity has a chance to set messageId. At this point messageId is null (default), so viewModel.init(null) returns early without loading content. When the Activity later sets messageId in render() and calls show(), init() is not called again since onAttachedToWindow() only fires once. The result is the modal displays with no content - empty header, title, and cards list.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1200581511062568/task/1211783737656697?focus=true
Description
Adds support for displaying remote messages in a modal surface. This implementation introduces a new activity that can display card list messages in a full-screen modal format, allowing users to interact with various card items.
The PR includes:
ModalSurfaceActivityfor displaying remote messages on aMODALsurface (full screen).CardsListRemoteMessageViewto render card list based content based in what comes from the server.RemoteMessageModalSurfaceEvaluatorfor evaluating if there is any remote message that should be displayed. Conditions to be met: feature flag enabled (remoteMessagingandremoteMessaging.remoteMessageModalSurface), onboarding finished, an active remote message with surface=MODAL is present).remoteMessagingandremoteMessaging.remoteMessageModalSurface).Steps to test this PR
Modal Surface Display and Integration
Various tests with the flags enabled / disabled and messages on other surfaces
UI changes
Note
Introduces a full-screen modal surface to display remote messages (cards list), wired via new activity/view/viewmodels, feature toggles, repository/mappers updates, and evaluator, with tests and resources.
ModalSurfaceActivity,CardsListRemoteMessageView,ModalSurfaceAdapter, andModalSurfaceViewModelwith layouts (activity_modal_surface,view_cards_list_remote_message,view_remote_message_entry) and colors; register inAndroidManifest.RemoteMessageModalSurfaceEvaluatorto launch modal when flags enabled, onboarding complete, and aSurface.MODALmessage exists.remoteMessageModalSurfacetoggle toremoteMessagingfeature (RemoteMessagingFeatureToggles) and remove fromAndroidBrowserConfigFeature.Content.CardsList.UrlInContextaction mapper.RemoteMessagingConfigJsonMapperto filter messages by surface based on toggles; keep MODAL messages only when enabled.surfacesfrom source inAppRemoteMessagingRepository(no longer forced toNEW_TAB_PAGE).UrlInContextby submitting URLs likeUrlinRemoteMessageViewModel.remote-messaging-impl.Written by Cursor Bugbot for commit a2f7db4. This will update automatically on new commits. Configure here.