Fixes for the E2E Unpackaged scenario#1431
Conversation
ghost
added
the
danielayala94
requested review from
hulumane,
loneursid,
pmpurifoy and
sharath2727
danielayala94
added
the
area-Notifications
This reverts commit d5d021a.
|
All unpkg tests are working now. |
| PushNotificationChannelManager channelManager{}; | ||
| winrt::PushNotificationChannel pushChannelReceived{ co_await channelManager.CreatePushNotificationChannelForApplicationAsync(unpackagedAppUserModelId.get()) }; | ||
|
|
||
| auto notificationPlatform{ wil::CoCreateInstance<NotificationsLongRunningPlatform, INotificationsLongRunningPlatform>(CLSCTX_LOCAL_SERVER) }; |
There was a problem hiding this comment.
Please move this block to a helper function.
| // Command line format: ----WindowsAppRuntimePushServer:-Payload:"<payloadAsEscapedUriFormat>" | ||
| std::wstring commandLine = L"----WindowsAppRuntimePushServer:-Payload:\""; | ||
| std::string commandLine = "----WindowsAppRuntimePushServer:-Payload:\""; | ||
| commandLine.append(reinterpret_cast<char*>(payload), payloadLength); |
There was a problem hiding this comment.
Why are we re-introducing this escape character bug? @danielayala94 ?
There was a problem hiding this comment.
Saw something like this in the other PR as well.
There was a problem hiding this comment.
@pmpurifoy Please look at the existing code in WNP_LRP. This seems to be an incorrect merge.
| } | ||
| CATCH_RETURN() | ||
|
|
||
| const std::string NotificationListener::ConvertProcessNameToUtf8String() |
There was a problem hiding this comment.
Not needed. Just use the helper method provided by @sharath2727 in his PR.
There was a problem hiding this comment.
I would remove this method and just call his Helper with m_processName wherever needed
There was a problem hiding this comment.
use the helper method provided by @sharath2727 Sharath Manchala FTE in his PR.
link?
There was a problem hiding this comment.
This function was used before the Protocol activation fixes. @pmpurifoy needs to fix the merge with WNP_LRP (i.e. remove this function).
| STDMETHOD(OnRawNotificationReceived)(unsigned int payloadLength, _In_ byte* payload, _In_ HSTRING correlationVector) noexcept; | ||
|
|
||
| private: | ||
| const std::string ConvertProcessNameToUtf8String(); |
There was a problem hiding this comment.
Remove. Not needed. Use @sharath2727 's helper method
| // Command line format: ----WindowsAppRuntimePushServer:-Payload:"<payloadAsEscapedUriFormat>" | ||
| std::wstring commandLine = L"----WindowsAppRuntimePushServer:-Payload:\""; | ||
| std::string commandLine = "----WindowsAppRuntimePushServer:-Payload:\""; | ||
| commandLine.append(reinterpret_cast<char*>(payload), payloadLength); |
There was a problem hiding this comment.
Saw something like this in the other PR as well.
| shellExecuteInfo.nShow = SW_NORMAL; | ||
|
|
||
| if (!ShellExecuteEx(&shellExecuteInfo)) | ||
| if (!ShellExecuteExA(&shellExecuteInfo)) |
There was a problem hiding this comment.
New code should use the -W variants. Why the shift to -A?
Verified. They do indeed all pass now - as long as you have this FrameworkUDK fix installed on your devide: https://microsoft.visualstudio.com/OS/_git/os.2020/pullrequest/6502196 thanks. |
loneursid
dismissed
their stale review
Verified all unit tests passing, given the patched frameworkUDK is installed on the device.
danielayala94
left a comment
danielayala94
left a comment
There was a problem hiding this comment.
@pmpurifoy I cannot block the PR because I opened it. But please do a thorough review of all files edited on this PR against the current WNP_LRP code. I noticed that NotificationListener is missing part of the fixes for Protocol.
This reverts commit 132ce5c.
There was a problem hiding this comment.
This branch cannot be merged into WNP_LRP in its current state, as would regress other changes. It seems that WNP_LRP was merged, then the merge was reverted. This means that if this branch is merged into WNP_LRP as is, we'll regress previous changes. Furthermore, if WNP_LRP was then merged into main, the regression would propagate to main. (the branch is currently bringing changes to 182 files, most of them outside of the Notifications firectories..., this isn't good at all)
@pmpurifoy, please work with @jonwis to see what is the correct solution here. I've the feeling that this branch will have to be reset to the initial merge of WNP_LRP or the the commit prior to that. That would mean re-applying the few "nits" PR that have been merged since then (as they would be lost after the reset operation.)
Also note that whatever issues you faced the first time WNP_LRP was merged in this branch, and which prompted the revert, will have to be adressed sooner or later. This branch will have to be merged into WNP_LRP at some point and that merge will need to be clean and not introduce any unwanted side-effects.
|
Moving PR here: #1543 |
7 participants
While doing extensive testing for the unpackaged apps E2E scenario, I caught a couple of bugs:
1. Bug on foreground notifications: On platform initialization, if the app list size is 0, the NotificationListenerManager was never initialized with a ForegroundSinkManager. Even after adding a listener, the ForegroundSinkManager reference on NotificationListenerManager remained empty. Now we initialize the NotificationListenerManager appropriately regardless of the app list size.
2. NotificationSink update bug: Whenever a channel request arrives, wpncore cleans the corresponding NotificationSink of the app. In such case, the LRP doesn't know if the NotificationSink (a NotificationListener in LRP jargon) is still valid. This caused new push notifications to be dropped and block the channel. With the fix, whenever RegisterForegroundActivator or RegisterLongRunningActivator are called, we remove the existing NotificationListener and replace it with a new one, doing a best effort to have an up-to-date listener. This may still require a RegisterLongRunningActivator call from WAS channel request, if applicable.