Skip to content

Modify constructor for AppNotificationProgressData per API review #2148

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.

Sign up for GitHub

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

Merged
hulumane merged 7 commits into from
Feb 24, 2022
Merged

Modify constructor for AppNotificationProgressData per API review #2148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b5b0676
Add ctor to take in sequenceId
hulumane Feb 24, 2022
1925e7b
Modify test case
hulumane Feb 24, 2022
88cae57
Set AppProgressData to null by default and add null checks everywhere
hulumane Feb 24, 2022
7892d7f
Add a small comment for transient property
hulumane Feb 24, 2022
bde3ebf
Add a missing macro check for failure
hulumane Feb 24, 2022
d89296b
Address comments
hulumane Feb 24, 2022
ce1c571
Address more comments
hulumane Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dev/AppNotifications/AppNotification.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ namespace winrt::Microsoft::Windows::AppNotifications::implementation

winrt::hstring m_payload{};

winrt::Microsoft::Windows::AppNotifications::AppNotificationProgressData m_progressData{};
winrt::Microsoft::Windows::AppNotifications::AppNotificationProgressData m_progressData{ nullptr };

winrt::Windows::Foundation::DateTime m_expirationTime{};

Expand Down
9 changes: 8 additions & 1 deletion dev/AppNotifications/AppNotificationProgressData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@

namespace winrt::Microsoft::Windows::AppNotifications::implementation
{
AppNotificationProgressData::AppNotificationProgressData(uint32_t sequenceNumber)
{
THROW_HR_IF(E_INVALIDARG, sequenceNumber == 0); // The sequence number is always greater than 0
Comment thread
hulumane marked this conversation as resolved.
m_sequenceNumber = sequenceNumber;
}
uint32_t AppNotificationProgressData::SequenceNumber()
{
auto lock{ m_lock.lock_shared() };
Expand All @@ -12,8 +17,10 @@ namespace winrt::Microsoft::Windows::AppNotifications::implementation
}
void AppNotificationProgressData::SequenceNumber(uint32_t sequenceNumber)
{
auto lock{ m_lock.lock_exclusive() };
THROW_HR_IF(E_INVALIDARG, sequenceNumber == 0); // The sequence number is always greater than 0

auto lock{ m_lock.lock_exclusive() };

m_sequenceNumber = sequenceNumber;
}
hstring AppNotificationProgressData::Title()
Expand Down
4 changes: 2 additions & 2 deletions dev/AppNotifications/AppNotificationProgressData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace winrt::Microsoft::Windows::AppNotifications::implementation
struct AppNotificationProgressData : AppNotificationProgressDataT<AppNotificationProgressData>
{
AppNotificationProgressData() = default;

AppNotificationProgressData(uint32_t sequenceNumber);
Copy link
Copy Markdown

@robertzhou-wpn robertzhou-wpn Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we use default parameter here like: AppNotificationProgressData(uint32_t sequenceNumber =1)?

Copy link
Copy Markdown
Member Author

@hulumane hulumane Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would assume that we provide a default constructor in our projections. However, there is no default projection per API review feedback. We only have an overload ctor.

uint32_t SequenceNumber();
void SequenceNumber(uint32_t sequenceNumber);
hstring Title();
Expand All @@ -19,7 +19,7 @@ namespace winrt::Microsoft::Windows::AppNotifications::implementation
void Status(hstring const& progressStatus);

private:
uint32_t m_sequenceNumber{};
uint32_t m_sequenceNumber = 1;
Comment thread
hulumane marked this conversation as resolved.
hstring m_title;
double m_progressValue{};
hstring m_progressValueString;
Expand Down
7 changes: 3 additions & 4 deletions dev/AppNotifications/AppNotificationUtility.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,8 @@ winrt::Microsoft::Windows::AppNotifications::AppNotification Microsoft::Windows:
THROW_IF_FAILED(properties->get_ToastProgressData(toastProgressData.put()));
if (toastProgressData)
{
winrt::AppNotificationProgressData progressData{};

// SequenceNumber is transient and thus, left to its default.
// Sequence number is a transient property and we give it a default non-zero value of 1 in the ctor
winrt::AppNotificationProgressData progressData{ 1 };

wil::unique_hstring status{};
THROW_IF_FAILED(toastProgressData->get_Status(&status));
Expand All @@ -368,7 +367,7 @@ winrt::Microsoft::Windows::AppNotifications::AppNotification Microsoft::Windows:
progressData.Title(wil::str_raw_ptr(title));

double progressValue{};
toastProgressData->get_Value(&progressValue);
THROW_IF_FAILED(toastProgressData->get_Value(&progressValue));
Comment thread
hulumane marked this conversation as resolved.
progressData.Value(progressValue);

wil::unique_hstring progressValueString{};
Expand Down
5 changes: 3 additions & 2 deletions dev/AppNotifications/AppNotifications.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ namespace Microsoft.Windows.AppNotifications
runtimeclass AppNotificationProgressData
{
// Initializes a new Instance of NotificationProgressData
AppNotificationProgressData();
// The sequence number is non-zero or this will throw.
AppNotificationProgressData(UInt32 sequenceNumber);

// Gets or sets the sequence number of this notification data.
// Gets or sets a non-zero sequence number of this notification data.
// When multiple NotificationProgressData objects are received, the system displays the data with the greatest non-zero number.
UInt32 SequenceNumber;

Expand Down
6 changes: 5 additions & 1 deletion dev/AppNotifications/NotificationProgressData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ namespace ToastABI
}

NotificationProgressData::NotificationProgressData(winrt::AppNotificationProgressData const& progressData)
: m_progressData(progressData.SequenceNumber())
{
m_progressData = progressData;
m_progressData.Status(progressData.Status());
m_progressData.Title(progressData.Title());
m_progressData.Value(progressData.Value());
m_progressData.ValueStringOverride(progressData.ValueStringOverride());
}

STDMETHODIMP NotificationProgressData::get_SequenceNumber(_Out_ unsigned int* value) noexcept
Expand Down
12 changes: 9 additions & 3 deletions dev/AppNotifications/NotificationProperties.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ NotificationProperties::NotificationProperties(winrt::AppNotification const& toa

m_expiresOnReboot = toastNotification.ExpiresOnReboot();

m_toastProgressData = winrt::make_self<NotificationProgressData>(toastNotification.Progress());
if (toastNotification.Progress() != nullptr)
{
m_toastProgressData = winrt::make_self<NotificationProgressData>(toastNotification.Progress());
}
}

STDMETHODIMP_(HRESULT __stdcall) NotificationProperties::get_NotificationId(_Out_ unsigned int* notificationId) noexcept
Expand Down Expand Up @@ -130,9 +133,12 @@ STDMETHODIMP_(HRESULT __stdcall) NotificationProperties::get_ExpiresOnReboot(_Ou

STDMETHODIMP_(HRESULT __stdcall) NotificationProperties::get_ToastProgressData(_Out_ ToastABI::IToastProgressData** progressData) noexcept
{
*progressData = nullptr;
Comment thread
hulumane marked this conversation as resolved.
auto lock{ m_lock.lock_shared() };

m_toastProgressData.copy_to(progressData);
if (m_toastProgressData != nullptr)
{
m_toastProgressData.copy_to(progressData);
}

return S_OK;
}
Expand Down
2 changes: 1 addition & 1 deletion dev/AppNotifications/NotificationProperties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,5 @@ struct NotificationProperties : winrt::implements<NotificationProperties, ::ABI:

bool m_expiresOnReboot = false;

winrt::com_ptr<ABI::Microsoft::Internal::ToastNotifications::IToastProgressData> m_toastProgressData;
winrt::com_ptr<ABI::Microsoft::Internal::ToastNotifications::IToastProgressData> m_toastProgressData{ nullptr };
};
58 changes: 26 additions & 32 deletions test/TestApps/ToastNotificationsTestApp/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,19 +197,13 @@ winrt::AppNotificationProgressData GetToastProgressData(
std::wstring const& title,
double const& progressValue,
std::wstring const& progressValueString,
bool setSequence,
uint32_t sequenceNumber)
{
winrt::AppNotificationProgressData progressData{};
winrt::AppNotificationProgressData progressData{sequenceNumber};
progressData.Status(status);
progressData.Title(title);
progressData.Value(progressValue);
progressData.ValueStringOverride(progressValueString);
if (setSequence)
{
progressData.SequenceNumber(sequenceNumber);
}

return progressData;
}

Expand Down Expand Up @@ -413,7 +407,7 @@ bool VerifyToastProgressDataFromToast()
{
winrt::AppNotification toast{ CreateToastNotification() };

winrt::AppNotificationProgressData progressData{};
winrt::AppNotificationProgressData progressData{ 1 };
progressData.Status(L"SomeStatus");
progressData.Title(L"SomeTitle");
progressData.Value(0.14);
Expand Down Expand Up @@ -442,6 +436,11 @@ bool VerifyToastProgressDataFromToast()
return false;
}

if (progressDataFromToast.SequenceNumber() != 1)
{
return false;
}

return true;
}

Expand Down Expand Up @@ -574,7 +573,7 @@ bool VerifyUpdateToastProgressDataUsingValidTagAndValidGroup()
toastNotificationManager.UnregisterAll();
});

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 1);

auto progressResultOperation = winrt::AppNotificationManager::Default().UpdateAsync(progressData, L"PTag", L"PGroup");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::Succeeded);
Expand All @@ -592,7 +591,7 @@ bool VerifyUpdateToastProgressDataUsingValidTagAndValidGroup_Unpackaged()

PostToastHelper(L"Tag", L"Group");

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", 1);

auto progressResultOperation = toastNotificationManager.UpdateAsync(progressData, L"Tag", L"Group");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::Succeeded);
Expand All @@ -610,7 +609,7 @@ bool VerifyUpdateToastProgressDataUsingValidTagAndEmptyGroup()

PostToastHelper(L"PTag", L"");

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 1);

auto progressResultOperation = winrt::AppNotificationManager::Default().UpdateAsync(progressData, L"PTag");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::Succeeded);
Expand All @@ -628,7 +627,7 @@ bool VerifyUpdateToastProgressDataUsingValidTagAndEmptyGroup_Unpackaged()

PostToastHelper(L"Tag", L"");

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", 1);

auto progressResultOperation = toastNotificationManager.UpdateAsync(progressData, L"Tag");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::Succeeded);
Expand All @@ -643,20 +642,15 @@ bool VerifyToastUpdateZeroSequenceFail_Unpackaged()
[&] {
toastNotificationManager.UnregisterAll();
});
PostToastHelper(L"Tag", L"");

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", false, 1);

auto progressResultOperation = toastNotificationManager.UpdateAsync(progressData, L"Tag");

if (progressResultOperation.wait_for(std::chrono::seconds(2)) == winrt::AsyncStatus::Completed)
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 1);
try
{
return false;
progressData.SequenceNumber(0);
}
else
catch (...)
{
progressResultOperation.Cancel();
return true;
return winrt::to_hresult() == E_INVALIDARG;
}
}

Expand All @@ -672,7 +666,7 @@ bool VerifyUpdateToastProgressDataUsingEmptyTagAndValidGroup()
toastNotificationManager.UnregisterAll();
});

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 1);

auto progressResultOperation = winrt::AppNotificationManager::Default().UpdateAsync(progressData, L"", L"Group").get();
}
Expand All @@ -695,7 +689,7 @@ bool VerifyUpdateToastProgressDataUsingEmptyTagAndEmptyGroup()
toastNotificationManager.UnregisterAll();
});

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 1);

auto progressResultOperation = winrt::AppNotificationManager::Default().UpdateAsync(progressData, L"", L"").get();
}
Expand All @@ -718,7 +712,7 @@ bool VerifyToastProgressDataSequence0Fail()
toastNotificationManager.UnregisterAll();
});

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", true, 0);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 0);
}
catch (...)
{
Expand All @@ -740,7 +734,7 @@ bool VerifyFailedUpdateNotificationDataWithNonExistentTagAndGroup()

PostToastHelper(L"PTag", L"PGroup");

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 1);

auto progressResultOperation = winrt::AppNotificationManager::Default().UpdateAsync(progressData, L"NonExistentTag", L"NonExistentGroup");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::AppNotificationNotFound);
Expand All @@ -757,7 +751,7 @@ bool VerifyFailedUpdateNotificationDataWithNonExistentTagAndGroup_Unpackaged()

PostToastHelper(L"Tag", L"Group");

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", 1);

auto progressResultOperation = toastNotificationManager.UpdateAsync(progressData, L"NonExistentTag", L"NonExistentGroup");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::AppNotificationNotFound);
Expand All @@ -773,7 +767,7 @@ bool VerifyFailedUpdateNotificationDataWithoutPostToast()
toastNotificationManager.UnregisterAll();
});

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"PStatus", L"PTitle", 0.10, L"10%", 1);

auto progressResultOperation = winrt::AppNotificationManager::Default().UpdateAsync(progressData, L"Tag", L"Group");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::AppNotificationNotFound);
Expand All @@ -788,7 +782,7 @@ bool VerifyFailedUpdateNotificationDataWithoutPostToast_Unpackaged()
toastNotificationManager.UnregisterAll();
});

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", true, 1);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", 1);
auto progressResultOperation = toastNotificationManager.UpdateAsync(progressData, L"SomeRandomTag", L"SomeRandomGroup");
return ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::AppNotificationNotFound);
}
Expand Down Expand Up @@ -831,7 +825,7 @@ bool VerifyGetAllAsyncWithOneActiveToast()
toast.Priority(winrt::AppNotificationPriority::High);
toast.SuppressDisplay(true);

winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", false, 0);
winrt::AppNotificationProgressData progressData = GetToastProgressData(L"SomeStatus", L"SomeTitle", 0.14, L"14%", 1);
toast.Progress(progressData);

auto toastNotificationManager = winrt::AppNotificationManager::Default();
Expand Down Expand Up @@ -906,12 +900,12 @@ bool VerifyGetAllAsyncIgnoresUpdatesToProgressData()
winrt::AppNotification toast{ CreateToastNotification() };
toast.Tag(L"Tag");
toast.Group(L"Group");
winrt::AppNotificationProgressData initialProgressData = GetToastProgressData(L"Initial Status", L"Initial Title", 0.05, L"5%", true, 1);
winrt::AppNotificationProgressData initialProgressData = GetToastProgressData(L"Initial Status", L"Initial Title", 0.05, L"5%", 1);
toast.Progress(initialProgressData);

toastNotificationManager.Show(toast);

winrt::AppNotificationProgressData updatedProgressData = GetToastProgressData(L"Updated Status", L"Updated Title", 0.14, L"14%", true, 2);
winrt::AppNotificationProgressData updatedProgressData = GetToastProgressData(L"Updated Status", L"Updated Title", 0.14, L"14%", 1);
auto progressResultOperation = toastNotificationManager.UpdateAsync(updatedProgressData, L"Tag", L"Group");
if (!ProgressResultOperationHelper(progressResultOperation, winrt::AppNotificationProgressResult::Succeeded))
{
Expand Down