Skip to content
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

Fix the behavior when dragging the divider of split container #1618

Merged
merged 3 commits into from Dec 30, 2020

Conversation

lusingander
Copy link
Contributor

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.

Copy link
Member

@toaster toaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

lusingander commented Nov 29, 2020

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
Copy link
Contributor Author

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
Copy link
Member

toaster commented Nov 30, 2020

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
Copy link
Member

toaster commented Nov 30, 2020

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

@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.

andydotxyz
andydotxyz previously approved these changes Dec 1, 2020
stuartmscott
stuartmscott previously approved these changes Dec 1, 2020
Copy link
Member

@stuartmscott stuartmscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks @lusingander!

@toaster
Copy link
Member

toaster commented Dec 2, 2020

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
Copy link
Member

toaster commented Dec 2, 2020

  • 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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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

@lusingander
Copy link
Contributor Author

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
andydotxyz previously approved these changes Dec 16, 2020
@andydotxyz
Copy link
Member

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 ?

# Conflicts:
#	widget/splitcontainer.go
@toaster
Copy link
Member

toaster commented Dec 20, 2020

@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
Copy link
Member

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
Copy link
Member

toaster commented Dec 29, 2020

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

toaster commented Dec 30, 2020

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
Copy link
Member

toaster commented Dec 30, 2020

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

toaster commented Dec 30, 2020

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

@andydotxyz
Copy link
Member

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

@andydotxyz andydotxyz merged commit 4ba698d into fyne-io:develop Dec 30, 2020
@lusingander lusingander deleted the fix/split branch January 1, 2021 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants