fix(Notifications): split sound playing responsibility#21097
Merged
Conversation
jrainville
approved these changes
Jun 1, 2026
There was a problem hiding this comment.
Pull request overview
This PR adjusts where notification sounds are triggered so that OS notifications play sounds only when an OS notification is actually shown, and in-app toast notifications can control sound playback at the point they’re added to the toast model (to avoid sounds for suppressed toasts). It also includes an unrelated set of i18n additions for a WalletNetworkDelegate string.
Changes:
- Move OS-notification sound emission to occur only when an OS notification is shown (NotificationsManager).
- Trigger sound playback when adding an ephemeral/toast notification in the main module (intended for in-app notifications).
- Add a new WalletNetworkDelegate translation string across multiple locales.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/i18n/qml_ko.ts | Adds new WalletNetworkDelegate translation entry (unfinished). |
| ui/i18n/qml_es.ts | Adds new WalletNetworkDelegate translation entry (unfinished). |
| ui/i18n/qml_cs.ts | Adds new WalletNetworkDelegate translation entry (unfinished). |
| ui/i18n/qml_base_lokalise_en.ts | Adds the new base English WalletNetworkDelegate string. |
| ui/i18n/qml_base_en.ts | Adds new WalletNetworkDelegate source with unfinished translation placeholder. |
| src/app/modules/main/module.nim | Adds sound emission after adding an ephemeral notification item. |
| src/app/core/notifications/notifications_manager.nim | Ensures sound is emitted only alongside OS notifications (when they are shown). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Jenkins BuildsClick to see older builds (10)
|
sunleos
approved these changes
Jun 1, 2026
Collaborator
sunleos
left a comment
sunleos
left a comment
There was a problem hiding this comment.
tested on Android 15
the fix is successful
let's merge it
- for OS notifications, always play the sound (if allowed) - for toasts, delay the decision for until the toast is added to the model as it might get suppressed Fixes #21040
caybro
force-pushed
the
fix/21040-suppress-toasts-sounds
branch
from
97205ab to
10faf37
Compare
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.
What does the PR do
Fixes #21040
Affected areas
Notifications/Manager
Quality checklist
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
N/A
Impact on end user
Less annoying toasts, mainly on mobile
How to test
Risk
low