-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API Spec for Badge Notifications in WindowsAppSdk #4823
base: main
Are you sure you want to change the base?
Conversation
static BadgeNotificationManager Default{ get; }; | ||
|
||
// Applies a change to the badge's glyph or number. | ||
void UpdateBadge(BadgeNotification notification); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this more, is the BadgeNotification
class necessary? It seems like it's "in the way" - providing little additional value over just calling methods directly. You could imagine a model like:
void SetNotificationCount(UInt32 notificationCount);
void SetNotificationCount(UInt32 notificationCount, DateTime expiration);
void SetNotificationGlyph(BadgeNotificationGlyph glyph);
void SetNotificationGlyph(BadgeNotificationGlyph glyph, DateTime expiration);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Encapsulation: By having a separate
BadgeNotification
class, I have encapsulate all the properties and behaviors related to the badge's appearance and data. This makes the class easier to understand and maintain. TheBadgeNotificationManager
then focuses solely on the management tasks, such as updating and clearing the badge. -
Extensibility: In the future, the
BadgeNotification
class may need to support additional properties or methods (e.g., badge color, expiration time, or animation). Having a dedicated class for the badge notification makes it easier to extend its functionality without cluttering the management class or violating SRP. -
Testability: With a separate
BadgeNotification
class, I can test the badge's functionality in isolation from the management logic. This leads to more focused and reliable unit tests, as each test can concentrate on a single aspect of the system. -
Reusability: If the badge notification logic is encapsulated in its own class, it's possible to reuse the
BadgeNotification
class in different contexts within the application or across different applications, without needing to bring along the management logic. -
Separation of Concerns: By keeping the
BadgeNotification
andBadgeNotificationManager
classes separate, I have adhered to the principle of separation of concerns. Each class addresses a distinct concern: one for representing the badge notification data and another for handling the lifecycle of badge notifications. -
Maintainability: As the application grows and evolves, having a clear separation between the representation of a badge notification and its management will make the codebase more maintainable. Changes to the notification appearance or data structure will not impact the management logic, and vice versa.
-
Collaboration: In a team environment, different developers might work on different aspects of the badge notifications. Having separate classes allows for clearer ownership and reduces the likelihood of merge conflicts when multiple developers are working on the same codebase.
In summary, keeping the BadgeNotification
class separate from the BadgeNotificationManager
class adheres to good object-oriented design principles and sets a strong foundation for a maintainable, extensible, and testable codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks. Next up is API review with the reps group. I'll get something on the calendar for us. You are welcome to make a child branch now and start implementation if you'd like, just be aware that there may be interface-changing feedback.
> Note: all of this new WinAppSDK API is to support Badge Notifications in WindowsAppSdk | ||
|
||
```c++ (but really MIDL3) | ||
namespace Microsoft.Windows.BadgeNotifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why create a new namespace for .BadgeNotifications instead of putting these enum and classes into Microsoft.Windows.AppNotifications? Is there a use-case for these outside of apps' badges?
(Adding to the existing namespace looks analogous to BadgeUpdater being in Windows.UI.Notifications Namespace which is described as "Contains classes that encapsulate tile, toast, and badge notifications.". Is WinAppSDK intentionally more granular?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DISCUSSION: This follows the PSDK model of having two different namespaces.
RECOMMEND: OK with this as-is, under the BadgeNotification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DISCUSSION: This is the last of the -Notification
types coming in from PSDK (app, push, badge.) There's also Widget notifications. We should consider what happens when other visual formats of apps need to convey information...
@SatwikKrSharma please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
```c++ | ||
private void updateBadgeGlyph() | ||
{ | ||
winrt::BadgeNotificationGlyph badgeNotificationGlyph = winrt::BadgeNotificationGlyph::Alert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TYPO
winrt::BadgeNotificationGlyph badgeNotificationGlyph = winrt::BadgeNotificationGlyph::Alert | |
winrt::BadgeNotificationGlyph badgeNotificationGlyph = winrt::BadgeNotificationGlyph::Alert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RECOMMEND: Fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
static BadgeNotificationManager Current{ get; }; | ||
|
||
// Applies a change to the badge's glyph or number. | ||
void Update(BadgeNotification notification); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the app has multiple windows, which one gets the badge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DISCUSSION: Only the "main window" gets the badge - controlled by the existing shell implementation. For people who disdain glomming ("Combine taskbar icons"), the shell has to pick one top-level Window, where the "main badge" lives. Document that the selected tile is system-controlled.
```c++ | ||
private void updateBadgeGlyph() | ||
{ | ||
winrt::BadgeNotificationGlyph badgeNotificationGlyph = winrt::BadgeNotificationGlyph::Alert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RECOMMEND: Fix
static BadgeNotificationManager Current{ get; }; | ||
|
||
// Applies a change to the badge's glyph or number. | ||
void Update(BadgeNotification notification); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DISCUSSION: Only the "main window" gets the badge - controlled by the existing shell implementation. For people who disdain glomming ("Combine taskbar icons"), the shell has to pick one top-level Window, where the "main badge" lives. Document that the selected tile is system-controlled.
This is the API Spec for the Badge Notifications in WindowsAppSdk. Issue - #138
A microsoft employee must use /azp run to validate using the pipelines below.
WARNING:
Comments made by azure-pipelines bot maybe inaccurate.
Please see pipeline link to verify that the build is being ran.
For status checks on the main branch, please use TransportPackage-Foundation-PR
(https://microsoft.visualstudio.com/ProjectReunion/_build?definitionId=81063&_a=summary)
and run the build against your PR branch with the default parameters.