mauview icon indicating copy to clipboard operation
mauview copied to clipboard

No break in SetFocused loop applies old focus after exiting

Open ardangelo opened this issue 11 months ago • 2 comments

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.

ardangelo avatar Mar 23 '24 15:03 ardangelo

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
		}
	}

n-peugnet avatar Apr 10 '24 08:04 n-peugnet

Thanks, updated PR with change.

ardangelo avatar Apr 19 '24 23:04 ardangelo