Skip to content
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

Tweaked OS Detection project structure and added os check for PT Run #4253

Merged

Conversation

arjunbalgovind
Copy link
Contributor

Summary of the Pull Request

This PR fixes the issue of PowerLauncher.exe getting launched on OS version less than 19H1 and also makes a few changes to the os-detection project structure.

  • os-detect.cpp and os-detect.h references have been added to common project. That way we can use the functions in C++ executables/dlls directly by referencing and without requiring os-detection.dll in that folder.
  • The reference to os-detection.dll in runner has been removed, and instead the method is accessed from common
  • os-detection.dll is still required for Image Resizer so no change has been made to affect that. In the future we could merge os-detection.dll into the PowerToysInterop library to reduce the number of additional projects/dlls.
  • In the Microsoft.Launcher project (which is the PowerToyModule project for PT Run), under the enable method where PowerLauncher.exe is launched a check has been added to launch the executable only if it matches the OS requirement.

References

PR Checklist

Validation Steps Performed

Verified that behavior is as expected for Launcher/ImageResizer/Settings on 1909, 1809 VM and an internal build number VM. Only for the 1809 VM the old settings would be loaded, image resizer would use its old settings UI, and PowerLauncher.exe would not be launched.

@arjunbalgovind arjunbalgovind added the Product-PowerToys Run Improved app launch PT Run (Win+R) Window label Jun 11, 2020
@@ -132,60 +133,64 @@ class Microsoft_Launcher : public PowertoyModuleIface {
// Enable the powertoy
virtual void enable()
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only change made on this file was the following:
Earlier it was

enable() 
{
  logic; 
  m_enabled=true;
}

Now it is

enable() 
{
  if (UseNewSettings())
  {
    logic;
  }
  m_enabled = true;
}

@arjunbalgovind
Copy link
Contributor Author

arjunbalgovind commented Jun 11, 2020

@enricogior this solves #3730 and partially solves some of the issues in #3579 . I have still kept the dll since it is required for Image Resizer so that it can make it's old settings UI visible when the OS version is older than 19h1.

@enricogior
Copy link
Contributor

@arjunbalgovind
we should remove the DLL and use the PowerToysInterop DLL instead. Having one DLL for one function is not ideal, otherwise we end up adding individual DLLs every time we have to expose something to C# projects. But we can merge your PR and complete the work later on, so please keep #3579 open after merging this. Thank you.

Copy link
Contributor

@enricogior enricogior left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-PowerToys Run Improved app launch PT Run (Win+R) Window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants