🚧 SizerBase class for GridSplitter + new ContentSizer and PropertySizer components #4083
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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
requested review from
ghost
added
bug 🐛
|
Ugh forgot to add headers, will fix in a min |
michael-hawker
force-pushed
the
user/mhawker/gridsplitter
branch
4 times, most recently
from
543791e to
99b8ae5
Compare
8 tasks
michael-hawker
force-pushed
the
user/mhawker/gridsplitter
branch
from
99b8ae5 to
cfc2c57
Compare
This was referenced Jan 21, 2022
michael-hawker
force-pushed
the
user/mhawker/gridsplitter
branch
from
cfc2c57 to
95006fb
Compare
|
Filed #4500, looks like there's an underlying issue with our CI at the moment. I built this locally with no issues before pushing up. |
michael-hawker
changed the title
Content Sizer Issues: - [ ] TargetControl isn't loaded yet causes problems (Expander Issue) - [ ] Size Direction needs to be more specific (only works Left/Top, not Right/Bottom) - [ ] Do we support 'Auto'? - [ ] Need to set Automation Property Name in Code-Behind - [ ] Content Initial value as binding converter to ResizeDirection?
Bonus also fixes issue with sample loading the first time...
Removed now unused SplitterCursorBehavior property
…en GridSplitter and ContentSizer - Share XAML template between all controls - Includes sharing VSM and logic to control that - Centralized control manipulation logic to base class - Cleaned-up Keyboard handling code to be shared in base class using OnKeyDown override - Move OnManipulationDelta to SizerBase - Share Automation Name and setup via XAML vs. code-behind - Renamed ResizeDirection to Orientation in the base for ContentSizer and future controls, synced from GridSplitter with original ResizeDirection for polyfill (removed unneeded ContentSizerDirection/SizerResizeDirection enum as using system Orienation now) TODO: - Update/Fix AutomationPeer to generalize - Provide OnLoaded virtual event? - Figure out Content/ContentTemplate setup do we want this flexibility? Do we just have Content which is set to the TextBlock in Style as Default instead? - Create Converter to set content for horizontal/vertical text in XAML (bound to Orientation) - Update GripperCursor in Orientation changed callback - Do we try inheriting from Thumb? Does that automatically solve the mouse drag back issue? - Audit Sizer/Splitter/Gripper/Thumb naming???
Rename Horizontal/VerticalMove methods to prefix with On
… own files in the partial class. Move IsDragInverted to ContentSizer as it is specific to that class for now.
…er all use Cumulative change to update sizes vs. Delta This fixes long-standing issue where mouse would de-sync from gripper bar and provide a disorienting experience to users. Now the bar only moves when the mouse is over it and ignores extra movement - the same as Thumb controls do within a Slider for instance.
…erBase controls. Fix issue where we broke keyboard with cumulative change fix in last commit Remove GripperKeyboardChange constant Guard against keyboard input during drag event. Add more comments on abstract methods and behavior in SizerBase
Removed Content/ContentTemplate for now from SizerBase, may want to add back in the future. Simplified template to include TextBlock and use converter to setup glyphs Provided more keys for manipulating resource values in template
Fixes issue with ContentSizer Horizontal mouse cursor being incorrect until drag Clean-up Cursor setting code to be handled in property changed callback of Orientation instead Will aid in the future if we get a Cursor property in WinUI 3 for ProtectedCursor, one spot to change only.
…used GridSplitter sample page/code-behind
Works amazing, first try!
…ill with new base class)
michael-hawker
force-pushed
the
user/mhawker/gridsplitter
branch
from
95006fb to
0ac40ef
Compare
2 tasks
michael-hawker
added a commit
to CommunityToolkit/Labs-Windows
that referenced
this pull request
May 5, 2022
michael-hawker
added a commit
to CommunityToolkit/Labs-Windows
that referenced
this pull request
May 6, 2022
michael-hawker
added a commit
to CommunityToolkit/Labs-Windows
that referenced
this pull request
May 10, 2022
michael-hawker
added a commit
to CommunityToolkit/Labs-Windows
that referenced
this pull request
May 11, 2022
michael-hawker
added a commit
to CommunityToolkit/Labs-Windows
that referenced
this pull request
May 12, 2022
michael-hawker
commented
May 12, 2022
Comment on lines
+21
to
+27
| DependencyProperty.Register(nameof(HorizontalValue), typeof(object), typeof(TypeToObjectConverter), new PropertyMetadata(null)); | ||
|
|
||
| /// <summary> | ||
| /// Identifies the <see cref="VerticalValue"/> property. | ||
| /// </summary> | ||
| public static readonly DependencyProperty VerticalValueProperty = | ||
| DependencyProperty.Register(nameof(VerticalValue), typeof(object), typeof(TypeToObjectConverter), new PropertyMetadata(null)); |
There was a problem hiding this comment.
Need to fix typeof() here referencing TypeToObjectConverter (found in Labs via crash on Skia (go Uno)). Fixed in labs.
|
Moved this over to labs for now, so this may just get closed out and we'll re-import later for 8.0 via labs restructure/future work there. |
michael-hawker
added a commit
to CommunityToolkit/Labs-Windows
that referenced
this pull request
May 12, 2022
25 tasks
|
This is now an experiment in Toolkit Labs (see #4487 for info on Labs). It can be found at the following Experiment: CommunityToolkit/Labs-Windows#101 Closing this PR out now as we'll re-integrate from Labs later for 8.0. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2976 #2017 #3949
The end goal of this PR is to have a base component which both ContentSizer and GridSplitter inherit from. It will also simplify the GridSplitter component to not have a sub-component for controlling the mouse-cursor behavior and instead use our mouse cursor extensions. This should resolve #2017. This also allows us to properly implement GridSplitter cursor behavior for WinUI 3 as it should be owned by the component itself and we can just use
ProtectedCursorinstead of our cursor extensions.The end goal is that GridSplitter will be functionally equivalent to the one within the toolkit, though technically from a component/style standpoint this is a breaking change for 8.0. Updating should be straight-forward unless using some properties we've removed/changed around style/cursors which have been moved to the XAML template.
Overview of new SizerBase class
This PR introduces a new base class
SizerBase(name to be finalized) which centralizes the logic for 'splitter/sizer/gripper' type controls likeGridSplitter. The base class handles the following:Cursorhandling using our Mouse extension (also one line change for WinUI 3 when we port forProtectedCursor)OrientationToObjectConverterto handle character change based on OrientationRightToLeftflow handling (🆕 fixed for keyboard, unknown bug)IsEnabledstates and aligns toThumbstates)The subclasses like
GridSplitterthen only need to implement the abstract methods providedOnLoaded,OnDragStarting,OnDragHorizontal, andOnDragVerticalto apply the requested movement amount to the element being manipulated, e.g. aGrid. This makes the sub-class controls very simple and flexible and easier to follow logic-wise.New Controls: ContentSizer and PropertySizer
On top of this new refined base class, we introduce two new controls:
ContentSizercontrol is a more flexible/generic version of ourGridSplittercontrol. It provides a bar which can then control the size of anyTargetelement. This can be used to make more complex UIs and things like collapsible trays with theExpandercontrol. It's actually the component I use to remember the Sidebar position within XAML Studio.PropertySizercontrol is even more generalize and a little more niche, but allows the manipulation of anydoubleproperty via aTwoWaybinding to that property (either another UI Element or even a ViewModel). This makes it super simple for example to manipulate aNavigationView'sOpenPaneLengthproperty to create a customized sidebar experience for NavigationView with no further modifications, this is the sample provided in our sample app. This addresses Allow app users to resize NavigationView's Pane microsoft/microsoft-ui-xaml#190.PR Type
What kind of change does this PR introduce?
What is the current behavior?
Grid Splitter is a complex component made of a separate piece which controls mouse cursor behavior. It is also restricted for use within a Grid.
What is the new behavior?
Grid Splitter is now based off a new base class which provides the look-and-feel and mouse behavior with our mouse extensions API. A new ContentSizer and PropertySizer also inherit from this base to provide a more generalized solution for other types of UI scenarios.
Note: The underlying logic for how GridSplitter works with a
Gridhas been left unchanged in this PR (for the most part - outside of updating it to use Cumulative values vs. deltas basically). If we want to adopt the WPF logic specifically, I suggest we do that afterwards as a separate PR, though note the bug note below as well. We can open an issue later.PR Checklist
Please check if your PR fulfills the following requirements:
List of Work to be Done
TargetControlloading issues? - Haven't noticed with latest incarnations of components with shared base class.SizerBar,SplitterBar,SlidingBar,HandleBar?) so can be used for GridSplitterCursorproperty can be provided as a custom one without interferenceGripperBarBase,ContentGripperBar, andPropertyGripperBarinstead? (GridSplitteris a polyfill so remains the same.) Or some other naming scheme?Any suggestions on the name for the common base component?