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

[Experiment] 🧪 DataTable Initial Prototype #418

Merged
merged 38 commits into from
Aug 2, 2023
Merged

Conversation

michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented Apr 12, 2023

Implements #415

Wrote up all details of current prototype and to dos here: #415 (comment)

Tested on Win10 with UWP and WinUI 3.

(Uno WASM won't work due to ItemsPresenter not supporting Header/Footer, see Uno links in discussion above)

Want to fix the following before committing:

  • resizable columns
  • sample with many rows
  • how the layout interacts for the 'Auto' sized columns
  • how do columns that get resized behave
  • how to account for different padding/alignment of different parent controls
  • P2: Make a sample with sticky header
  • P2: Sample with blank headers, just columns in rows?

@michael-hawker michael-hawker added the experiment 🧪 Used to track issues that are experiments (or their linked discussions) label Apr 12, 2023
@michael-hawker
Copy link
Member Author

Hmm.. interesting we're hitting Uno0001 errors:

"D:\a\Labs-Windows\Labs-Windows\CommunityToolkit.AllComponents.sln" (default target) (1:2) ->
"D:\a\Labs-Windows\Labs-Windows\components\DataTable\samples\DataTable.Samples.csproj" (default target) (6:39) ->
"D:\a\Labs-Windows\Labs-Windows\components\DataTable\samples\DataTable.Samples.csproj" (Build target) (6:51) ->
"D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\CommunityToolkit.WinUI.Controls.DataTable.csproj" (default target) (7:75) ->
  D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\Uno.UI.SourceGenerators\Uno.UI.SourceGenerators.XamlGenerator.XamlCodeGenerator\HeaderedItemsControl_3b991c3e61b47b1397f01a80384c3484.cs(158,6): error Uno0001: Windows.UI.Xaml.Controls.ItemsPresenter.FooterProperty is not implemented in Uno [D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\CommunityToolkit.WinUI.Controls.DataTable.csproj]
  D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\Uno.UI.SourceGenerators\Uno.UI.SourceGenerators.XamlGenerator.XamlCodeGenerator\HeaderedItemsControl_3b991c3e61b47b1397f01a80384c3484.cs(166,6): error Uno0001: Windows.UI.Xaml.Controls.ItemsPresenter.FooterTemplateProperty is not implemented in Uno [D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\CommunityToolkit.WinUI.Controls.DataTable.csproj]
  D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\Uno.UI.SourceGenerators\Uno.UI.SourceGenerators.XamlGenerator.XamlCodeGenerator\HeaderedItemsControl_3b991c3e61b47b1397f01a80384c3484.cs(174,6): error Uno0001: Windows.UI.Xaml.Controls.ItemsPresenter.HeaderProperty is not implemented in Uno [D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\CommunityToolkit.WinUI.Controls.DataTable.csproj]
  D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\Uno.UI.SourceGenerators\Uno.UI.SourceGenerators.XamlGenerator.XamlCodeGenerator\HeaderedItemsControl_3b991c3e61b47b1397f01a80384c3484.cs(182,6): error Uno0001: Windows.UI.Xaml.Controls.ItemsPresenter.HeaderTemplateProperty is not implemented in Uno [D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\CommunityToolkit.WinUI.Controls.DataTable.csproj]
  D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\DataTable\DataRow.cs(24,17): error Uno0001: Windows.UI.Xaml.Controls.ItemsPresenter.Header is not implemented in Uno [D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\CommunityToolkit.WinUI.Controls.DataTable.csproj]
  D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\DataTable\DataRow.cs(26,24): error Uno0001: Windows.UI.Xaml.Controls.ItemsPresenter.Header is not implemented in Uno [D:\a\Labs-Windows\Labs-Windows\components\DataTable\src\CommunityToolkit.WinUI.Controls.DataTable.csproj]

Oh, I didn't make this multi-target only UWP/WinAppSDK... maybe I should do that just for now?

@michael-hawker
Copy link
Member Author

Ran XAML Styler, missed some things when adding the tree test before apparently.

Also have a test case and logic working for auto-sized columns (doesn't work with resizing interaction yet). But seems to work in simple case.

Need to add a sample with more rows and varying length data width to see how it really works in practice. That's my next step I think.

@michael-hawker
Copy link
Member Author

Thinking to deal with the various padding options, I may be able to look at the position of the first item in the row in relation to the position of the DataTable column (with the positioning/coordinate APIs in the composition layer we have extensions for).

I think this would mean we could factor it in without having to worry about built-in container padding or developer padding, and possible would work well for TreeView and the indentation padding in the first row, we could get the other columns to ignore it after in the layout pass, maybe... Needs more investigation for that last bit.

Comment on lines +18 to +29
// TODO: Check with Sergio if there's a better structure here, as I don't need a Dictionary like ConditionalWeakTable
internal HashSet<DataRow> Rows { get; private set; } = new();

internal void ColumnResized()
{
InvalidateArrange();

foreach (var row in Rows)
{
row.InvalidateArrange();
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@Sergio0694 wanted to check in with you here, as my instinct was a ConditionalWeakTable for safety of reference holding. Though I have the DataRow class remove itself on Unloaded (though we know how that is in the XAML lifecycle). Right now, I was just trying to get the layout logic to work, and I think that's in a good place now.

Theoretically these should all be owned by the same parent, so not sure if that would cause problems with reference counts. Realizing though that I should probably have the DataRow grab a WeakReference to the parent too...

What are your thoughts on this connection? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm missing some context on who's keeping alive what. As in, if this is just some internal data structure for a control, I'm not necessarily seeing an issue with it tracking other things and objects it needs. Even if there's some reference cycle, the GC is completely able to handle that. Is there some global (static) state being involved here that's worrying you? Or, could you explain in simple terms what the relationship is between the various components here so I can better understand the scenario and what you're thinking might cause problems? 🙂

@michael-hawker
Copy link
Member Author

Alright, auto-sizing columns effectively works. Layout becomes a bit unstable when adjusting columns, so not sure how kosher that is. Not sure how we can account for that better though to fix. Performance seems fine though. I did try to debounce, but then it wasn't updating layout at all in-between, so it was either all or nothing it seems.

Looking into the auto-detect padding... that seems difficult as we can sometimes detect it using the composition layer coordinates positioning in the Arrange step, but that's pretty late. We'd have to invalidate arrange and re-layout possible for a third time (2nd from auto-sizing columns, though that gets triggered much earlier).

And I think the padding from TreeView is outside the DataRow itself, effectively a variable margin that appears on certain rows, and it didn't always appear detectable upon expansion of a node... So, definitely some tricky stuff there, still haven't really discovered what that space/width/margin is a part of in the construct of the TreeViewItem... That could help figuring out where it is to at least try and detect it more concretely, but then that's a special case for TreeView.

@michael-hawker
Copy link
Member Author

@Arlodotexe it's trying to run the WASM head on the package, but I turned the multi-target to just uwp/wasdk in the src project... or I have to update the sample one too, eh?

@michael-hawker
Copy link
Member Author

For now, manually added margin to DataTable in each sample to align columns between containers. We could introduce an Offset property maybe that when unset does some 'auto-detection'? Or maybe tries to lookup resource values (though that's tricky). Needs more thought, for now we can get the PoC in folks hands and gather feedback?

'Tree Table' is working now, which is really awesome!

Just should add the sticky header behavior to the virtualization sample for completeness.

@michael-hawker
Copy link
Member Author

@Arlodotexe I'm still missing something to make this work for only uwp/wasdk with the build... is that documented somewhere yet? I added multitarget props to the source and samples... thought that'd be it?

@michael-hawker
Copy link
Member Author

Stack trace from resizing a column too far to the right:

System.ArgumentOutOfRangeException: Non-negative number required.
Parameter name: width
   at Windows.Foundation.Rect..ctor(Double x, Double y, Double width, Double height)
   at CommunityToolkit.WinUI.Controls.DataTable.ArrangeOverride(Size finalSize) in H:\code\Labs-WCT\components\DataTable\src\DataTable\DataTable.cs:line 125

@Arlodotexe
Copy link
Member

Arlodotexe commented Jul 7, 2023

@Arlodotexe I'm still missing something to make this work for only uwp/wasdk with the build... is that documented somewhere yet? I added multitarget props to the source and samples... thought that'd be it?

Your MultiTarget files look good. At first glance, this appears related to CommunityToolkit/Tooling-Windows-Submodule#5, but we already have components that do this. It shouldn't be causing any issues here 🤔

@michael-hawker Just a hunch, try removing the MultiTarget.props from your sample project and let it inherit from the source project like we do with the others.

@michael-hawker
Copy link
Member Author

@michael-hawker Just a hunch, try removing the MultiTarget.props from your sample project and let it inherit from the source project like we do with the others.

Thought I had tried that... will see though on next go-around.

@michael-hawker
Copy link
Member Author

Waiting on CommunityToolkit/Windows#136

@michael-hawker
Copy link
Member Author

Thanks @Arlodotexe, just waiting for packages from Behaviors, then can apply local patch to add Sticky Header to the virtualization sample and we'll call this good for 'V1'... 🙂

@michael-hawker michael-hawker marked this pull request as ready for review July 17, 2023 22:51
@michael-hawker
Copy link
Member Author

michael-hawker commented Jul 17, 2023

Sticky Header works, though not perfect as default style is Transparent, added a border for now (which was a cool scenario to test anyway), and it works fine.

There'll be more clean-up to do with this and how it works in the future anyway.

@niels9001 suggested a helper maybe for the items panel to fade the top away on scrolling with some sort of gradient brush maybe... something to investigate later.

So this is ready to go then otherwise, just needs a sign-off.

@Arlodotexe
Copy link
Member

Arlodotexe commented Jul 19, 2023

I'm seeing this error in the console on WASM. UWP and WinAppSDK seem okay.

Unhandled Exception:
put_char @ dotnet.js:2360
dotnet.js:2360 System.Collections.Generic.KeyNotFoundException: The given key 'ListViewTableSample' was not present in the dictionary.
put_char @ dotnet.js:2360
dotnet.js:2360    at System.Collections.Generic.Dictionary`2[[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[CommunityToolkit.Tooling.SampleGen.Metadata.ToolkitSampleMetadata, CommunityToolkit.Tooling.SampleGen, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].get_Item(String key) in D:\a\Uno.DotnetRuntime.WebAssembly\Uno.DotnetRuntime.WebAssembly\runtime\src\libraries\System.Private.CoreLib\src\System\Collections\Generic\Dictionary.cs:line 199
put_char @ dotnet.js:2360
dotnet.js:2360    at CommunityToolkit.App.Shared.Renderers.ToolkitDocumentationRenderer.LoadData() in D:\source\uwp\lib\Labs-Windows\tooling\CommunityToolkit.App.Shared\Renderers\ToolkitDocumentationRenderer.xaml.cs:line 110
put_char @ dotnet.js:2360
dotnet.js:2360    at CommunityToolkit.App.Shared.Renderers.ToolkitDocumentationRenderer.OnMetadataPropertyChanged(DependencyObject dependencyObject, DependencyPropertyChangedEventArgs args) in D:\source\uwp\lib\Labs-Windows\tooling\CommunityToolkit.App.Shared\Renderers\ToolkitDocumentationRenderer.xaml.cs:line 85
put_char @ dotnet.js:2360
dotnet.js:2360    at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state) in D:\a\Uno.DotnetRuntime.WebAssembly\Uno.DotnetRuntime.WebAssembly\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1917
put_char @ dotnet.js:2360
dotnet.js:2360    at System.Threading.QueueUserWorkItemCallbackDefaultContext.Execute() in D:\a\Uno.DotnetRuntime.WebAssembly\Uno.DotnetRuntime.WebAssembly\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\ThreadPoolWorkQueue.cs:line 976
put_char @ dotnet.js:2360
dotnet.js:2360    at System.Threading.ThreadPoolWorkQueue.Dispatch() in D:\a\Uno.DotnetRuntime.WebAssembly\Uno.DotnetRuntime.WebAssembly\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\ThreadPoolWorkQueue.cs:line 724
put_char @ dotnet.js:2360
dotnet.js:2360    at System.Threading.ThreadPool.Callback() in D:\a\Uno.DotnetRuntime.WebAssembly\Uno.DotnetRuntime.WebAssembly\runtime\src\mono\System.Private.CoreLib\src\System\Threading\ThreadPool.Browser.Mono.cs:line 119
put_char @ dotnet.js:2360
dotnet.js:2360 ThreadPool Callback threw an unhandled exception of type System.Collections.Generic.KeyNotFoundException

image

@michael-hawker
Copy link
Member Author

michael-hawker commented Jul 19, 2023

@Arlodotexe these don't work on WASM... so expected - see multi-target. They just show up as we have that other issue open: CommunityToolkit/Tooling-Windows-Submodule#5

@michael-hawker
Copy link
Member Author

Our transient test failure again... will re-run once others done. Probably something need to investigate later.

@michael-hawker
Copy link
Member Author

Test are constantly dying after 3 attempts on this one, not sure why. Opened up a runner issue to start a conversation as that's the only thing I can think that's changed (as we haven't modified our tests themselves in the last while): actions/runner-images#7937

Only extra change on this PR is the upgraded package version of the extensions, though we've been seeing this test failure before that was released...

michael-hawker and others added 20 commits August 1, 2023 14:45
…t supposed to work...)

Will have to see how it runs in the CI and after I've rebooted VS...
Invalidate Arrange of children when column autosizes - auto size works!
Test virtualization with more items, works too!
…tainer... need to investigate, adding a TODO comment for now
…taTable

Looks explicitly for the MultiSelectGrid which drives the indentation/gutter size for TreeViewItem
Add manual margins to samples for alignment between different containers
Add more depth to tree table example
For now add a solid background, would be nice to have a fadebehavior on the items panel to fade behind the sticky header that's transparent still...
…rios

Fixes a bug/issue with a Row's Height not being properly calculated, which was not only pivotal for this scenario but would have caused issues in practical usage of all other scenarios as well.
@michael-hawker
Copy link
Member Author

Alright, rebased on the clean tooling build/infrastructure we have which is building on main fine, so hopefully we should be all clean here too (I've updated the package dependencies in the DataTable component to match).

I also took the time while re-testing locally to add the requested "Blank" header sample to the docs/examples. It helped exposed a bug with improper DataRow Height calculation which would of effected other scenarios as well for content larger than the column size, this should be resolved now and enable those scenarios properly.

Once this builds clean, it should be merged and a new package pushed for folks to try more easily! 🎉

@michael-hawker michael-hawker merged commit e7a28e0 into main Aug 2, 2023
@delete-merged-branch delete-merged-branch bot deleted the llama/datatable branch August 2, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experiment 🧪 Used to track issues that are experiments (or their linked discussions)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants