Add Split Widget

[stuartmscott]

Description:

Adds widget for splitting an area between two objects with an adjustable divider.

Fixes #205

Demo

Checklist:

• [y] Tests included.
• [y] Lint and formatter run with no errors.
• [y] Tests all pass.

Where applicable:

• [y] Public APIs match existing style.
• [n/a] Any breaking changes have a deprecation path or have been discussed.

[andydotxyz]

You’re on fire! This looks really good. I had a quick look and I’m not sure this widget can really be created using the &...{} syntax as the key fields are private. Is this for a reason?

[stuartmscott]

No particular reason, fields are exported now

[andydotxyz]

A great feature and well written code thanks. Perhaps this is an opportunity to add your name to AUTHORS as well?

My main question (aside from small comments inline) is around initial placement. I didn’t see tests for that I don’t think but through reading the code I guess it aims for split down the middle? Is there some way that the api could support setting the position? It opens up 2 questions 1) does a 0 offset being central match what people would expect (range is -(width/2-childAwidth) to +(width/2-childBwidth) if I understand correctly, and 2) when resizing I guess the space is split equally on each side, is that the desired approach or an outcome of the current offset maths?

Sorry, mostly open ended questions on this one.

[stuartmscott]

Thanks @andydotxyz !!

I didn’t see tests for that I don’t think but through reading the code I guess it aims for split down the middle?

Correct, the offset is from the middle so a negative offset shrinks the leading child and expands the trailing child within the limits you mentioned. TestSplitContainer creates a splitter with two rectangles and resizes it, then checks that both rectangles have the same size so the space is evenly shared.

Is there some way that the api could support setting the position?

func (s *SplitContainer) updateOffset(offset int) could be exported additionally if needed NewXSplitContainer could accept an initial offset.

1. does a 0 offset being central match what people would expect (range is -(width/2-childAwidth) to +(width/2-childBwidth)

The two options here are 0 == center and 0 == (top or left). The later maybe closer to how scroller works, but when you first show a scroller you expect it to be at the top left. However when you first show a splitter it would be confusing if one was bigger than the other, you'd then need another API to say "split evenly", or an API to assign weights to each child. 0 == center is the least surprising to me - I create a splitter with two children and I see an area split evenly between the two children. I can drag it to change it and if updateOffset get exported then I could change it programmatically and maybe attach it to setting/preferences to remember where I like it.

2. when resizing I guess the space is split equally on each side, is that the desired approach or an outcome of the current offset maths?

I think both - should the new space only go to one child?

[stuartmscott]

@Jacalz hit this again on Travis Windows go1.14:

--- FAIL: TestWatchFile (0.15s)
settings_test.go:92: File watcher callback was not called
--- FAIL: TestFileWatcher_FileDeleted (0.25s)
settings_test.go:117: File watcher callback was not called
FAIL

[andydotxyz]

TestSplitContainer creates a splitter with two rectangles and resizes it, then checks that both rectangles have the same size so the space is evenly shared.

That is really testing MinSize layout though, I think it's missing a test for "initial layout with extra space".

func (s *SplitContainer) updateOffset(offset int) could be exported additionally if needed NewXSplitContainer could accept an initial offset.

To support setting with a struct &...{} setup maybe it's easier to just to expose offset?

However when you first show a splitter it would be confusing if one was bigger than the other

That depends on the use-case... For a file splitter with 2 edit panes I agree... but for a list-detail view (like an email app) it's probably not true. These conflict also in the expectations on resizing I think. (thinking out loud here) would it benefit from a "OffsetAlign" or similar flag so we know whether space should be allocated to leading, trailing or both?

[Jacalz]

@Jacalz hit this again on Travis Windows go1.14:

--- FAIL: TestWatchFile (0.15s)
settings_test.go:92: File watcher callback was not called
--- FAIL: TestFileWatcher_FileDeleted (0.25s)
settings_test.go:117: File watcher callback was not called
FAIL

I think it still is fsnotify being wonky and not working perfectly on cross platform. I will create a PR to hopefully fix this later today.

[stuartmscott]

I will create a PR to hopefully fix this later today.

Thanks @Jacalz

That is really testing MinSize layout though, I think it's missing a test for "initial layout with extra space".

TestSplitContainer_MinSize tests MinSize, TestSplitContainer calls widget.Resize which calls renderer.Layout. The size given is way bigger than the rectangles need and the test ensures the space is split evenly. Can you point out the codepath that isn't tested, or share an example of a better test for "initial layout with extra space"?

expose offset

Offset could certainly be exposed, the only problem is that it bypasses the limits in updateOffset(int) so the caller can set it to any number and cause layout issues because the offset doesn't respect a child's MinSize. I think the way to go is to export UpdateOffset. The alternative is to export Offset and move the limits into splitContainerRenderer which doesn't feel right.

That depends on the use-case...

You're right, looking at the original bug (#205) there isn't anything about splitting evenly vs unevenly. The linked GTK Paned appears to split evenly, but does have "resize" and "shrink" attributes to determine how the children are changed when the widget is resized. @pbarnum as the bug filer, can you elaborate on the use-cases for this splitter?

Another example is Java Swing's JSplitPane https://docs.oracle.com/javase/tutorial/uiswing/components/splitpane.html which has a resize weight. The default weight 0.5 means the children share the space evenly. A weight of 1 means the trailing child is MinSize and the leading child gets all the rest. 0 means the leading child is MinSize and the trailing child gets all the rest.

[pbarnum]

My use case for this was to set up two panes side by side, one side being fields for a request made to an API, the other side holding the response payload. It will be set to one on top of the other so I could control what I wanted to see more of.

As for even/uneven panes, default I think should be centered, but could be adjustable depending on if you want the panes to be "anchored" or not.

[andydotxyz]

I have tried to find other code where we have used offsets and it's not clear what we should be consistent with here. I think there are 3 options:

• Go with this version with standard coordinate reference, but with 0 in the centre (and resize re-allocates 50/50)
• Copy gradient implementation of using a float 0.0 to 1.0 (and size would be adjusted to match ratio) - matches well if we want pro-rated space allocation - https://github.com/fyne-io/fyne/pull/318/files
• Go with combination of scroller and text align where offset is standard coordinates from top left, and align controls allocation of extra space (leading, centre, trailing) - this fits when people want either 50/50 or snapping left or right (or top or bottom)

I'd like to get other developer opinions on this because I think it's a choice that could set a future trend. @toaster @lucor @Jacalz @okratitan @steveoc64?

[andydotxyz]

TestSplitContainer creates a splitter with two rectangles and resizes it, then checks that both rectangles have the same size so the space is evenly shared.

Apologies you are quite right, it was a bit subtle and I missed it. If the test is actually verifying an equal split perhaps the test name could include that so it's clear. Maybe it changes anyway depending on the discussion above?

This is a really great addition and I wish we could land it right away but some of the API stuff needs to be confirmed. I wondered if we could land with a private API and expose the Offset later, but it will be a behavioural change as well. If we can get this in 1.2.4 it needs to be settled quickly. Otherwise we could drop it into 1.3 and develop it further, but it seems like a shame to miss the release.

[toaster]

If we can get this in 1.2.4 it needs to be settled quickly. Otherwise we could drop it into 1.3 and develop it further, but it seems like a shame to miss the release.

I agree, that this is a great addition and should land better sooner than later.
But IMO it's a new feature and therefore has to land in a minor release anyhow.
Shouldn't patch releases be for bugfixes only?

[andydotxyz]

That is a level headed response, thanks @toaster :).
I am just aware that 1.3 is slipping further and so is new feature delivery.
Should we be considering data api into another new release like 1.4 so 1.3 is the widgets and enhancements that are kind contributors are working on?

[Jacalz]

I agree with @toaster that in general only bug fixes and essential patches should slip into patch releases. Getting 1.3 out the door with these changes and leaving data api to 1.4 sounds like a good plan to me. It does not really mean that the data api will be delayed, just that the other stuff gets here faster and that sounds positive to me :)

[Jacalz]

Regarding the offset question, that is quite a hard task to answer. The current implementation with 0 in the center seems like the best from a pure mathematical standpoint having it more like a ordinate system. I also believe integer math is faster than floating point by some degree, it might how ever not make any difference here so take that with a grain of salt as I haven't had time to look through this PR.

At the same time however, it is good to be consistent throughout the code base. That might be the better way to go. I know that it isn't a conclusive answer, sorry.

[stuartmscott]

Moved to a ratio as per our earlier discussion with a default of 0.5 for an even split.

[stuartmscott]

Issue: if the splitter is made really big, and the divider is moved all the way to one side, and the splitter is then resized smaller, one of the children will get rendered below min size as the ratio limits are only applied in SetRatio, not in Resize/Refresh/Layout.

The renderer could be responsible for re-applying these limits, but it feels wrong, wdyt?

[pbarnum]

It sounds like we could benefit from an anchoring system. If the children could anchor to the parent container, the hooks into the parents size would automatically calculate the size of the children.

Would this be a blocker?

[stuartmscott]

@pbarnum I'm not sure I follow, could you elaborate?

[pbarnum]

Sorry for any confusion. I was commenting on my phone without context. It looks like the child widgets are already "anchored" to the parent container (the adjustable panes). Please disregard my previous comment, I need more knowledge of the codebase to be helpful here.

[stuartmscott]

@spatocode b0b2a1a was only possible because of #750 :)

[stuartmscott]

No worries @pbarnum - I appreciate that you tried to help :)

[andydotxyz]

Moved to a ratio as per our earlier discussion with a default of 0.5 for an even split.

I think the maths looks good - but the naming doesn't seem right. It's not actually a ratio ;).
Offset wasnt a terrible name - isn't that what Gradient uses?

Also, for a UI designer to persist this state the field should really be public. And that answers the question of whether or not it's OK to only have the calculations in a Setter :).
And in response to the original question - we have to check on Resize, as moving smaller than the side's MinSize is not OK.

[stuartmscott]

@Jacalz hit this again

--- FAIL: TestWatchFile (0.34s)
    settings_desktop_test.go:62: File watcher callback was not called
FAIL

[andydotxyz]

Looks pretty slick - just a couple of minor things inline.
I think this approach means that we can do clever things when offset is 0.0 or 1 .0 as well because the intent is saved and it's just Layout() that calculates where the line needs to be to accomodate content.

[stuartmscott]

Yeah that's true. It's outside the scope of this initial PR, but it affords future work to support "clamping" where a child can be hidden if the divider is pulled all the way to 0 or 1.

[andydotxyz]

Great, thanks :)