Skip to content

Remove uses of renderer cache in textgrid - #5834

Merged
andydotxyz merged 2 commits into
fyne-io:developfrom
redawl:textgrid_remove_cache_renderer
Jul 4, 2025
Merged

Remove uses of renderer cache in textgrid#5834
andydotxyz merged 2 commits into
fyne-io:developfrom
redawl:textgrid_remove_cache_renderer

Conversation

@redawl

@redawl redawl commented Jul 2, 2025

Copy link
Copy Markdown
Member

Description:

Remove usage of renderer cache in TextGrid, per #5827

I also fixed a bug where textGridRow was only calling ExtendBaseWidget once its renderer was created. This was causing a couple of tests to fail, because with the cache.Renderer calls removed, we were no longer creating the renderer during unit test setup.

Because we are now extending BaseWidget on creation of rows, a lot of snapshots needed updating. I took a look at the changes, and I think they were actually wrong before. LMK if I'm wrong about that :)

Checklist:

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

@coveralls

coveralls commented Jul 2, 2025

Copy link
Copy Markdown

Coverage Status

coverage: 62.3% (+0.03%) from 62.266%
when pulling d278882 on redawl:textgrid_remove_cache_renderer
into 0c795d6 on fyne-io:develop.

@Jacalz Jacalz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this. This is great stuff and an important change. I'm not entirely sure how to solve this some other way but I think this still might be breaking the promise of widgets not referencing their renders because some of the widgets that make up the rows etc. are now referencing the objects that the renderer is rendering. See discussion on #5822.

Comment thread widget/textgrid.go

func (t *textGridRowRenderer) appendTextCell(str rune) {
th := t.obj.text.text.Theme()
func (t *textGridRow) appendTextCell(str rune) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The method ordering here is a little strange now that the widgets methods are below the renderer struct.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved textGridContentRenderer and textGridRowRenderer typedefs so that they are above their methods, thanks!
d278882

@redawl

redawl commented Jul 3, 2025

Copy link
Copy Markdown
Member Author

Thanks for this. This is great stuff and an important change. I'm not entirely sure how to solve this some other way but I think this still might be breaking the promise of widgets not referencing their renders because some of the widgets that make up the rows etc. are now referencing the objects that the renderer is rendering. See discussion on #5822.

I could be wrong here, but I think this is a different case. textGridRow and textGridContent are wrapper components that are only known about by the renderers. TextGrid, TextGridRow and TextGridCell are the structs that contain the "state" that is being referred to here.

@Jacalz

Jacalz commented Jul 3, 2025

Copy link
Copy Markdown
Member

Hmm. Good point. If you think about those sub widgets being opened by the main renderer it kind of makes sense. However, they are still widgets so I don't know how to reason about it. I think it is best to check with Andy.

@Jacalz
Jacalz requested a review from andydotxyz July 3, 2025 11:15
@andydotxyz

Copy link
Copy Markdown
Member

I think this change makes sense. Many widgets will be children of the parent widgets, the pattern here doesn't seem to go against the pattern of referencing renderers.

@andydotxyz
andydotxyz merged commit 9fe136f into fyne-io:develop Jul 4, 2025
11 checks passed
@redawl
redawl deleted the textgrid_remove_cache_renderer branch July 4, 2025 13:53
@Jacalz

Jacalz commented Jul 5, 2025

Copy link
Copy Markdown
Member

Great :)

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.

4 participants