Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ardangelo
Copy link

@ardangelo ardangelo commented Mar 23, 2024

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
Copy link
Contributor

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
Copy link
Author

Thanks, updated PR with change.

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.

2 participants