Fix the behavior when dragging the divider of split container

[lusingander]

Description:

In SplitContainer, when the default offset value is less than MinSize ratio, dragging the divider does not seem to work as expected.

For example, in the following image, the offset of the main window is set to 0 and the offset of each of the SplitTab is set to 1.

I think that the following behavior would be expected:

Checklist:

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

[toaster]

I’ve a couple of questions/comments:

1. I don’t understand why the numbers of the new test are as they are.
   Why is a move of 10 a change in the offset of 0.3?
2. I’d prefer a test which checks the rendered output of the whole container not only some property of the divider.
3. I cannot reproduce the behaviour shown on your GIFs. On my Mac only the divider of the outer container misbehaves while the dividers of the inner container work like expected. Also, I did not understand why the current code misbehaves. Do you have an explanation?

[lusingander]

Let me check on 3. first,

I cannot reproduce the behaviour shown on your GIFs.

Would you reproduce it with a code like the following?

func main() {
	a := app.New()
	w := a.NewWindow("Test")

	windowSize := fyne.NewSize(500, 500)
	rectMinSize := fyne.NewSize(150, 150) // windowSize * 0.3
	r1 := canvas.NewRectangle(color.RGBA{128, 0, 0, 0})
	r2 := canvas.NewRectangle(color.RGBA{0, 128, 0, 0})
	r3 := canvas.NewRectangle(color.RGBA{0, 0, 128, 0})
	r1.SetMinSize(rectMinSize)
	r2.SetMinSize(rectMinSize)
	r3.SetMinSize(rectMinSize)

	s1 := widget.NewVSplitContainer(r1, r2)
	s2 := widget.NewHSplitContainer(r3, s1)

	// set 0.0 - 0.3 if check the leading
	// set 0.7 - 1.0 if check the trailing
	s1.SetOffset(0.1)
	s2.SetOffset(0.1)

	w.SetContent(s2)
	w.Resize(windowSize)

	w.ShowAndRun()
}

[lusingander]

In the above example, if move the divider to the 50 right or down, value of Offset becomes 0.2 because of divider#Dragged calculation.
The width or height of Leading calculated using this value will be approximately 100.
However, since this value is less than the MinSize of the Rectangle, the splitContainerRenderer#computeSplitLengths will not be able to detect a change in Offset.

[toaster]

Ah, so the problem is not the computation in *divider.Dragged() but the *SplitContainer.Offset not representing the real offset.

IMO, this should be fixed in the container instead.

• offset should be unexported because you cannot set whatever value you like without having it changed.
• SetOffset should (eventually, probably via Refresh() and Layout()) compute and adjust the offset accordingly.
• Offset() (new) should always deliver the actually used offset.

[toaster]

P.S.: I’m not dogmatic about offset being unexported but I find it unexpected if I assign a value and this is changed after calling Refresh().

[andydotxyz]

@toaster perhaps compare it to Slider where Value is exported but we clamp it to valid values on refresh.
To me this seems reasonable - we are correcting invalid values, but if a developer wants to set Offset to 0.1 then the chance is they meant "10% of the size, or as near as possible" - it is unlikely that 10% is a numerically significant number that we should error out of if not possible.

[andydotxyz]

On the face of it I think this change makes sense, and does fix an issue I have encountered before as well.
But let's finish the conversation @toaster raised before deciding.

[lusingander]

Thanks for the confirmation.

conversation @toaster raised

Does this refer to the following comments?

I’ve a couple of questions/comments: #1618 (review)

How should this test be done?

test which checks the rendered output of the whole container

Like assert.Equal(t, 10, split.Leading.Size().Width), would it be better to check the size in the same test, or to do other kind test?

[andydotxyz]

@lusingander I was referring to the 3 bullets message that was raised regarding offset, but yes the PR comments would need addressed as well.

@toaster:

• Offset() (new) should always deliver the actually used offset.

It's my understanding that's what the Offset field should currently do. The change added here stops it from setting invalid values, so the outcome of this landing is that Offset behaves like your proposed Offset() but without the breaking change.

[stuartmscott]

Nice work, thanks @lusingander!

[toaster]

Regarding (un)exported Offset: I already said, I’m not dogmatic about it. I would not have it exported in the first place because it is subject to adjustments due to constraints but I don’t object to keep it exported.

Regarding the fix: It is definitely needed but I still think that the place is wrong and that the tests should be more “integrative” (testing the render output).
The correct place would be a Split.adjustOffset() helper which has to be called from Split.SetOffset() and Split.Layout().
The tests should validate the numbers in the render output via test.AssertRendersToMarkup().

[toaster]

• Offset() (new) should always deliver the actually used offset.

It's my understanding that's what the Offset field should currently do. The change added here stops it from setting invalid values, so the outcome of this landing is that Offset behaves like your proposed Offset() but without the breaking change.

No, it doesn’t because Layout() respects the constraints but does not update the offset.

[andydotxyz]

The correct place would be a Split.adjustOffset() helper which has to be called from Split.SetOffset() and Split.Layout().

Seems like this would resolve the concerns, If you can address it @lusingander we would like to get this merged into 1.4.2 which I hope to call for vote at the end of the week.

[stuartmscott]

As SetOffset calls Refresh, which calls Layout, it would suffice to apply the limits to Offset in Layout. This would be similar to Slider.clampToRange

However the original intention was that Offset represented the developers wishes, and the Splitter would do its best to achieve this. For example, if the dev wanted a master/detail-style layout with a collection (A) on the left taking up a small proportion of space, and a detail section (B) on the right taking up the majority of space they could set Offset to 0.25. If the user resized the window too small such that A was too big to fit in 0.25 of screen it would be allocated its MinSize even though this would result in A taking up more than 0.25 of the space. And when the user resizes the window back to being large enough A would return to being only 0.25.

However the real bug here is that splitContainerRenderer.MinSize doesn't take into account the ratio;

func (r *splitContainerRenderer) MinSize() fyne.Size {
	s := fyne.NewSize(0, 0)
	for _, o := range r.objects {
		min := o.MinSize()
		if r.split.Horizontal {
			s.Width += min.Width
			s.Height = fyne.Max(s.Height, min.Height)
		} else {
			s.Width = fyne.Max(s.Width, min.Width)
			s.Height += min.Height
		}
	}
	return s
}

Instead it should calculate the minsize of A and multiply that the reciprocal of the Offset. And calculate the minsize of B and multiple that by the reciprocal of 1-Offset. Finally add the minsize of the divider. This way the splitter could never be resized smaller than required to show the children with the current Offset.

The downside to this, and the reason I didn't implement it, is because it resulted in odd behaviour where the user dragged the divider, and the splitter's minsize changed to accommodate the children so the window kept expanding.

[andydotxyz]

I think it's better without having the offset be included in the MinSize.
Afterall if we're returning the true min size it really is just the left + right + split.

[andydotxyz]

@toaster where are we on your thoughts for this?
I note that you said the Offset is not something you are hugely bothered by, but this PR is blocked on your request for changes...

[andydotxyz]

@lusingander can you please rebase on latest develop?
That will check the code is up to date, and also trigger the new tests.

[lusingander]

I've rebased it to the latest version.
I have not been able to get it to work by changing where the Offset is adjusted.

And I see the windows test is failing...

[andydotxyz]

The passes check (was a temporary failure) and I'm happy that this is a useful fix.
It could be wider reaching but I think we shuold accept as is.
@toaster ?

[toaster]

@lusingander @andydotxyz I cannot see what the latest changes were. Force pushes are a very bad idea on a work-in-progress PR.

However, looking at the complete diff, I cannot see were my objections were addressed:

• There is still a lot of computation in the Dragged method which is still out-of-place IMO.
• The tests still don’t check the rendered state but some number that might or not might have the desired effect. The relation of the dragged distance and the resulting offset change is still unclear and the test does not even test a change in offset but a change between a desired and a (maybe) actual offset value.

[andydotxyz]

Reviewing tickets for 1.4.3 which is being put together now and saw this ticket.

Thinking about the request for image tests I'm not sure how it can prove correctness without the mouse cursor visible, so maybe the tests present are sufficient.
The location of the code matches what is there, and I would like to see the fix in if possible - could we perhaps do a follow-on to refactor it's location @toaster?

[toaster]

I didn’t ask for image tests but for markup tests. Also, the mouse pointer does not need to be visible for an image based test to work.
The location of the code is still wrong. I’ll try to provide a better fix tomorrow. Is this sufficient?

[andydotxyz]

If we want to cherry-pick this to 1.4.x we can't use the markup tests, as they are not available in 1.4 code...

As for code location I worry that we are setting a precident here that bug fixes must come with code refactoring if the code it's fixing is out of date / not well structured... Is that what we want?

[andydotxyz]

Also, the mouse pointer does not need to be visible for an image based test to work.

Normally, absolutely. However in this case the fix is in applying the correct behaviour based on mouse location.
If we only have the render state then how can we tell that that the bar is under the mouse pointer?

[toaster]

If we want to cherry-pick this to 1.4.x we can't use the markup tests, as they are not available in 1.4 code...

That’s the scenario I had in mind with https://gophers.slack.com/archives/CK5U4BU87/p1606297439264800. I don’t like the idea to have to do things in a certain way on dev because of some release branch.

As for code location I worry that we are setting a precident here that bug fixes must come with code refactoring if the code it's fixing is out of date / not well structured... Is that what we want?

That is what I want at least.
Do not add lower quality patches to code to avoid refactoring. That might be suitable in rare situations when you want to get an urgent fix that otherwise would require a bigger refactoring on a release branch out of the door. But it should not be done on the dev branch.

[toaster]

Also, the mouse pointer does not need to be visible for an image based test to work.

Normally, absolutely. However in this case the fix is in applying the correct behaviour based on mouse location.
If we only have the render state then how can we tell that that the bar is under the mouse pointer?

Because we “move” the mouse to position x,y and then expect that the bar’s area includes this exact position.

[andydotxyz]

Just for clarity, what I was hoping for was to land this so it could be in the 1.4.3 release, which we were hoping to prepare iminently ("Late December" was the due date).

It is relatively clear that, despite this being a simple and useful fix, the objections listed about making that change on develop seem to indicate it will not be possible to land this for 1.4.

[andydotxyz]

Do not add lower quality patches to code to avoid refactoring

I do not consider this a lower quality patch - I think it matches the quality of the code in that area well.
For bug fixes, particularly from contrubutors not in our core team, I don't want to add the requirement that code be refactored to meet higher standards than the code being fixed was implemented with - I think this gets in the way of accepting donated work.

[toaster]

As discussed in Slack, I’ll refactor the SplitContainer later on.

[andydotxyz]

Thanks very much @toaster I appreciate your help on this. We can get it into 1.4.3 and work on the refactor later :).