tulir / mauview

A Go TUI library based on tcell.
Mozilla Public License 2.0
48 stars 7 forks source link

No break in SetFocused loop applies old focus after exiting #13

Open ardangelo opened 6 months ago

ardangelo commented 6 months ago

In gomuks, ran into a tsrange problem with SetFocused. It is not updating flex.focused unless a break is added to the loope, despite the condition matching and updating only once.

Added debug logging,

func (flex *Flex) SetFocused(comp Component) {

    dbglog(fmt.Sprintf("SetFocused on component %p\n", comp))
    for _, child := range flex.children {
        if child.target == comp {
            dbglog(fmt.Sprintf("    found component match %p == %p\n", child.target, comp))
            flex.focused = &child
            flex.focused.Focus()
        }
    }
    dbglog(fmt.Sprintf("    flex focused target: %p\n", flex.focused.target))
}

The focused component is only matched and updated once, yet the focused target is still set to the older value.

SetFocused on component 0xc0000e41b0
    found component match 0xc0000e41b0 == 0xc0000e41b0
    flex focused target: 0xc000000140

Adding a break to the loop after a match is found fixes the issue.

n-peugnet commented 6 months ago

Adding a break seems a good idea, as we don't need to check the rest of the children if we had one match. But another problem I see with this code is that &child does not reference the value from flex.children, but rather its copied value from the range loop. It should probably look something like this instead:

-   for _, child := range flex.children {
-       if child.target == comp {
-           flex.focused = &child
+   for i := range flex.children {
+       childp = &flex.children[i]
+       if childp.target == comp {
+           flex.focused = childp
            flex.focused.Focus()
            break
        }
    }
ardangelo commented 5 months ago

Thanks, updated PR with change.