Storybook: Correctly focus first button after changing page - #2682
Conversation
Keyboard is useable to click through StoryQuests after a page has been flipped.
|
Play this branch at https://play.threadbare.game/branches/endlessm/storybook-focus-fix/. (This launches the game from the start, not directly at the change(s) in this pull request.) |
manuq
left a comment
manuq
left a comment
There was a problem hiding this comment.
@alcole2 do you understand why this works? Preferrably, you would explain the why of your changes in the pull request (or commit) description. Our contributing page has useful links like Telling stories through your commits. Please take a read! Or if you like it in video form, here is an interview with a quote I like very much: "the hardest part of contributing to the linux kernel is not the code, is the description". If you had to get a single thing out of your internship, I hope this is it!
In particular, I don't think that changing the way nodes are freed from memory is the best way to update the focus. It is probably a side effect, but is better to set the focus explicitely (after refilling the list with new items). But you can try to convince me with a good explanation!
|
@manuq When flipping a page, I usually test a lot of different ideas and this one just happened to work, and everything else I was trying did not work so I just went with this solution. If I need to change it and do it a different way I can. Also, I'll make sure to be more descriptive in my commit descriptions! |
| @@ -58,9 +58,9 @@ func _ready() -> void: | |||
| func _populate_quest_lists() -> void: | |||
| #Clear out any existing buttons from previous views | |||
| for child in left_quest_list.get_children(): | |||
| child.queue_free() | |||
| child.free() | |||
| for child in right_quest_list.get_children(): | |||
| child.queue_free() | |||
| child.free() | |||
There was a problem hiding this comment.
Having read the code, I'm not sure it's incorrect :)
I can see why it works now: the function ends with a call to reset_focus which calls _switch_to_page(0) which ... actually no, in this case it doesn't call _update_page_visibility (which is what calls first_button.grab_focus()) because of the if spread_index == _current_spread_index: guard. Odd. Oh, but then the code that calls _populate_quest_lists() calls _update_page_visibility directly,
Personally, although it's more verbose, I think the intent is clearer if you explicitly remove each child from the list, and queue it to be free'd, as separate actions.
| child.free() | |
| func _clear_list(quest_list: Node) -> void: | |
| for child: Node in quest_list.get_children(): | |
| quest_list.remove_child(child) | |
| child.queue_free() | |
| # Clears and regenerates the quest buttons based on the current page view | |
| func _populate_quest_lists() -> void: | |
| _clear_list(left_quest_list) | |
| _clear_list(right_quest_list) |
And I agree that the commit message should explain why the change fixes the bug - it wasn't at all obvious at first reading, particularly because the grab_focus call is so far away!
There was a problem hiding this comment.
That makes sense! My apologies, I thought the commit message was for a general summary of the change. I will redo the commit with the requested changes!
There was a problem hiding this comment.
It's also a summary of the changes, but particularly for small changes the "what" is normally clear from reading the code plus the one line summary - it is often the "why" that needs more explanation. In this case, the "what" is "freeing some objects immediately rather than at the end of the current" frame - but why that affects the focus logic is less clear.
This does not use ```.free()``` like the previous commit. Instead, the new ```_clear_list()``` function uses ```remove_child()``` to detach each old StoryQuest button from its container before ```queue_free()``` is called. ```_clear_list()``` is called inside ```_populate_quest_lists()``` so the container's child list is immediately emptied when the page turns. This ensures the focus is on the StoryQuest button on the top of the new page instead of accidentally grabbing a removed node from the previous page.
alcole2
changed the title
wjt
left a comment
wjt
left a comment
There was a problem hiding this comment.
Looks good. I've reworded the message a bit to explain more specifically:
- What the problem was
- Why it occurred
- How this change fixes it
wjt
changed the title
3 participants
After
_populate_quest_lists()rebuilds the quest lists,_update_page_visibility()picks the first child of the left list and assigns focus to it. However, previously_populate_quest_lists()did not remove the old children from the left (and right) lists before adding new ones; it merely queued them to be freed at the end of the current frame. This meant that at the point where_update_page_visibility()gave focus to the 0th child of the left list, it was still the old button, which was freed shortly afterwards.Fix this by explicitly removing each button from each list before
queue_free()ing it, so that when_update_page_visibility()gets the first child, it finds the first of the new buttons.Resolves #2616