Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[UWP] Fixes for font when using MultiWindow #11752

Merged
merged 4 commits into from
Jan 15, 2021
Merged

[UWP] Fixes for font when using MultiWindow #11752

merged 4 commits into from
Jan 15, 2021

Conversation

rmarinho
Copy link
Member

Description of Change

When using multiple windows we need to take in account that each CoreApplicationView has it's own thread so caching items doesn't work if they are shared. Like FontFamilies for example

We also need to relayout our contents when the window is resized.

Issues Resolved

API Changes

None

Platforms Affected

  • UWP

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Try open 2 our 3 windows from the control gallery

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@KPixel
Copy link

KPixel commented Aug 12, 2020

Regarding the fix for resizing secondary windows, my suggestion was a workaround.

Can you please investigate why that is not an issue for the main window? I would think that they should be resized in the same way.

@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Aug 13, 2020
@KPixel
Copy link

KPixel commented Aug 24, 2020

@rmarinho @hartez Please, only commit the fix for FontExtensions.

The issue with resizing windows should be investigated first...

Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

Split up this fix and the resizing one.

@@ -26,6 +26,10 @@ public async Task OpenSecondaryWindow(Type pageType)
ContentPage instance = (ContentPage)Activator.CreateInstance(pageType);
frame.Navigate(instance);
Window.Current.Content = frame;
Window.Current.SizeChanged += (sender, args) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

@KPixel makes a good point - why does this work? We should pull this out and create a separate issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok i removed that code.. Also another code that works is something like this bellow. But we don't want to create a new Window right?

public class NewSecondaryWindowsPage : WindowsPage
{
	protected override void OnNavigatedTo(Windows.UI.Xaml.Navigation.NavigationEventArgs e)
	{
		var pageType = e.Parameter as Type;
		ContentPage instance = (ContentPage)Activator.CreateInstance(pageType);
		RegisterWindow(instance);
		base.OnNavigatedTo(e);
	}
}

public async Task OpenSecondaryWindow(Type pageType)
{
	CoreApplicationView newView = CoreApplication.CreateNewView();
	int newViewId = 0;
	await newView.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
	{
		var frame = new Windows.UI.Xaml.Controls.Frame();
		Window.Current.Content = frame;
		frame.Navigate(typeof(NewSecondaryWindowsPage), pageType);
		Window.Current.Activate();
		newViewId = ApplicationView.GetForCurrentView().Id;
	});
	bool viewShown = await ApplicationViewSwitcher.TryShowAsStandaloneAsync(newViewId);
}

Copy link

Choose a reason for hiding this comment

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

Interesting. I think this points to the root of the issue: Application.MainPage...
I would like to create a new issue to discuss a solution there. Ok?

Copy link
Member Author

@rmarinho rmarinho Aug 25, 2020

Choose a reason for hiding this comment

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

Yeah the issue from what i understand is here :

var root = new Windows.UI.Xaml.Controls.Page();

We create a page just to fake it as "root" and so the code that listens to size changes here:

_container.SizeChanged += OnRendererSizeChanged;

Doesn't work, because the _page (that we passed to the Platform) is not added to the VisualTree

Copy link

@KPixel KPixel Aug 26, 2020

Choose a reason for hiding this comment

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

There is also the following code which expects all windows to have a specific structure:

internal static Platform Current
{
get
{
var frame = Window.Current?.Content as Windows.UI.Xaml.Controls.Frame;
var wbp = frame?.Content as WindowsBasePage;
return wbp?.Platform;
}
}

So, your code above seems like the expected way to initialize a secondary window.

I've updated my app to follow this approach, and it works.

Copy link

Choose a reason for hiding this comment

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

This means that the "fix" for the resize issue is to update OpenSecondaryWindow() in the Control Gallery app.

Also, the PageExtensions class was a trap. From what I can see, in this entire repo, it is used in two scenarios:

  1. For FormsEmbeddedPageWrapper pages (and there is no sample/test for that).
  2. For the UWP version of DualScreen. And I think that code should be updated to follow the WindowsPage approach, otherwise, it will suffer from the same resize bug.

@rmarinho rmarinho changed the title [UWP] Fixes for font and resize on Multi Window [UWP] Fixes for font when using MultiWindow Aug 25, 2020
@KPixel
Copy link

KPixel commented Aug 25, 2020

Let's ship this fix! :)

@samhouts samhouts requested a review from hartez August 25, 2020 22:03
@samhouts samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Aug 26, 2020
@samhouts samhouts changed the base branch from 4.8.0 to 5.0.0 August 26, 2020 21:31
@samhouts samhouts removed this from In Review in vCurrent (4.8.0) Aug 26, 2020
@samhouts samhouts added this to In Progress in vNext+1 (5.0.0) Aug 26, 2020
@KPixel
Copy link

KPixel commented Oct 18, 2020

Hello,

Can we please merge this simple fix before 5.0?

It is ready to go!

@rmarinho rmarinho moved this from In Progress to In Review in vNext+1 (5.0.0) Oct 29, 2020
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Nov 2, 2020
@samhouts samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Nov 3, 2020
@PureWeen PureWeen added this to In progress in v5.0.1 via automation Nov 5, 2020
@PureWeen PureWeen removed this from In Review in vNext+1 (5.0.0) Nov 5, 2020
@rmarinho rmarinho merged commit c7f2090 into 5.0.0 Jan 15, 2021
v5.0.1 automation moved this from In progress to Done Jan 15, 2021
@rmarinho rmarinho deleted the fix-11705 branch January 15, 2021 11:16
@samhouts samhouts added this to the 5.0.0 milestone Jan 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

[Bug] [UWP] Setting the FontFamily crashes a multi-window app
6 participants