perf(view): avoid unnecessary Layout() calls in Set* methods. #83#85
perf(view): avoid unnecessary Layout() calls in Set* methods. #83#85imicky wants to merge 2 commits into
Conversation
use standard CSS property names in Tree() output
yohamta0
left a comment
yohamta0
left a comment
There was a problem hiding this comment.
Amazing improvements! I have left some comments.
| sb.WriteString( | ||
| fmt.Sprintf("left: %d, right: %d, top: %d, bottom: %d, width: %d, height: %d, marginLeft: %d, marginTop: %d, marginRight: %d, marginBottom: %d, position: %s, direction: %s, wrap: %s, justify: %s, alignItems: %s, alignContent: %s, grow: %f, shrink: %f", | ||
| cfg.Left, *cfg.Right, cfg.Top, *cfg.Bottom, cfg.Width, cfg.Height, cfg.MarginLeft, cfg.MarginTop, cfg.MarginRight, cfg.MarginBottom, cfg.Position, cfg.Direction, cfg.Wrap, cfg.Justify, cfg.AlignItems, cfg.AlignContent, cfg.Grow, cfg.Shrink)) | ||
| intPtrString := func(p *int) string { |
There was a problem hiding this comment.
Just curious, why not make it a regular method like func formatIntPtr(p *int) string? Is it performance reason or something?
There was a problem hiding this comment.
It's not about performance. I just thought it returns the string "nil" when p == nil – just for the debug output in Tree(). Not sure if this is generic enough. Maybe in another case, someone might want it to be "none", "0", or even "-"... Am I overthinking this? Haha.
|
|
||
| // SetLeft sets the left position of the view. | ||
| func (v *View) SetLeft(left int) { | ||
| if v.Left == left { |
There was a problem hiding this comment.
I'm wondering if we should refactor it to save the previous state at the end of View.startLayout(), and then compare it with the new state inside View.Layout() to determine whether it's dirty. What do you think?
There was a problem hiding this comment.
Let's assume this snapshot approach is scoped only to the basic fields listed below, and does not cover complex structures like the children slice.
// TODO: Remove these fields in the future.
Left int
Right *int
Top int
Bottom *int
Width int
WidthInPct float64
Height int
HeightInPct float64
MarginLeft int
MarginTop int
MarginRight int
MarginBottom int
Position Position
Direction Direction
Wrap FlexWrap
Justify Justify
AlignItems AlignItem
AlignContent AlignContent
Grow float64
Shrink float64
Display Display
Hidden boolGiven this scope, here are my thoughts on the pros and cons:
Pros:
- Detects direct field assignment(e.g.,
view.Width = 50) and triggers re-layout. - Reduces setter boilerplate.
- Centralizes dirty-check logic for basic field changes. (However, structural changes like
AddChild/RemoveChild, etc. still require manually settingisDirty, so it's not fully unified.)
Cons:
- Ongoing maintenance burden – a shadow snapshot must be kept in sync with every field. Any newly added field requires updating both the snapshot copy and the comparison logic – easily overlooked and leads to subtle bugs.
- Overhead of full-field comparison per Setter – a single SetWidth currently compares only 1 field; with snapshot, it would compare all ~20 fields. Feels like "doing more work than necessary" on every Set* calls.
It seems technically feasible, but I wonder if it's worth it for the added complexity – what do you think?
Plus, I see the // TODO: Remove these fields in the future. – does that mean this discussion might become irrelevant once those fields change?
2 participants
perf(view): avoid unnecessary Layout() calls in Set* methods. #83
debug: make Tree() safe for nil pointer fields.