AppNotificationComboBox Builder Bug Fixes by loneursid · Pull Request #3393 · microsoft/WindowsAppSDK

loneursid · 2023-02-02T05:20:38Z

This PR addresses two bugs:

AppNotificationComboBox should respect the developer's wish when ordering its items in xml.
When generating its xml output, AppNotificationComboBox sorts the items according to their keys (items are key value pairs). The issue with that strategy is that the key names are completely unrelated to the sorting order. AppNotificationComboBox should simply list the items in the order they were added by the developer.
AppNotificationComboBox should immediately report an error when a duplicate item is added
AppNotificationComboBox items are represented by key value properties. If a developer adds an item with the same key twice, the old item is replaced with the new one and no error is reported. This behavior is almost never what a developer wants. Adding two items with the same key in a combobox is almost always an error on the part of the developer. Instead of silently replacing old item with the new one, AppNotificationComboBox should detect the situation and immediately report the error to the developer.

There are other issues with how properties are managed / validated (or not validated for that matter) but these will be addressed in a separate PR as they also involve other parts of the builder: #3405

@MikeHillberg was invited to the review as we are making a few changes to the API, including deprecating a property.

loneursid · 2023-02-03T03:37:21Z

/azp run

azure-pipelines · 2023-02-03T03:37:26Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

hulumane · 2023-02-03T17:41:55Z

    };

+    [contract(AppNotificationBuilderContract, 2)]
+    runtimeclass AppNotificationComboBox2


Can you follow up with our API reps on this? It will be a point of contention in the API review. Traditionally, we add more contracts and update the versioning in the existing runtime class. See wpnapps idl on how this is accomplished.

Older properties that are not applicable anymore should go through a deprecation ritual. ie. You could say E_NOTIMPL and throw an exception.

Done. We're going to go the deprecation road. For now, the caller should see a warning that the old property is being deprecated. if the developer use the old property they will incur a copy as we have to convert data from the old structure to the new one and vice-versa. The good news is that we can't have more than 5 items, so the copy won't have any performance impact.

loneursid · 2023-02-04T00:06:05Z

        AppNotificationComboBox(String id);

+        [deprecated("Use ItemList instead of Items. For more info, see MSDN.", deprecate, AppNotificationBuilderContract, 2)]
        Windows.Foundation.Collections.IMap<String, String> Items;


The Items property has been deprecated and replaced by the itemList property (couldn't think of a better name that wouldn't conflict, but open to suggestions).

In this implementation, AppNotificationComboBox does the conversion between this old property and its internal representation. The alternative would be to throw a "not implemented" error. Throwing is easier but I'm not sure this would be appropriate behavior. @MikeHillberg do you have an opinion on this?

loneursid · 2023-02-04T14:29:21Z

/azp run TransportPackage-Foundation-PR

azure-pipelines · 2023-02-04T14:29:26Z