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
[Prototype] Implement createchannelasync for requesting channels #508
[Prototype] Implement createchannelasync for requesting channels #508
Changes from 1 commit
78714ec
1a35ff2
3c6e8bc
cf7f238
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.
ProjectReunion
shouldn't be used in any symbols or code references.What's the target namespace? Please update to Microsoft::Something::Something::ChannelStatus
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 will address this appropriately. Thanks.
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.
@DrusTheAxe I'm aligned with you here, but WinUI 3 is going GA with
Microsoft.ProjectReunion
namespace prefix. I think the rules changed overnight?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.
WinUI3 is
Microsoft.UI.Xaml
- we just had a namespace naming discussion internally, and will publish guidance here soon. The short version is that this should becomeMicrosoft.Windows.PushNotification.Something
... theMicrosoft.ProjectReunion
namespace was purely a placeholder that got out of hand from my initial commit. :) (Notably, we hadn't had a conversation aboutMicrosoft.ProjectReunion
initially; so view this as going from "no rules" to "some rules" overnight.)That'll be part of the API design & review guidelines that get published, probably early next week at this point.
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.
Jon, thanks for clearing up the namespace related concerns. Once the spec is approved, I would be changing the namespace appropriately. Till then this PR would only be targetted to be part of our private branch WNP_Reunion and not the main. Thanks.
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.
@DrusTheAxe - The spec is being done by Pavan which is currently going through the spec review process. I will be using that spec details to make changes to my code for namespaces.
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.
Not namespace. Package name
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.
Given that Mr. Gallo was in the discussion last night and approved of the guidelines I'll be publishing, yes. :)
There may be a short period of time where the guidance is being rolled out and people move their code around. I recognize that's disruptive, so we want to get it sorted before 0.8, at least. @MikeHillberg or @jevansaks can comment on the WinUI3 namespaces, but I'm pretty sure they've always been
Microsoft.UI.*
and will continue to be.Maybe the confusion is that WinUI3 will be delivered via the Project Reunion vehicle that @EHO-makai and company are making steady progress on. We intend Project Reunion as a delivery mechanism for lots of interesting types in the coming months. WinRT and the framework package system we're using lets us be flexible on mapping typenames to implementation DLLs.
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 reader context: I quickly pulled my comment after posting (asking Jon to double-check) but I was too slow; two folks replied. 👀
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.
#458 - this is the spec for reference. Thanks.
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 #define? Why not e.g.
inline constexpr std::uint32_t c_maxBackoffSeconds{ 960 };
?Bonus points for putting that inside a namespace e.g.
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.
Since this is a time, maybe use the time constants and time-related functions.
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 co_await: co_await winrt::resume_after(std::chrono::seconds(backOffTimeInSeconds)); this takes care of it here. I think this should suffice here: inline constexpr std::uint32_t c_maxBackoffSeconds{ 960 }; ?? please correct me if I am missing something 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.
Are these HRESULTs defined by winerror.h? Are they new constants not in the SDK we're currently compiling against?
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.
@jonwis We have some internal error codes(not part of winerrors.h) that we need to retry on. How do we expose this in Reunion? Is there a common header that we could use or do we go with our own header?
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.
Just declare them here as you have them, but use the MAKE_HRESULT macro. No need to find or add some global common header. If consumers of the API are expected to do something interesting with them, add specific Status codes to the result object that clarify what happened.
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.
NIT: Rename to
hr
There's nothing about this function that requires the parameter to have come from an exception.
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 what the C++ Core Guidelines say about case fallthrough (some linters would flag it).
Simpler to avoid the switch statement entirely
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 verify the implications of lifetime management? What would happen if the process exited but this AsyncProgress operation is not yet complete? Is the process termination graceful?
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.
https://en.cppreference.com/w/cpp/language/coroutines - When the coroutine state is destroyed either because it terminated via co_return or uncaught exception, or because it was destroyed via its handle, it does the following:
calls the destructor of the promise object.
calls the destructors of the function parameter copies.
calls operator delete to free the memory used by the coroutine state
transfers execution back to the caller/resumer.
So the coroutines completes gracefully.
Also it is important that the caller of the coroutine API: CreateChannelAsync will have to call IAsyncOperationWithProgress.Cancel to stop the asynchronous operation before the caller process exits. We would be clearly mentioning this in the sample that we are providing to cancel the asyncoperation if you are going out of scope. Thanks.
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.
remoteId
is awinrt::guid
. That's not the same memory layout as aGUID&
Why do you need to cast
remoteId
toGUID
?Can lines 57-58 be replaced by
?
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 whole PR was meant to be a prototype to see if I am able to request channel. Maybe I should have been clear with the PR heading and must have created a draft. This will change to using winrt::guid and will throw is guid is null as it is considered a critical failure :)
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.
GitHub doesn't do 'draft' PRs (not that I've found so far; do speak up if you figure out how)
@jonwis @EHO-makai @BenJKuhn do we have guidance how we should handle 'draft' code reviews? A branch lacks sharing/comment support so a PR's superior to get feedback. If I make a 3-level branching structure main->foo->foo_x and make PRs for foo_x->foo what's the review expectations? Standard review intensity for L3->L2 as L2->L1 (main)? And when the L2 is finally deemed ready to move to L1, do the same review standards apply? As a practical matter, that can lead to very large L2 PRs (harder and more costly to review), but a rubberstamp flush L2->L1 is bad if the L3->L2 review doesn't have the same strict level of review attention and intensity as going into main "because it's just a draft" etc.
There are potential differences here vs how it's handled in other environments. We should have a clear definition of expectations, or folks will do what they've done before (reasonably) assuming that's the way.
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.
Draft PRs were introduced a year+ ago. https://github.blog/2019-02-14-introducing-draft-pull-requests/
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.
Super! Thanks for the pointer
You can convert a Draft PR to not-Draft. Can you go the other way? 'downgrade' a PR to draft? I don't see it mentioned
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 don't have appropriate permissions, but you should see this under Reviewers area in top-right corner:
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.
@DrusTheAxe Sure. I have converted this to a draft. I think I agree to the fact that PR review L3->L2 and L2->L1 should have the same review standards (if it is not a draft). Can we have some guidelines on the process so it makes it easier for the developers to follow that approach going ahead.
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 API call will fail for Unpackaged apps. Recommendation is to have a check somewhere to see if the solution is packaged or not and maybe fail the API for unpackaged. We should fix this for V1 though when we call into Registration private APIs for unpackaged apps
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.
Note: When you call into the Private APIs for unpackaged apps, you would need to set the PFN to remoteId so that we send it across the wire. That should be supported in downlevel OS's too.
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.