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.

Sign up for GitHub

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

Jump to bottom

Update to WinUI 2.4 #3291

Merged
merged 5 commits into from
May 29, 2020
Merged

Update to WinUI 2.4 #3291

merged 5 commits into from
May 29, 2020

Conversation

michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented May 18, 2020 •
edited
Loading

Fixes #3288

Updates/Consolidates NuGet dependencies (not sure if I got all of them?), also updates to WinUI 2.4 which required Sample App Changes.

PR Type

What kind of change does this PR introduce?

  • Refactoring
  • Build or CI related changes
  • Sample app changes

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

TODO

  • Need to fix searchbox focus stealing on search (marked code line with TODO), this is a bug in the existing sample app as well, but was masked by performance.
  • Do some more testing of Layout controls with WinUI 2.4

@ghost
Copy link

ghost commented May 18, 2020

Thanks michael-hawker for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned Kyaa-dost May 18, 2020
@michael-hawker michael-hawker added this to the 6.1 milestone May 18, 2020
@michael-hawker
Copy link
Member Author

@azchohfi are we maybe missing a reference to the ValueTuple or something or should I just down-grade it back?

@azchohfi
Copy link
Contributor

azchohfi commented May 18, 2020 •
edited
Loading

Thats what is looks like. Does the same 3 tests fails on your box when you run the tests?

@michael-hawker
Copy link
Member Author

@azchohfi good call, it's weird I'm getting different but similar errors there. I'll take a look, grr.

@azchohfi azchohfi marked this pull request as ready for review May 19, 2020 20:34
@michael-hawker
Copy link
Member Author

Thanks @azchohfi I'll look at this last TODO part so we can finalize this: https://github.com/windows-toolkit/WindowsCommunityToolkit/pull/3291/files#diff-3cb108bbfe147427fb6b9a3f93d89cc7R130

@michael-hawker michael-hawker mentioned this pull request May 20, 2020
Open
4 tasks
skendrot
skendrot reviewed May 20, 2020
View reviewed changes
Microsoft.Toolkit.Uwp.SampleApp/Shell.SamplePicker.cs Outdated
@@ -125,7 +126,7 @@ private async void ShowSamplePicker(Sample[] samples = null, bool group = false)

private void NavView_ItemInvoked(Microsoft.UI.Xaml.Controls.NavigationView sender, Microsoft.UI.Xaml.Controls.NavigationViewItemInvokedEventArgs args)
{
if (args.InvokedItem is SampleCategory category)
if (args.InvokedItem is FrameworkElement fe && fe.DataContext is SampleCategory category)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this never work? Or was there a breaking change that replaced the item with the element?

Copy link
Member Author

Choose a reason for hiding this comment

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

They changed something in WinUI 2.4, I'm a bit concerned by this. We also have a custom style, so I'm not sure if it has something to do with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ranjeshj should we be concerned here? @anawishnoff @YuliKl

Or is this just because we have a fraken-template now?

Copy link

Choose a reason for hiding this comment

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

@ojhad might be able to give more context on this

Copy link
Contributor

Choose a reason for hiding this comment

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

@michael-hawker can you please file a bug in WinUI. You should not have to make this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, thanks @ranjeshj, I'll do a quick test in the morning to see if I see the same behavior in the default template, if so I'll file a bug. If not, then it must be something in our template, so I'll try from fresh and see if I still see the issue in our environment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ranjeshj @ojhad @anawishnoff Yes, this indeed a regression on 2.4 causing this to break. I have an isolated repro without custom styling and detailed information in the new bug I filed here in the WinUI repo.

I'd expect this to break pretty much anyone using NavigationView from MUX upgrading to 2.4. Is this something you'd hotfix, and if so, what would the turn-around time be for that? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if you do, I'd suggest unlisting the 2.4.0 and 2.4.2 packages on NuGet after to prevent accidental updates?

Copy link
Member Author

Choose a reason for hiding this comment

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

@azchohfi I thought we had a Keyboard focus issue in the top nav, but it's another bug in the NavigationView control as well (in both 2.3 and 2.4). I filed another issue on WinUI here for that. FYI @ranjeshj @anawishnoff @ojhad.

Microsoft.Toolkit.Uwp.SampleApp/Styles/Generic.xaml Show resolved Hide resolved
@ghost
Copy link

ghost commented May 20, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@michael-hawker
Copy link
Member Author

@azchohfi made updates. We may want to completely re-work the top-level nav to not use as much custom styling, but that can come later and/or if needed for keyboard accessibility (as I am seeing other odd behavior there).

azchohfi
azchohfi requested changes May 20, 2020
View reviewed changes
build/build.cake Outdated Show resolved Hide resolved
michael-hawker
michael-hawker commented May 21, 2020
View reviewed changes
build/build.cake Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented May 21, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@azchohfi
Copy link
Contributor

Its looking good.

@ghost ghost removed the needs attention 👋 label May 21, 2020
@michael-hawker
Copy link
Member Author

Thanks @azchohfi! Since we found an issue in WinUI, should we merge this as is and fix later if they release a new version OR should we hold???

@skendrot
Copy link
Contributor

@michael-hawker I would say hold until a new version comes if they can get it out the door quickly.

@michael-hawker
Copy link
Member Author

Updated this PR, we still have some TokenizingTextBox style issues, but no worse than where it was before this PR.

This should be good to go now if we want to take it or see if we can get a WinUI update before we close out the release?

@azchohfi
Copy link
Contributor

azchohfi commented May 28, 2020 •
edited
Loading

I think it is too risky to wait for an update at this point (the reported issue mentions it will ship by 2.5 only). This is a clean workaround, which is good enough for now. We can push a minor update if this is fixed on WinUI later.

@michael-hawker
Copy link
Member Author

@azchohfi makes sense, it's only the sample app anyway, so it should be fine. This should guard if the store updates the package for us.

@michael-hawker
Copy link
Member Author

Think this is ready to go. @Kyaa-dost do you want to take the sample app for a spin from this PR and see if you notice any major differences/inconsistencies from the store sample app?

I think the TokenizingTextBox was the main thing I saw and we still have style work to do there, but that's tracked in a separate issue, and the fix I did in here makes it no worse (even though we have to copy the style).

@Kyaa-dost Kyaa-dost closed this May 28, 2020
@Kyaa-dost Kyaa-dost reopened this May 28, 2020
@ghost ghost added the improvements ✨ label May 28, 2020
@Kyaa-dost
Copy link
Contributor

@michael-hawker Overall performance looks consistent. The only difference I noticed that this PR comes with additional sections that the current SampleApp lacks... There also has been some reorganizing in the Helpers menu as well.

Additional Sections:

Brushes (Acrylic Brush, PipelineBrush, Tiles Brush), Extensions (ClipToBounds, Icon Extensions), Helpers (ObservableGroup) State Triggers section, Controls (LayoutItemsRepeater)

@michael-hawker
Copy link
Member Author

Thanks @Kyaa-dost, yeah, that's just all the new things we've added this release, so those are expected :)

@michael-hawker
Copy link
Member Author

@Kyaa-dost want to sign-off on the review? However, we should review and merge #3310 first, so I can resolve merge conflicts here first.

Kyaa-dost
Kyaa-dost approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

SampleApp is performing as expected 🚀

@michael-hawker
Copy link
Member Author

I'm going to merge this PR in now, as I should be able to forward fix the TokenizingTextBox+WinUI Style gap after some great help from @StephenLPeters, and it'll be easier to do that and test on top of the changes from @marcpems we merged earlier and having WinUI at the latest. I'll create a new PR for those style fixes.

@michael-hawker michael-hawker merged commit ced5b7d into master May 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the mhawker/winui-2.4 branch May 29, 2020 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skendrot skendrot skendrot left review comments

@anawishnoff anawishnoff anawishnoff left review comments

@ranjeshj ranjeshj ranjeshj left review comments

@azchohfi azchohfi azchohfi approved these changes

@Kyaa-dost Kyaa-dost Kyaa-dost approved these changes

Assignees

@Kyaa-dost Kyaa-dost

Labels
build 🔥 improvements ✨ sample app 🖼
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.

Update & Consolidate NuGet packages
6 participants
@michael-hawker @azchohfi @skendrot @Kyaa-dost @anawishnoff @ranjeshj