Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Redesign of GridSplitter API #2976

Closed
verelpode opened this issue Aug 1, 2019 · 6 comments
Closed

Redesign of GridSplitter API #2976

verelpode opened this issue Aug 1, 2019 · 6 comments
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ feature request 📬 A request for new changes to improve functionality help wanted Issues identified as good community contribution opportunities
Milestone

Comments

@verelpode
Copy link

I'm opening this issue to discuss problems with GridSplitter, as requested by @michael-hawker. Actually I presumed that GridSplitter was only in the Community Toolkit and not officially in UWP because the WinUI team already knew of problems; already knew that changes to GridSplitter's API are desirable, but yes it's good to open a new issue and get feedback from multiple people.

Currently 2 versions of GridSplitter exist:

Both of those versions have mostly the same API/design. My opinion is that this design is problematic from a programming point of view. From a user point of view, GridSplitter is good except when it malfunctions (or is misconfigured) and produces mangled GUI visible to the user.

One of the problems is what happens when you try to limit the user to a minimum pane size without breaking compatibility with a small app window size. It's very easy to accidentally configure GridSplitter incorrectly, and even if you do configure it correctly, it still does not behave well when the user resizes the window to a small size.

In my opinion, in order to make a rock solid alternative to GridSplitter, it should be made independent of Grid -- it should not use Grid. It can potentially use Grid internally if desired, but not in the public API.

In closed issue #465, @SeriousGeorge wrote:

For example if you have like 10 columns and adjust one in width.. each column/row after the current column modified in width, should shift as well.

@SeriousGeorge raised a valid issue, but I think his issue was closed because unfortunately he mentioned "a timeline control" and this confused the issue, but nevertheless his core point was valid. If people still don't understand what he said, then I'll make screenshots or diagrams to explain the problem and solution.

In closed issue #1666, @wstaelens experienced difficulty configuring GridSplitter. Even if his case is fully analyzed and determined to be his mistake and not a bug in GridSplitter, it still indicates problems with the design of GridSplitter. It's far too easy to misconfigure GridSplitter. The design of GridSplitter can be simplified and improved in a manner that eliminates this problem of GridSplitter being too difficult to configure. (And as I said, problems still exist even after configuration mistakes are fixed.)

To adequately describe the GridSplitter problems, I think I'll have to write some sample code that demonstrates problems, and also post a couple of screenshots of GUI mangled by GridSplitter. I won't be able to get around to this task this week, but in the meantime, people can start posting their feedback/experiences with using GridSplitter in Universal Windows apps, and I'll also post more detail next week or when I get the chance.

Another important question: Touchscreens. In regards to GridSplitter in Community Toolkit not WPF, are people satisfied or dissatisfied with its compatibility/behavior when operated by users on touchscreen-based devices that have no mouse?

See also: "Allow app users to resize NavigationView's Pane" #190

@ghost ghost added the needs triage 🔍 label Aug 1, 2019
@michael-hawker michael-hawker added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ labels Aug 1, 2019
@verelpode
Copy link
Author

In WinUI #190, @YuliKl wrote:

Provide minimum and maximum open pane widths to limit user preferences within a range

I suggest that it would be best to apply YuliKl's wording literally. Limit the user but not the programmer. Also don't limit the internals of the new splitter Control to these min and max pane sizes. The new Control can be given guideline/suggestion limits and/or user limits, but it needs the ability to ignore or modify these limits when it is impossible to apply them, depending on window size etc.

Currently GridSplitter is limited by Windows.UI.Xaml.Controls.ColumnDefinition.MinWidth, ColumnDefinition.MaxWidth, RowDefinition.MinHeight, etc. These limits are enforced upon all stakeholders:

  • Users
  • Programmers
  • Internals of GridSplitter
  • Internals of Grid

ColumnDefinition.MinWidth etc are OK for the purposes of Grid but not for the purposes of GridSplitter. The enforcement of these limits upon GridSplitter sometimes triggers mangled GUI , such as when insufficient space is available.

In order to perform well in all circumstances, GridSplitter needs to be given more freedom and flexibility, but it lacks this freedom/flexibility because ColumnDefinition.MinWidth etc are forced upon GridSplitter by Grid. The size limits of resizable panes should apply only to users.

@michael-hawker
Copy link
Member

Thanks @verelpode, I think GridSplitter is an important concept still when used specifically for a Grid. However, I do agree there are other scenarios for resizing (like with a NavigationView) that it is not really built for.

In my XAML Studio app, I ran into this type of problem recreating VS Code's Activity Bar/Side Bar area and being able to allow the user to resize it. For that I created a new Splitter called ContentSizer which looked like the GridSplitter does now, but instead controls the sizing of its parent's container. Therefore you can do things like have it remember sizing when it gets collapsed in the visual tree and such.

My plan has been to refactor a common base for GridSplitter and ContentSizer in the toolkit, as I made improvements to use the mouse pointer code we have in the toolkit now when I wrote ContentSizer; however, I haven't had time yet.

@michael-hawker michael-hawker added feature request 📬 A request for new changes to improve functionality and removed needs triage 🔍 labels Aug 2, 2019
@michael-hawker michael-hawker added this to the 6.0 milestone Aug 2, 2019
@michael-hawker
Copy link
Member

I'll put this in the 6.0 milestone for now. ContentSizer is super simple compared to the GridSplitter we have now, so maybe as a first step it just makes sense to add that in. Then it'd also be easier for someone else to pick up the GridSplitter refactor.

@michael-hawker michael-hawker added the help wanted Issues identified as good community contribution opportunities label Aug 2, 2019
@verelpode
Copy link
Author

have it remember sizing when it gets collapsed in the visual tree and such.

Oh yes, that's another relevant issue I forgot to mention. I also encountered this issue. I think this issue should be included in a new design of a splitter.

I think GridSplitter is an important concept still when used specifically for a Grid.

In regards to backwards compatibility. In regards to new apps, I don't think any kind of splitter widget should be publicly placed in a Grid, regardless of whether it is the existing GridSplitter or a new version with the same name "GridSplitter". I think a widget/Control for resizing columns and rows of Grid (or any other grid-like element) shouldn't exist at all. I think the whole idea of splitters for rows + columns is too unreliable and should be abandoned.

I think, in order to make it reliable, the concept needs to be changed to horizontal and vertical orientation like how StackPanel works, not rows + columns anymore. I realize that people immediately respond to this idea by saying that some apps need a scenario with both horizontal and vertical splitters in the same window therefore it must be a grid. But that's the knee-jerk answer. Think further. The scenario is still supportable even without using a grid, because you can nest elements similar to how a horizontal StackPanel can be placed inside a vertical StackPanel or vice-versa.

This means the whole splitter concept can be radically simplified, without losing the ability to have both horizontal and vertical splitters in the same window. Because the new concept is simpler, it's much more feasible to make it completely reliable, user-friendly, and programmer-friendly.

In contrast, the existing design of GridSplitter is excessively and unnecessarily complex, and nobody inside nor outside of Microsoft is smart enough to figure it out and untangle this mess. The existing design of GridSplitter is a mess that will never be fully reliable. If the public API is unchanged, GridSplitter's implementation is impossible to repair, because of unavoidable conflicts with the rules of Grid such as the rigid enforcement of ColumnDefinition.Min/MaxWidth etc.

I've watched people trying to untangle programming messes with roughly the same amount of entanglement as GridSplitter, and they fall into the trap of thinking, "I have 25 years experience! I can do this!", but it's like watching someone trying to untangle 300 meters of accumulated CAT6 cabling in the crawl-space instead of ripping it all out and laying new cable. It's like watching a beefy champion weight-lifter attempting the sport of rock climbing and then being thoroughly beaten by a female gymnast with spindly arms.

GridSplitter was a good version 1 and it was valuable for teaching us lessons that we wouldn't have learned otherwise, so thank you GridSplitter for teaching us those valuable lessons, but now those lessons are learned and it's time to use this acquired knowledge to make a better + simpler + cleaner design.

@michael-hawker michael-hawker mentioned this issue Aug 23, 2019
27 tasks
@michael-hawker michael-hawker modified the milestones: 6.0, 6.1 Oct 24, 2019
@michael-hawker michael-hawker modified the milestones: 6.1, 7.0 Jun 23, 2020
@michael-hawker
Copy link
Member

Linking back to discussions on details of GridSplitter from this unmerged PR: #2018

When I get to this work (Aug/Sept?), I'll start with the common base between ContentSizer and abstracting the modification logic and reusing the visuals for a new GridSplitter, then we can hopefully reworks some logic and clarify what needs to be done where.

@verelpode @skendrot would you be interested in helping to summarize a distinct list of scenarios you'd like to see GridSplitter handle? I'm thinking akin to what the WinUI folks do in their issue templates where there's a table of items of basic requirements for things we expect to work, stretch goals, and things that are explicitly out of scope?

It'd be nice to have some nice upgrade path for existing GridSplitter users, but 7.0 will be a major release, so we can make some breaking changes if needed. E.g., our start is:

Capability Priority
... Must
Developers using WCT GridSplitter can easily update their code Should
Developers using WCT GridSplitter can just upgrade directly Could
Developers using WPF GridSplitter can leverage this component easily (not sure what this story is today?) Could

@michael-hawker
Copy link
Member

Linking to #2719 as well

@michael-hawker michael-hawker modified the milestones: 7.1, 7.2/8.0? Aug 31, 2021
@CommunityToolkit CommunityToolkit locked and limited conversation to collaborators Jul 29, 2022
@LalithaNadimpalli LalithaNadimpalli converted this issue into discussion #4672 Jul 29, 2022
@ghost ghost removed the In-PR 🚀 label Oct 19, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ feature request 📬 A request for new changes to improve functionality help wanted Issues identified as good community contribution opportunities
Projects
Status: Done
2 participants