GridSplitter: Make IsTabStop work and simplify control's visual tree #1145

lukasf · 2017-05-03T15:06:26Z

This is to resolve #1143.

GridSplitterGripper was removed, keyboard events are now taken on top level control. I have checked that the control behaves exactly the same, including custom gripper content and "auto content" based on alignment.

…ify visual tree.

skendrot · 2017-05-03T19:34:58Z

Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.Events.cs

@@ -22,30 +24,22 @@ namespace Microsoft.Toolkit.Uwp.UI.Controls
    /// </summary>
    public partial class GridSplitter
    {
+        // Symbols for GripperBar in Segoe MDL2 Assets
+        private const string GripperBarVertical = "\xE784";


These styles should be applied within the control style and visual states instead of here.

It would be possible to move the gripper content into the control template and use visual states for selecting the right content. But then it would always be shown and we would lose the functionality of easily overriding the gripper content by using the "Element" property. This would be a breaking change. We would have to remove the Element property, and our users would instead have to override the control template for custom content. I do not think that this is a good idea.

If the control allows for content then it should be a ContentControl. I would suggest changing to a ContentControl, mark Element as deprecated and when it changes set the content to be the value. Default the content to be the style proposed

I tend to agree with Shawn it should have been a content control from the beginning (but of course not my bad :P )
We can do it right with a small change, just moving the textbox to the grid inside the control template and set the textbox value based on orientation on splitter loaded.
I see no need to move GripperBarVertical to visual state as it should be used only twice once on splitter loaded and when the splitter orientation change to update the gripper with the new value (which is not currently implemented)

Let's do the right thing :)

The nearest case for the splitter is Button control wish is a ContentControl that allows custom content to change the way it looks :)
Anyway, I see we can consider moving the splitter from using Element to ContentControl in V2 so we don't have to use DEPRECATED keyword with a change that doesn't add extra functionality.
Will review the PR this weekend.

I don't see it like that. A button's main purpose is to show content and allow you to click it. Each button in your UI will usually show different content. So unlike the GridSplitter, showing custom content is inherent part of the button's functionality. Much like a checkbox, which is there to show content and allow you to check it. The gripper is more like the checkmark of a checkbox. It is part of the style which is usually the same for all instances of the control. It is not like the content of a checkbox.

But well, just because I don't think it should be a ContentControl, that surely does not mean you cannot make it one :) It's just my opinion on this.

@skendrot It would be great if you could approve the PR. Please see the discussion above. If someone wants to do a redesign of the GridSplitter control, please create a separate issue/PR.

@skendrot I juse see that @IbraheemOsama has already created an issue for changing the control to ContentControl. Then my PR should be ready to go?

I'm fine with waiting to change the control. However:
If we plan to keep the concept that it will allow custom content (via the Element property or by making it a ContentControl), then let's embrace that now. Instead of having an internal textblock, set that to be the Element property and change the text based on a visual state.

Or do we want to remove the capability to allow custom content (eg: Remove Element property and do not change to be a ContentControl), which I think is also a valid solution and is how the WPF GridSplitter behaved. This option would probably need to be done in the 2.0 release.

IbraheemOsama · 2017-05-06T03:22:37Z

Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.Events.cs

-            var gripper = sender as GridSplitterGripper;
-
-            if (gripper == null)
+            if (_gripperDisplay == null)


No need for this check as _gripperDisplay will always be null if this function is reached

The Loaded event can fire multiple times. It is called each time a control is (re-)added to the visual tree. For example if you use a Frame with page caching and navigate to a different page, the old page content will get Unloaded event and then is put into cache. When moving back to the old page, it is re-added to the visual tree and so it will get Loaded event again, for the second time. In that case, our _gripperDisplay already exists, no need for us to re-create it.

IbraheemOsama · 2017-05-06T03:25:03Z

Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.Events.cs

+                    HorizontalAlignment = HorizontalAlignment.Center,
+                    VerticalAlignment = VerticalAlignment.Center,
+                    Foreground = GripperForeground,
+                    Text = gripperText


I see this variable can be replace with it's value directly, no need to create a new variable.

If expressions get too long, I sometimes like to introduce a separate variable to keep lines shorten. But I will chage as you suggested.

IbraheemOsama · 2017-05-06T03:32:40Z

Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.Events.cs


+        private void GridSplitter_KeyDown(object sender, KeyRoutedEventArgs e)


What about override on key down instead of registering new event ?
`
protected override void OnKeyDown(KeyRoutedEventArgs e)
{
var step = 1;
var ctrl = Window.Current.CoreWindow.GetKeyState(VirtualKey.Control);
if (ctrl.HasFlag(CoreVirtualKeyStates.Down))
{
step = 5;
}

if (_resizeDirection == GridResizeDirection.Columns) { if (e.Key == VirtualKey.Left) { HorizontalMove(-step); } else if (e.Key == VirtualKey.Right) { HorizontalMove(step); } else { return; } e.Handled = true; return; } if (_resizeDirection == GridResizeDirection.Rows) { if (e.Key == VirtualKey.Up) { VerticalMove(-step); } else if (e.Key == VirtualKey.Down) { VerticalMove(step); } else { return; } e.Handled = true; } base.OnKeyDown(e); }

`

Sure, I can do that.

IbraheemOsama · 2017-05-06T03:36:05Z

Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.cs

@@ -174,9 +175,11 @@ protected override void OnApplyTemplate()

            // Unhook registered events
            Loaded -= GridSplitter_Loaded;
+            KeyDown -= GridSplitter_KeyDown;


This should be removed in favor of overriding Keydown event

IbraheemOsama · 2017-05-06T03:36:17Z

Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.cs


            // Register Events
            Loaded += GridSplitter_Loaded;
+            KeyDown += GridSplitter_KeyDown;


same as above

IbraheemOsama · 2017-05-06T03:40:28Z

Microsoft.Toolkit.Uwp.UI.Controls/GridSplitter/GridSplitter.Options.cs

@@ -180,10 +181,10 @@ public SplitterCursorBehavior CursorBehavior
        private static void OnGripperForegroundPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
        {
            var gridSplitter = (GridSplitter)d;
-            var grip = gridSplitter.Element as GridSplitterGripper;
+            var grip = gridSplitter._gripperDisplay;


Can we change variable grip to gripper to match the rest of the control ?
I know it is an old code :)

IbraheemOsama · 2017-05-13T11:00:33Z

@lukasf LGTM but method OnKeyDown is missing it's comment, you can use
/// <inheritdoc />

lukasf · 2017-05-16T19:26:04Z

@IbraheemOsama Thanks, I added the missing comment.

nmetulev · 2017-05-28T23:02:10Z

@skendrot, now that we created a new issue for changing the control to ContentControl, anything missing in the PR before you sign off?

skendrot · 2017-05-30T15:39:11Z

Sorry. It looks like I typed a comment but never submitted it!

I'm fine with waiting to change the control. However:
If we plan to keep the concept that it will allow custom content (via the Element property or by making it a ContentControl), then let's embrace that now. Instead of having an internal textblock, set that to be the Element property and change the text based on a visual state.

Or do we want to remove the capability to allow custom content (eg: Remove Element property and do not change to be a ContentControl), which I think is also a valid solution and is how the WPF GridSplitter behaved. This option would probably need to be done in the 2.0 release.

lukasf · 2017-05-30T18:48:53Z

@skendrot Can we please move the discussion on how to handle Content (now and in future) to the new issue submitted by @IbraheemOsama and close this PR? I am just trying to fix a problem with the control. I did not invent the Element stuff, and I did not change how it is done. It was an internal TextBlock before and it still is now, i just moved that code to a different place. I understand that you guys want to redesign that stuff, please go ahead, a separate issue has already been created for that.

skendrot · 2017-05-30T19:27:51Z

You're right @lukasf.

lukasf · 2017-05-30T19:36:09Z

Thank you.

Remove GridSplitterGripper to allow control of tab behavior and simpl…

9226023

…ify visual tree.

dnfclas added the cla-already-signed label May 3, 2017

nmetulev requested a review from IbraheemOsama May 3, 2017 17:50

skendrot suggested changes May 3, 2017

View reviewed changes

IbraheemOsama suggested changes May 6, 2017

View reviewed changes

GridSplitter: Rework according to review commments.

3e6070d

IbraheemOsama approved these changes May 15, 2017

View reviewed changes

GridSplitter: Add missing comment

e63ba1e

skendrot approved these changes May 30, 2017

View reviewed changes

nmetulev merged commit 29e9f8b into CommunityToolkit:dev May 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GridSplitter: Make IsTabStop work and simplify control's visual tree #1145

GridSplitter: Make IsTabStop work and simplify control's visual tree #1145

lukasf commented May 3, 2017

skendrot May 3, 2017

lukasf May 3, 2017

skendrot May 3, 2017

IbraheemOsama May 3, 2017

IbraheemOsama May 3, 2017

IbraheemOsama May 5, 2017

lukasf May 5, 2017

lukasf May 16, 2017

lukasf May 16, 2017

skendrot May 16, 2017

IbraheemOsama May 6, 2017

lukasf May 7, 2017

IbraheemOsama May 6, 2017

lukasf May 7, 2017

IbraheemOsama May 6, 2017

lukasf May 7, 2017

IbraheemOsama May 6, 2017

lukasf May 7, 2017

IbraheemOsama May 6, 2017

lukasf May 7, 2017

IbraheemOsama May 6, 2017 •

edited

Loading

lukasf May 7, 2017

IbraheemOsama commented May 13, 2017 •

edited

Loading

lukasf commented May 16, 2017

nmetulev commented May 28, 2017

skendrot commented May 30, 2017

lukasf commented May 30, 2017

skendrot commented May 30, 2017

lukasf commented May 30, 2017


		private void GridSplitter_KeyDown(object sender, KeyRoutedEventArgs e)

GridSplitter: Make IsTabStop work and simplify control's visual tree #1145

GridSplitter: Make IsTabStop work and simplify control's visual tree #1145

Conversation

lukasf commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IbraheemOsama May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IbraheemOsama commented May 13, 2017 • edited Loading

lukasf commented May 16, 2017

nmetulev commented May 28, 2017

skendrot commented May 30, 2017

lukasf commented May 30, 2017

skendrot commented May 30, 2017

lukasf commented May 30, 2017

IbraheemOsama May 6, 2017 •

edited

Loading

IbraheemOsama commented May 13, 2017 •

edited

Loading