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

[FancyZones] Set 3-zones PriorityGrid as default layout #6248

Merged
merged 3 commits into from
Sep 4, 2020

Conversation

stefansjfw
Copy link
Collaborator

Summary of the Pull Request

What is this about?
Instead of no-layout at FZ startup, now 3-zones PriorityGrid is applied by default.

PR Checklist

  • Applies to [fancyzones] Make default layout for new users #4292
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Validation Steps Performed

How does someone test & validate?

  1. Delete zone-settings.json file
  2. Run FZ
  3. Confirm that 3-zone PriorityGrid layout is applied on startup
  4. Create new Virtual Desktop
  5. Confirm that new Virtual Desktop has 3-zone PriorityGrid
  6. Delete VD
  7. Change layout on primary desktop to 4-zone Column
  8. Create new VD
  9. Confirm that new VD has 4-zone Column layout applied (Clone layout from parent feature still works)

}
else
{
deviceInfoMap[deviceId] = DeviceInfoData{ ZoneSetData{ NonLocalizable::NullStr, ZoneSetLayoutType::Blank } };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add anything into deviceInfoMap in this case? I remember that ZoneSetLayoutType::Blank has been added for some purpose, but I can't recall why exactly.
Everywhere there are checks if zone set UUID is valid and ZoneSetLayoutType is not Blank. It won't be saved in the settings file and just takes a place in the memory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll double check whether Blank type and checks are still needed at all

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that we will add functionality to the editor to set no zoneset on a desktop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🔼 That's why it is still needed :)

@enricogior enricogior added the Product-FancyZones Refers to the FancyZones PowerToy label Sep 3, 2020
Copy link
Collaborator

@SeraphimaZykova SeraphimaZykova left a comment

Choose a reason for hiding this comment

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

LGTM! Tested manually as described, works well. Unit tests passed.

wil::unique_cotaskmem_string guidString;
if (result == S_OK && SUCCEEDED(StringFromCLSID(guid, &guidString)))
{
DeviceInfoData defaultDeviceInfoData{ ZoneSetData{ guidString.get(), ZoneSetLayoutType::PriorityGrid }, true, 16, 3 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace true, 16 and 3 with consts or explicit argument names?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@enricogior
Copy link
Contributor

Code looks good, tested and works as expected.

@stefansjfw stefansjfw merged commit 78edaf5 into microsoft:master Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-FancyZones Refers to the FancyZones PowerToy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants