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

Create Window Specific Orientation Extensions #7338

merged 4 commits into from May 20, 2022


Copy link

@PureWeen PureWeen commented May 19, 2022

Description of Change

Create extensions inside core from the essentials code that's able to base orientation on a passed in window. The FlyoutPage uses orientation information to determine initial layout. We are moving the WinUI tests to run in separate windows so the FlyoutPage needs to be able to base that information off the window it's attached to.

I thought about moving all the essentials bits to extension methods inside essentials and then calling into that but it felt like that would just start mudding up the essentials code. At some point I assume essentials will have better multi window hooks

Issues Fixed

Fixes #7306

@PureWeen PureWeen requested a review from mattleibow May 19, 2022
@@ -110,5 +111,19 @@ static void OnBackButtonPressed(Window window)

internal static Devices.DisplayOrientation GetOrientation(this IWindow window)
Copy link
Contributor Author

@PureWeen PureWeen May 19, 2022

Choose a reason for hiding this comment

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

@Eilon Eilon added the area/FlyoutPage label May 19, 2022
Base automatically changed from fix_winui_tests_step_1 to main May 19, 2022
@PureWeen PureWeen force-pushed the fix_winui_flyoutpage_tests branch from e5ef146 to dc2c2ec Compare May 19, 2022
Copy link

mattleibow commented May 19, 2022

essentials should not have hooks for ui things.

There is the DeviceDisplay.SetCurrent(xxx) and that should probably be used.

No use having an extension method on IWindow that does nothing window related

Also, not sure we have agreed on the definition of orientation... essentials works with display - such as the screen dimensions... This appears to be something that you are doing based on window size?

Are they the same?

For example, I have a landscape monitor, but my windows app is very tall (think flappy bird) so is that landscape or portrait?

And for android, you are reading the activity but essentials looks at the screen.

what should we be doing and lets make sure we figure it.

Copy link
Contributor Author

PureWeen commented May 19, 2022

@mattleibow those are all very valid questions :-)

  1. can we merge this for now as it maintains current behavior but works slightly better? Maybe log an issue?
  2. And for android, you are reading the activity but essentials looks at the screen. This seemed marginally better. It works. I tested against a device that was in landscape vs portrait.

These terms also become very muddy with foldables

Copy link

@mattleibow mattleibow left a comment

I think this PR is choosing the correct way. This choice of the flyout is based on window size, not orientation.

Maybe just a naming thing.

Just want to check that tizen as maybe there is some sort of window size vs this screen size.

Copy link

jsuarezruiz commented May 20, 2022

/azp run

Copy link

azure-pipelines bot commented May 20, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@PureWeen PureWeen enabled auto-merge (squash) May 20, 2022
Copy link
Contributor Author

PureWeen commented May 20, 2022

@rookiejava I had just copied the implementation from here

Should that code also get updated? Or is that code a little different because you don't have a window to work with?

@PureWeen PureWeen force-pushed the fix_winui_flyoutpage_tests branch from b72f87c to f470609 Compare May 20, 2022
@PureWeen PureWeen merged commit 7efa74c into main May 20, 2022
23 checks passed
@PureWeen PureWeen deleted the fix_winui_flyoutpage_tests branch May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

5 participants