feat: notification tray limit — setting plus both consumers (#35) - #112
Merged
Merged
Conversation
The bell tray's size was configurable on the web and unreachable on Android, where two independent constants decided it instead: the notifications list paged by a hard-coded 20 and the push-poll collapsed anything over five items into a single tray summary. Expose the preference and have both consumers respect it: - Settings gains a "Notification tray limit" number row under View preferences, which is where the web files it — /help/settings lists it there alongside Messages per page, and that page's Notifications section points back with "up to your Notification tray limit (see View Preferences above)". The range is the documented one, 10 to 40 with a default of 20 (/help/settings, echoed by /help/api/notifications' "clamped to 10-40"), enforced client-side so an out-of-range entry never costs a request. - :feature:notifications sizes both its list pages and the poll's fetch by the preference, and the poll passes the same number to the tray raiser as its group cap, replacing SystemNotificationPoster's fixed five. The value reaches :feature:notifications through NotificationTrayLimitStore in :core:network, following the ViewingPreferenceStore pattern #19 established for exactly this problem: no feature module here depends on another, and :feature:profile owns SettingsRepository. The store caches for the process and DefaultSettingsRepository publishes into it on every read and write, so a limit changed in Settings takes effect on the next refresh rather than after a restart. That leaves two narrow accessors in :core:network beside the full SettingsRepository, which adds a reader to the ownership tangle issue #104 already tracks. Closes #35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #35. Part of epic #31.
Placement and range are documented, not invented
The web files Notification tray limit under View preferences, not under a notifications group.
/help/settingslists it there alongside Messages per page, and its own Notifications sectionpoints back: "Open it to see your most recent notifications, up to your Notification tray limit
(see View Preferences above)". So it went into
ViewPreferencesGroup, directly under Messages perpage.
Range 10–40, default 20.
/help/settings: "The default is 20 and you can set any value from 10to 40." Corroborated by
/help/api/notifications("default 20, clamped to 10–40") and by the liveGET /api/uservalue of20.Both consumers honour it — this is the part that makes the issue worth doing
DefaultNotificationsRepositoryreplacesPaginationDto.DEFAULT_LIMITwith theaccount's limit for
refresh,loadMoreandfetchLatest. The endpoint's ownlimitaccepts1–50, which contains 10–40, so the preference is sent verbatim.
SystemNotificationRaiser.postnow takesmaxIndividual, supplied byNotificationPollRunnerfrom the account's limit (cached by then, so no second request). Thehard-coded
SystemNotificationPoster.MAX_INDIVIDUAL = 5is gone, replaced by a purecollapsesToSummary(count, maxIndividual)so the grouping rule is assertable without an Androidnotification manager.
How the value crosses modules
Follows the #19 pattern exactly: a second narrow accessor in
:core:network/preferences/—NotificationTrayLimitStore, besideViewingPreferenceStore. It readsnotificationTrayLimitoffthe shared
GET /api/user, clamps to the documented range, caches for the process, and exposespublish(Int?)soDefaultSettingsRepositoryforwards the saved value after every read/write —without that forward, a limit changed in Settings would not take effect until a restart.
This adds a reader to the ownership tangle already tracked as #104, noted in the store's KDoc
and the commit body.
Verification
./gradlew :app:assembleDebug testDebugUnitTest→ BUILD SUCCESSFUL, 1102 tests, 0 failures.Every required behaviour is covered: the PATCH carries only
notificationTrayLimit(assertedboth against the real serializer via
touchedFieldNames()and against the wire body), out-of-rangerefused with no request, the list honours the limit, the tray grouping honours it, optimistic apply
and rollback on failure. 5 Compose tests compile, not executed (no emulator).
Two judgement calls worth a second opinion
DefaultSettingsRepositorynow depends onNotificationTrayLimitStore— one constructor arg,one line in
publish. Without it the notifications side caches a stale limit for the process. Thealternative, never caching, costs an extra
GET /api/userper pull-to-refresh and perbackground poll.
previously collapsed to one summary. That is the point of the issue, but with a limit of 40 the
shade can hold considerably more than before. Android caps an app at ~25 active notifications
anyway, so the practical ceiling is lower than 40.