-
Notifications
You must be signed in to change notification settings - Fork 309
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
User/aeloros/crashandforeground #1416
Conversation
// Major.Minor version, MinVersion=0 to find any framework package for this major.minor version | ||
const UINT32 c_Version_MajorMinor{ 0x00040001 }; | ||
const PACKAGE_VERSION minVersion{}; | ||
RETURN_IF_FAILED(MddBootstrapInitialize(c_Version_MajorMinor, nullptr, minVersion)); |
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 - when you go add your generated versioning headers, please make a pass and find and fix anybody calling Mdd with manual parameters in samples & tests.
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.
Should refactor the identity functions plus some minor comments but otherwise LGTM
@@ -14,15 +14,35 @@ using namespace winrt::Microsoft::Windows::AppLifecycle; | |||
|
|||
using namespace std::chrono; | |||
|
|||
bool IsPackagedProcess() | |||
HWND g_window = NULL; |
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: HWND g_window{};
Obligatory favorite link ;-) ES.23: Prefer the {}-initializer syntax
@@ -14,15 +14,35 @@ using namespace winrt::Microsoft::Windows::AppLifecycle; | |||
|
|||
using namespace std::chrono; | |||
|
|||
bool IsPackagedProcess() | |||
HWND g_window = NULL; | |||
wchar_t g_windowClass[] = L"TestWndClass"; // the main window class 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.
NIT: wchar_t g_windowClass[]{ L"TestWndClass"; };
Or even 'const auto' if you prefer
wchar_t g_windowClass[] = L"TestWndClass"; // the main window class name | ||
|
||
ATOM _RegisterClass(HINSTANCE hInstance); | ||
BOOL InitInstance(HINSTANCE, int); |
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.
Missing parameter names
(yes, technically not required, but useful and you have the protocol down below)
BOOL InitInstance(HINSTANCE, int); | ||
LRESULT CALLBACK WndProc(HWND, UINT, WPARAM, LPARAM); | ||
|
||
std::wstring GetFullIdentityString() |
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.
'GetFullIdentityString' is GetCurrentPackageFullName but return as a std::wstring
?
Even better if added to dev\common\AppModel.Identity.h
namespace appModel::Identity
{
inline bool IsPackagedProcess()
{
...
}
inline std::wstring GetCurrentPackageFullName()
{
WCHAR packageFullName[PACKAGE_FULL_NAME_MAX_LENGTH + 1]{};
UINT32 n{ static_cast<UINT32>(ARRAYSIZE(packageFullName)) };
const auto rc{ ::GetCurrentPackageFullName(&n, packageFullName) };
if (rc == APPMODEL_ERROR_NO_PACKAGE)
{
return std::wstring();
}
THROW_IF_WIN32_ERROR(rc);
return std::wstring(packageFullName);
}
...
return identityString; | ||
} | ||
|
||
bool HasIdentity() |
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.
Remove and changed callers to
#include <appmodel.identity.h>
...
if (AppModel::Identity::IsPackagedProcess())
{ OnActivated(sender, args); } | ||
); | ||
|
||
auto hInstance = GetModuleHandle(NULL); |
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: nullptr
EnqueueRedirectionRequestId(id); | ||
AllowSetForegroundWindow(m_processId); |
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 assume you don't care if this succeeds or not? (ignoring BOOl return value)
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
* some fixes * update manual test to handle UI
Ensure all static calls initialize the singleton for the current instance. Fixes #1200
Attempt to transfer foreground rights when redirecting an activation to another instance. Fixes #1439
Updated a useful manual test app to have UI.