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.
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
AppNotificationBuilder - ProgressBar #2780
AppNotificationBuilder - ProgressBar #2780
Changes from 61 commits
7119200
4765ea2
30a4376
40bd764
8668d3a
1545006
e470027
422eae8
f55aad3
aa7ed04
e0b2847
15b477c
903956a
bbe8a26
f5a745f
435a152
238684d
5423935
d427450
5dc491a
2b24c69
33a4b1b
5bd1f9e
51fabe4
41a699e
97226a5
a1634f8
51a3312
5ea7ddd
ab89042
019077a
f8b850e
6b28335
f45fb7a
f81258e
8851cae
224877f
861014d
a934566
19d0cd4
ab271d6
24e7669
9c6bb22
8f51b89
f8ed6ca
49c02bb
3b3b08c
a2212e2
14a6e6c
604a24f
b2fafbf
f58cd45
cccfba6
057071a
b56536a
5d5a50f
29c561a
7be9f9c
232632e
e2f9e57
13fea1b
703c0a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We should remove the BindMode::Enum and the member *Status postfix member variables.
For all Bind* functions, we can set the member variable to the constant string. For all Set* functions, we can set the member variable to the passed in param.
The default ctor should set m_status/m_value to the bind values and leave the rest empty.
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.
What about value? how would you handle that?
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.
I would convert m_status from double->winrt::hstring and set the value to {progressValue}.
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.
But if the user sets the title property to "title", then calls TitleBind(), then retrive the Title property, he'd get {progreeTitle} but if he does the same for value, what would he get? The value before he called bind, 0.0, -1.0? we throw?
If we return the value that was set before calling bind, how do we document that the APIs behave so disparatly?
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.
I'm ok with throwing here.
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.
What should we use as the binding default double for value? @hulumane
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.
The toolkit sets the default to 0: https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/b21dbe71f64e86ca9bb0ba6cfb4cb4d20e93765a/Microsoft.Toolkit.Uwp.Notifications/Adaptive/AdaptiveProgressBar.cs#L39
If we do it this way, we need to track if the value was manually set to 0 by the dev since it is valid value.
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.
@andrewleader as well for input.
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.
I'm not sure I fully understand the specific problem trying to be addressed here, but I'll try to give some general info.
Note that toast progress bars MUST have a value to be valid (I just tested a toast XML without any value attribute and got a "New notification" notification).
I believe XAML progress bars have a default value of 0, hence why I decided to just make the default value 0 in the Toolkit, even though technically you could argue this is a "nullable" property (but if Value doesn't have a value, you have an invalid toast). I felt just defaulting to 0 is the most intuitive and what developers would "expect", similar to how a string value typically defaults to null, and a new XAML ProgressBar "just works" without setting a value (defaults to 0).
Why would we need to track if the value was manually set to 0 by the dev? Regardless of who sets it to 0, we write it as 0, yeah? Or is this related to returning a value depending on whether BindValue() vs SetValue() was called?
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.
@andrewleader : As I understand the specs the value element in the xml can have a value between 0 and 1 or {progressValue}. That all fine and dandy but we have a property that expose the value as a double. That property can represent a value between 0 and 1 but cannot represent {progressValue}.
There are 2 options:
1- represent {progressValue} as -1. so the double property can have a value from 0 to 1 or -1.
2- Only use 0 to 1 for the property and maintain an internal flag to indicate whether the value from the property (0 to 1) or {progressValue} should be placed in the xml.
There is no consensus yet, but we need to make a call soon.
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.
No check for empty args here or anywhere else?
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.
No, there's a task to handle that in a hollistic manner later. At this point we only check for empty IDs and such but not for normal values. This is the strategy used for the entirety of the builder for preview1
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.
Do we need locks here and everywhere else?
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.
No locks anywhere. Howard confirmed that we should be locking anything in this class as it isn't intended to be used in a multi-threaded situation
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.
2 lines please
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.
of course...
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.
Can we append *BindMode to all this m_ variables?
For example m_tileBindMode, m_statusBindMode
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.
Sure
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.
Done
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.
This is a little complicated to read. Can we break this up by assigning each string first and directly passing the arguments to printf rather than doing everything here?
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.
yup
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.
For example, we expect 4 strings here as an argument. So lets initialize the 4 strings and break it up before passing it to printf
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.
done
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.
I'm surprised this works since the object doesn't inherit IStringable in the idl, but maybe . I like the idea of processing everything through BuildNotification instead of casting to IStringable to simulate what devs would actually be doing.