-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Upgrade to Microsoft.UI.Xaml 2.2 #3027
Conversation
This commit introduces the code changes required to move to MUX 2.2 final. * We had to move to the final API: * Items -> TabItems * Items.VectorChanged -> TabItemsChanged * TabClose -> TabCloseRequested * TabViewItem.Icon -> TabViewItem.IconSource * TabRowControl has been converted to a ContentPresenter, which simplifies its logic a little bit. * TerminalPage now differentiates MUX and WUX a little better * Because of the change from Icon to IconSource in TabViewItem, Utils::GetColoredIcon needed to be augmented to support MUX IconSources. It was still necessary to use for WUX, so it's been templatized. * I moved us from WUX SplitButton to MUX SplitButton and brought the style in line with the one typically provided by TabView. * Some of our local controls have had their backgrounds removed so they're more amenable to being placed on other surfaces. * I'm suppressing the TabView's padding. * I removed a number of apparently dead methods from App. * I've simplified the dragbar's sizing logic and eventing.
KNOWN ISSUES
|
feedbackFlyout.Text(_resourceLoader->GetLocalizedString(L"FeedbackMenuItem")); | ||
|
||
Controls::FontIcon feedbackIco{}; | ||
WUX::Controls::FontIcon feedbackIco{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're here, mind changing the name to feedbackIcon
? :)
@@ -16,33 +16,3 @@ std::wstring GetWstringFromJson(const Json::Value& json) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we just moved GetColoredIcon()
to the header file, why not move this one too? One less file to worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't hate it. I'd prefer to do that in a separate PR (and make it templatable :P)
I'd argue that #597 is not closed by this PR - we still need to add some settings to control that. However, now we can add those settings. |
Good call -- yoink it from my desc'n plz 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: known issues:
- How bad is this crashing? How frequently/under what circumstances? Do we have LOS on a root cause? I'd rather not add a totally unknown crash, even if the rest of this PR is goodness.
- Presumably the tab sizing one is an upstream issue - anything we can do to mitigate in the meantime? Can we link the upstream issue here/somewhere?
@@ -339,4 +339,12 @@ | |||
</ItemGroup> | |||
</Target> | |||
|
|||
<Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.2.190917002\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.190917002\build\native\Microsoft.UI.Xaml.targets')" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These ^M
's look erroneous...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops thanks
<ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText> | ||
</PropertyGroup> | ||
<Error Condition="!Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.190917002\build\native\Microsoft.UI.Xaml.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.UI.Xaml.2.2.190917002\build\native\Microsoft.UI.Xaml.targets'))" /> | ||
</Target> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also wait why was this added to the packaging project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, this is so annoying. this is how the framework pkg dep gets into the appxmanifest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll comment it
|
||
// When the size of the titlebar content changes, we want to make sure to | ||
// update the size of the drag region as well. | ||
const auto fwe = content.try_as<winrt::Windows::UI::Xaml::FrameworkElement>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this safe to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had 3 places where we were registering "drag bar size changed" on three different things; all we really need is the one on the real drag bar space.
Crashing: 100% of the time all the time |
Do we have a way to fix this asap? |
I have LOS on why it's happening, but i hate my hack fix for it (defer init until first resize) |
/azp run |
Pull request contains merge conflicts. |
Oh. Derp. |
I'm not OK taking it until this is resolved one way or another. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me, but I don't want to sign-off while it 100% crashes.
Yes, merging it would be dangerous for our community. |
The crash no longer abides. |
@DHowett-MSFT What's the deal with x86 failing? |
Who even knows! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crash no longer abides.
I'm now fine with this.
I'm not yet ready to merge this -- I want to make sure the local test infrastructure still works. |
The local tests are reported broken in an e-mail from @zadjii-msft to the TAEF team; i have not regressed them. |
🎉 Handy links: |
This pull request introduces the code changes required to move to MUX 2.2
final.
Items
->TabItems
Items.VectorChanged
->TabItemsChanged
TabClose
->TabCloseRequested
TabViewItem.Icon
->TabViewItem.IconSource
TabRowControl
has been converted to aContentPresenter
, whichsimplifies its logic a little bit.
TerminalPage
now differentiates MUX and WUX a little betterIcon
toIconSource
inTabViewItem
,Utils::GetColoredIcon
needed to be augmented to support MUX IconSources.It was still necessary to use for WUX, so it's been templatized.
SplitButton
to MUXSplitButton
and brought thestyle in line with the one typically provided by
TabView
.they're more amenable to being placed on other surfaces.
PR Checklist
Validation Steps Performed
It's tabs, and they do a good. I tried them all.
References