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

[WIP] Styles and Xaml resources compatibility #1766

Draft
wants to merge 146 commits into
base: master
from

Conversation

@davidjohnoliver
Copy link
Contributor

davidjohnoliver commented Oct 8, 2019

GitHub Issue (If applicable): fixes #119 , fixes #1335

PR Type

  • Bugfix
  • Feature

What kind of change does this PR introduce?

What is the current behavior?

  • Uno only supports global-level resources, including implicit styles

What is the new behavior?

Numerous compatibility fixes:

  • Resources can be defined at any level of the visual tree
  • Resources are resolved by scope, including implicit styles
  • default (framework) styles are additive with implicit/explicit styles
  • Resources property is now correctly populated, resources can be accessed/modified from code-behind
  • theme resources can be dynamically updated when theme changes
  • DefaultStyleKey is now respected
  • custom subclasses will now use the default style of the parent framework control unless DefaultStyleKey is overridden

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template (for bug fixes / features)
  • Unit Tests and/or UI Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Wasm UI Tests are not showing unexpected any differences. Validate PR Screenshots Compare Test Run results.
  • Contains NO breaking changes
  • Updated the Release Notes
  • Associated with an issue (GitHub or internal)

Other information

Internal Issue (If applicable):

var files = new XamlFileParser(_excludeXamlNamespaces, _includeXamlNamespaces).ParseFiles(_xamlSourceFiles);
var filesFull = new XamlFileParser(_excludeXamlNamespaces, _includeXamlNamespaces).ParseFiles(_xamlSourceFiles);
var files = filesFull.Trim()
.OrderBy(f => f.UniqueID)

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Indentation

@@ -32,8 +32,6 @@ public XamlFileDefinition[] ParseFiles(string[] xamlSourceFiles)
return xamlSourceFiles
.AsParallel()
.Select(ParseFile)
.Trim()
.OrderBy(f => f.UniqueID)

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

The ordering was added to ensure the stability of the generation (w.r.t the parallel above) and provide repeatable builds.

This comment has been minimized.

Copy link
@davidjohnoliver

davidjohnoliver Oct 17, 2019

Author Contributor

It's still ordered immediately after ParseFiles() is called, I needed the unordered files list additionally.

return relativeTargetPath;
}

originDirectory = "C:\\" + originDirectory;

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Why is it a constant ?

This comment has been minimized.

Copy link
@davidjohnoliver

davidjohnoliver Oct 17, 2019

Author Contributor

It should not be! Good catch

@@ -0,0 +1,85 @@
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Should be an SDK-style project

This comment has been minimized.

Copy link
@davidjohnoliver

davidjohnoliver Oct 17, 2019

Author Contributor

I tried initially making it an SDK-style project, got a lot of errors along these lines:

9>C:\Program Files\dotnet\sdk\2.2.104\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(208,5): error NETSDK1005: Assets file 'C:\src\nv\Uno.UI-ext-2\src\Uno.UI.Tests.ViewLibrary\obj\project.assets.json' doesn't have a target for '.NETFramework,Version=v4.0'. Ensure that restore has run and that you have included 'net40' in the TargetFrameworks for your project.

eventually gave up.

[TestMethod]
public void When_Implicit_Style_In_Visual_Tree_Local_Type()
{
var app = UnitTestsApp.App.EnsureApplication();

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Should it be in test setup ?

[TestMethod]
public void When_Resource_On_Top_Control_Xaml()
{
var app = UnitTestsApp.App.EnsureApplication();

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Test Setup ?

/// Enables the behavior for which the style is applied before the inherited
/// FrameworkElement instances constructors. The UWP behavior is to apply
/// </summary>
public static bool UseLegacyApplyStylePhase { get; set; } = false;

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Make those obsolete and invisible to intellisense, no need to remove them.

PropagateThemeChanged(root);
}

void PropagateThemeChanged(object instance)

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Sweet ;)

@@ -59,10 +60,10 @@ private int PageSize
}
else
{
if (ScrollViewer?.Style == Uno.UI.GlobalStaticResources.ListViewBaseScrollViewerStyle)
if (ScrollViewer?.Style != null && ScrollViewer.Style == ResourceResolver.GetSystemResource<Style>("ListViewBaseScrollViewerStyle")) //TODO: this, too, properly

This comment has been minimized.

Copy link
@jeromelaban

jeromelaban Oct 17, 2019

Member

Todo in this PR ?

This comment has been minimized.

Copy link
@davidjohnoliver

davidjohnoliver Oct 17, 2019

Author Contributor

Depends if I end up adjusting the timing of template application to the Measure phase. I'd rather push that particular change back, but it depends on what regressions come up in testing.

Once templates are materialized on Measure, it'll be trivially easy to give ScrollViewers inside virtualized lists a 'fake' ScrollContentPresenter without any user intervention.

@davidjohnoliver davidjohnoliver mentioned this pull request Oct 17, 2019
2 of 9 tasks complete
These changes are inapplicable to the new resources handling model, reverting them eases rebasing. (But we keep the tests)
Assign to FrameworkElement.Resources and Application.Resources properties when Resources are defined in XAML.
These changes are inapplicable to the new resources handling model, reverting them eases rebasing. (But we keep the tests)
When Default and Light/Dark dictionaries are both present, Default dictionary should be ignored completely.
Add check that Application-level StaticResources are applied before entering the visual tree.
 - Top-level ResourceDictionaries in XAML files now create a ResourceDictionary property in GlobalStaticResources
 - Map <ResourceDictionary Source="Foo/Bar.xaml"/> to correct property
 - Populate MergedDictionaries with Source'd and inline dictionaries
Propagate loading actions when a view is added to the live visual tree.
This replaces the previous approach (only a single copy of a resource was possible/permitted per app).

Uno's new approach doesn't exactly match UWP (parse-time XAML scope resolution), instead it uses a 'two-speed' approach where the lookup is made in App.Resources when the XAML is initialized, and then again at load-time based on the resources in scope in the local tree.
Use Name as key (if no x:Key). Don't try to create named resource fields for Application.
UWP allows this method to be called from a background thread. This fixes the existing MessageDialog.xaml sample.
RegisterBackingFields() now gets called twice for named dictionary entries (Given_StaticResource_Control was failing to build), ignore duplicate registrations.
Ensure that top-level resources are retrieved, now that they're no longer accessible from individual Resources dictionaries via removed DefaultResolver property. Fix test.
@davidjohnoliver davidjohnoliver force-pushed the feature/styles-compat branch from 054fc51 to 73065c7 Oct 31, 2019
@davidjohnoliver davidjohnoliver force-pushed the feature/styles-compat branch from f6c8ef9 to a5ae9a8 Oct 31, 2019
For now only IsApiContractPresent()/IsApiContractNotPresent() is supported.
Ensure ResourceDictionary is built correctly when, eg, declared inline in a property of object type. This fixes parse failure in CollectionTests Xaml test.
@@ -37,6 +37,7 @@ sealed partial class App : Application
public App()
{
ConfigureFilters(LogExtensionPoint.AmbientLoggerFactory);
ConfigureFeatureFlags();

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

Why it's not called from a static constructor? Is it because you don't want to enlist it until it's actually used?

This comment has been minimized.

Copy link
@davidjohnoliver

davidjohnoliver Nov 5, 2019

Author Contributor

No particular reason...

[assembly: AssemblyVersion("1.0.0.0")]
[assembly: AssemblyFileVersion("1.0.0.0")]
Comment on lines +35 to +36

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

This file may clash with generated code on CI for versionning. You should consider using a SDK-style project.

This comment has been minimized.

Copy link
@davidjohnoliver

davidjohnoliver Nov 5, 2019

Author Contributor

I initially tried to make it an SDK-style project and ran into weird issues, I'll circle around and try again before I complete the PR.

xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:win="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:ext="using:Uno.UI.Tests.ViewLibrary">
<SolidColorBrush x:Key="BituminousColorBrush" Color="SlateGray" />

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

Bituminous ? ;-)

application.OnLaunched(null);
}

var app = Current as App;
app.HostView.Children.Clear();

#if !NETFX_CORE
//Clear custom theme
Uno.UI.ApplicationHelper.RequestedCustomTheme = null;

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

Why do we need that?


namespace Uno.UI.Tests.App.Views
{
public partial class SpiffyItemsControl : Control

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

:-)

/// <remarks>
/// This should only be enabled for debug builds, but can greatly aid layout debugging.
/// </remarks>
public static bool AssignDOMXamlProperties { get; set; } = false;

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

Wow that's very cool!

@@ -206,7 +206,7 @@ public double FontSize
typeof(double),
typeof(ContentPresenter),
new FrameworkPropertyMetadata(
11.0,
15.0,

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

You may need to document this change...

/// Contract for DependencyObjects which react to initialization and completion of Xaml parsing.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public interface IDependencyObjectParse

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor
Suggested change
public interface IDependencyObjectParse
internal interface IDependencyObjectParse
IsLoading = true;

Comment on lines +793 to +794

This comment has been minimized.

Copy link
@carldebilly

carldebilly Nov 5, 2019

Contributor

You should consider raising the OnLoading event.

This comment has been minimized.

Copy link
@davidjohnoliver

davidjohnoliver Nov 5, 2019

Author Contributor

Eh? The override of this method already raises it.

Add the option to include the current method of XamlFileGenerator being called in the output. This aids enormously in pinpointing the code responsible for undesired output.
This allows external dependencies that were built on older versions of Uno to work, as long as they don't have Xaml resources.
Put these back for backward compatibility, marked as obsolete.
This matches the runtime behaviour of UWP, and opens the door to lazy initialization.
This is a significant optimization since most of the time only one theme will be used.
Remove the key before trying to materialize entry, this prevents infinite materialization loop if we're materializing a framework-level theme dictionary that references other system resources.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.