Ensure we never access out of range table dividers, fixes #2018

[fpabl0]

Description:

Fixes #2018 and possibly #1887.

Checklist:

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

[andydotxyz]

Good catch on the fix - but I don't understand why the 0 width theme is required to test it

[fpabl0]

Good catch on the fix - but I don't understand why the 0 width theme is required to test it

Theme with separator thickness equals to zero is a special case that causes the current code accesses out of range dividers, indeed that is the root cause of #2018, so I thought it would be useful if there is a test for that situation, doesn't it?

[andydotxyz]

If that is the root cause, it should of course be fixed.
What I am unsure about is that the fix does not reference size - are you certain that is the root cause?

[andydotxyz]

Just FYI we just got the following in another PR tests - is this the same bug? The widget tests don't have 0 width separators.

panic: runtime error: index out of range [1] with length 1

goroutine 309 [running]:
fyne.io/fyne/v2/widget.(*tableRenderer).moveIndicators(0xc001d8b650)
	/Users/runner/work/fyne/fyne/widget/table.go:350 +0xc30
fyne.io/fyne/v2/widget.(*Table).CreateRenderer.func1(0x0)
	/Users/runner/work/fyne/fyne/widget/table.go:78 +0x65
fyne.io/fyne/v2/internal/widget.(*Scroll).updateOffset(0xc0010599e0, 0x0, 0x143f120)
	/Users/runner/work/fyne/fyne/internal/widget/scroller.go:580 +0x1dd
fyne.io/fyne/v2/internal/widget.(*Scroll).Refresh(0xc0010599e0)
	/Users/runner/work/fyne/fyne/internal/widget/scroller.go:534 +0x3a
fyne.io/fyne/v2/internal/widget.(*Scroll).Resize(...)
	/Users/runner/work/fyne/fyne/internal/widget/scroller.go:546
fyne.io/fyne/v2/widget.(*tableRenderer).Layout(0xc001d8b650, 0x0)
	/Users/runner/work/fyne/fyne/widget/table.go:243 +0x145
fyne.io/fyne/v2/widget.(*Table).CreateRenderer(0xc000dd1680, 0x14555e0, 0xc000dd1680)
	/Users/runner/work/fyne/fyne/widget/table.go:81 +0x59c
fyne.io/fyne/v2/internal/cache.Renderer(0x14f3dc0, 0xc000dd1680, 0xc000dd1680, 0x14f3dc0)
	/Users/runner/work/fyne/fyne/internal/cache/widget.go:29 +0x119
fyne.io/fyne/v2/internal/app.ApplyThemeTo(0x14f2820, 0xc000dd1680, 0x14f5080, 0xc000dd1720)
	/Users/runner/work/fyne/fyne/internal/app/theme.go:17 +0x1bf
fyne.io/fyne/v2/internal/app.ApplySettings(0x14f0ea0, 0xc001bc9e00, 0x14f4a40, 0xc001d578f0)
	/Users/runner/work/fyne/fyne/internal/app/theme.go:36 +0x105
fyne.io/fyne/v2/test.NewApp.func1(0xc001f68180, 0xc001d578f0)
	/Users/runner/work/fyne/fyne/test/testapp.go:106 +0x79
created by fyne.io/fyne/v2/test.NewApp
	/Users/runner/work/fyne/fyne/test/testapp.go:102 +0x1f8

[fpabl0]

What I am unsure about is that the fix does not reference size - are you certain that is the root cause?

Well the real problem is the initial state (like the case when a window is created and it reports zero size and then the real size), in that case almost all related variables to the renderer are 0 and the value of the theme.SeparatorThickness is decisive:

for y := theme.Padding() + t.scroll.Offset.Y - float32(count) - separatorThickness; y < t.scroll.Offset.Y+t.t.size.Height && i < rows-1 && divs < len(t.dividers); y += t.cellSize.Height + separatorThickness {
	if y < theme.Padding()+t.scroll.Offset.Y {
		continue
	}
        ....
}

If you run that test in the current develop branch it will fail 100% of the times with index out of range.

Just FYI we just got the following in another PR tests - is this the same bug? The widget tests don't have 0 width separators.

It is the same bug (index out of range), but different situation for sure. This PR would solve that bug too as now it is impossible to go out of range (unless there is a race condition and that is the root cause of the others?). For example #1887 is a different situation but with the same bug, however that situation is harder to replicate and so doing a test for that case is so difficult.

The test with SeparatorThicknessZero theme is consistent (and a special case), but certainly is not the best way to test this in a general form, should I remove it?

[andydotxyz]

Thanks for the summary. It would be a shame not to test so you're probably right to leave it.
Might be able to reduce the line count by embedding a theme, such as test.Theme() and override just the Size method...

Oh, and the private type should be at the end of the file, below the exported test methods.

[fpabl0]

Might be able to reduce the line count by embedding a theme, such as test.Theme() and override just the Size method...

Yes, that would be great, but currently it is not possible because there isn't a public theme object available to be embedded, is it?

Oh, and the private type should be at the end of the file, below the exported test methods.

You are right, I forgot it, sorry, will update :)

[andydotxyz]

Yes, that would be great, but currently it is not possible because there isn't a public theme object available to be embedded, is it?

This example works - could be adapted to reduce your line count I am sure:

type bigTextTheme struct {
	fyne.Theme
}

func (b *bigTextTheme) Size(n fyne.ThemeSizeName) float32 {
	if n == theme.SizeNameText {
		return 22
	}

	return b.Theme.Size(n)
}

That can be set like follows:

	a.Settings().SetTheme(&bigTextTheme{test.Theme()})

[fpabl0]

Oh that's great, I didn't know that it is possible to embed an interface 😅 into a struct.

[fpabl0]

@andydotxyz updated :)

[andydotxyz]

Thanks. I guess this should be added to the v2.0.1 work as well?

[fpabl0]

Thanks.

You're welcome.

I guess this should be added to the v2.0.1 work as well?

Yes, it should. In fact, any bug fixes that don't involve new methods or breaking changes should be added IMHO :)