WindowsCommunityToolkit icon indicating copy to clipboard operation
WindowsCommunityToolkit copied to clipboard

ListDetailsView not working with WinUI3/WindowsAppSDK 1.0.0-preview3

Open huoyaoyuan opened this issue 4 years ago • 11 comments

Describe the bug

ListDetailsView causes hard crash in normal usage.

  • [x] Is this bug a regression in the toolkit? If so, what toolkit version did you last see it work: 7.0.1

Steps to Reproduce

  • [ ] Can this be reproduced in the Sample App? (Either in a sample as-is or with new XAML pasted in the editor.) If so, please provide custom XAML or steps to reproduce. If not, let us know why it can't be reproduced (e.g. more complex setup, environment, dependencies, etc...)

Steps to reproduce the behavior:

  1. Create a WinUI3/WAS project, install toolkit 7.1.1-preview3
  2. Add a ListDetailsView, set ListPaneWidth and some content
  3. if ListPaneWidth set in xaml, it will fail with XamlParseException which wrappes NullReferenceException 4. When moving mouse over the control. it will fail with not being able to load reveal brush Edit: I used reveal brush in my custom style. It's not caused by the control itself.

Environment

NuGet Package(s): CommunityToolkit.WinUI.UI.Controls.Layout Microsoft.WindowsAppSDK 1.0.0-preview3

Package Version(s): 7.1.1-preview3

Windows 10 Build Number:

  • [ ] Fall Creators Update (16299)
  • [ ] April 2018 Update (17134)
  • [ ] October 2018 Update (17763)
  • [ ] May 2019 Update (18362)
  • [x] May 2020 Update (19043)
  • [ ] Insider Build ({build_number})

App min and target version:

  • [ ] Fall Creators Update (16299)
  • [ ] April 2018 Update (17134)
  • [x] October 2018 Update (17763)
  • [ ] May 2019 Update (18362)
  • [x] May 2020 Update (19041)
  • [ ] Insider Build ({build_number})

Device form factor:

  • [x] Desktop
  • [ ] Xbox
  • [ ] Surface Hub
  • [ ] IoT

Visual Studio version:

  • [ ] 2017 (15.{minor_version})
  • [ ] 2019 (16.{minor_version})
  • [x] 2022 (17.{minor_version})

Additional context

This could be a issue of WinUI side, but we should be aware of them here.

huoyaoyuan avatar Nov 01 '21 18:11 huoyaoyuan

Hello huoyaoyuan, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

ghost avatar Nov 01 '21 18:11 ghost

The call stack is native SystemNavigationManager::GetForCurrentView, so this is https://github.com/microsoft/microsoft-ui-xaml/issues/4174 again.

huoyaoyuan avatar Nov 03 '21 19:11 huoyaoyuan

Thanks @huoyaoyuan I think we missed from the original WinUI discussion that there was code we needed to update here too. Thanks for filing an issue.

@azchohfi looks like we missed how best to convert these APIs here:

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/82e2bbb5df41ca9a9ba6aa6ec6dad2070aa3c90a/CommunityToolkit.WinUI.UI.Controls.Layout/ListDetailsView/ListDetailsView.cs#L173-L176

https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/82e2bbb5df41ca9a9ba6aa6ec6dad2070aa3c90a/CommunityToolkit.WinUI.UI.Controls.Layout/ListDetailsView/ListDetailsView.cs#L196-L199

According to the docs this isn't a WinUI 3 API at all. We used it here to synchronize the behavior of the ListDetailsView to the main app back button. Since that concept doesn't exist, I think we just remove this code entirely in our WinUI 3 world, right?

I think our original hope was that Window.Current was going to be a guard for this, or am I mis-remembering?

michael-hawker avatar Nov 04 '21 20:11 michael-hawker

Since that concept doesn't exist, I think we just remove this code entirely in our WinUI 3 world, right?

How about WinUI 3 with UWP? On Win32 I think we can remove it.

huoyaoyuan avatar Nov 05 '21 02:11 huoyaoyuan

@huoyaoyuan sorry are you creating a WinUI 3 UWP project? We only support WinUI 3 Desktop projects with the Toolkit.

michael-hawker avatar Nov 09 '21 18:11 michael-hawker

Ah, we found some unguarded cases in https://github.com/CommunityToolkit/WindowsCommunityToolkit/blob/main/Microsoft.Toolkit.Uwp.UI.Controls.Layout/ListDetailsView/ListDetailsView.BackButton.cs, so those may be it as well. We're adding guards :)

michael-hawker avatar Nov 09 '21 18:11 michael-hawker

@michael-hawker No, just in case if you are missing that scenario.

huoyaoyuan avatar Nov 09 '21 19:11 huoyaoyuan

@huoyaoyuan we just shipped an update which we think will address the issue?https://github.com/CommunityToolkit/WindowsCommunityToolkit/releases/tag/winui-7.1.1-preview3.1

Please let us know, thanks!

michael-hawker avatar Nov 09 '21 23:11 michael-hawker

@michael-hawker Yes it works.

However, I discovered that ListPaneWidth has a another issue. It throws NRE (0x80004003 : 'Object reference not set to an instance of an object.') if I set it in xaml. It works correctly if I set it later in xaml hot reload.

This is not so critical and I can wait for next version.

huoyaoyuan avatar Nov 10 '21 07:11 huoyaoyuan

I do have the same problem with ListPaneWidth. Unfortunately it comes out of some template and I can't find any workaround for this:

Bildschirmfoto 2021-11-18 um 17 49 34

As far as I can see there is no custom template I supply which changes the ListPaneWidth. So I'm currently not able to use the ListDetailView. I'm using winui-7.1.1-preview3.1 and tested with .NET 5.0 as well as .NET 6.0.

suchja avatar Nov 18 '21 16:11 suchja

Have same isuue. Any workarounds? I'm using version 7.1.2 and .NET 6.0

DanyaSWorlD avatar Aug 02 '22 00:08 DanyaSWorlD

Was able to reproduce this in UWP as well, so not specific to WinUI 3. It was overlooked with the change to TwoPaneView for this component in PR #3768, so setting ListPaneWidth before loading is just broken due to lack of null check in OnListPaneWidthChanged, fix should be:

        private void OnListPaneWidthChanged()
        {
            if (_twoPaneView != null)
            {
                _twoPaneView.Pane1Length = new GridLength(ListPaneWidth);
            }
        }

michael-hawker avatar Aug 25 '22 19:08 michael-hawker

Hi, Thank you for raising the Issue, Created a PR with the suggested changes

LalithaNadimpalli avatar Aug 26 '22 16:08 LalithaNadimpalli