Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad #49928

DrRevert · 2021-06-26T18:12:26Z

This PR adds support for UI focus Tabs and TabContainer so they can be navigated using keyboard and gamepad. Related issues: #25877 #46111
Changes:

Tab and TabContainer focus mode is now set to "All" by default
When Tab/TabContainer node has focus it will render a blue rectangle (the same as the button)
When Tab/TabContainer node has focus pressing ui_left will switch to the tab on the left of the currently selected, if the first tab is currently selected it will loop over and choose the last tab
Similary when pressing ui_right the tab on the right be selected, if the last tab is currently selected, it will loop over and select first tab
~~Pressing ui_down when TabContainer has focus will in practice have the same effect as pressing ui_focus_next, more details in the Known issues section.~~ (refer to my second comment for more information)

Known issues:

- Because of the way TabContainer is implemented, changing focus from the TabContainer and Control inside the tab (child) is tricky. Engine for some reason doesn't treat TabContainer and child Control as valid neighbours. Switching from TabContainer to a child can be achieved using the find_next_valid_focus, but might cause issues in a more advanced UI design. Switching from child to TabContainer can be accomplished by explictly setting the Neighbor Top/Left/Right/Bottom parameter.
- Current implementation doesn't check if the tab is disabled, before switching to it.
- Behavior for looping over from last to first tab or vice versa might not be desired and should be enabled by a new property
- Focus graphic is the same as button focus graphic, it looks okay, but if the PR gets approved it should propably use its own graphic

…t and ui_right actions

…to focusable_tabs

Rollover can be turned on and off

DrRevert · 2021-06-27T13:16:44Z

Updated PR:

pressing ui_down when TabContainer is focus won't trigger "find next valid focus" - I came to the conclusion that if you need to prepare extra logic to handle getting from child element to the TabContainer you should also write logic for getting from TabContainer to child. Otherwise this breaks the flow, as the developer might not be expecting that the focus will get stolen because user pressed down. Fixing this issue globally probably requires changing how engine looks for neighbor Control nodes, which is way out of the scope of this PR and might cause more issue down the line.
added handing disabled tabs
added rollover boolean parameter, which controls if pressing right on the last tab or left on the first tab should select the tab on the other side

Calinou · 2021-06-27T14:07:17Z

Focus graphic is the same as button focus graphic, it looks okay, but if the PR gets approved it should propably use its own graphic

I think we could leave it as-is, especially since people using custom themes will have to provide a StyleBox for this new element once this PR is merged. What do you think the custom graphic should look like? How do other applications handle this?

DrRevert · 2021-06-27T14:17:56Z

I think we could leave it as-is, especially since people using custom themes will have to provide a StyleBox for this new element once this PR is merged. What do you think the custom graphic should look like? How do other applications handle this?

I didn't actually thought how the final graphic should look like. Windows in Win32 programs renders a dotted rectangle around the tab name if it has keyboard focus, which is slightly smaller that the tab

By default, users cannot switch tabs on TabContainers without using a mouse. This commit adds the ability to switch tabs using either a keyboard or a controller, using the focus system. This commit is based on the Godot PR: godotengine#49928

Eoin-ONeill-Yokai

Hey, thanks for taking the time to write this PR. I'm sorry it's taken a while for the community to review.

I've provided some feedback but none of them are major blockers. Most are just my opinion, but things are working as expected for the most part. I think this PR is a good idea so I wanted to provide some feedback. I hope that by reviewing this PR we can speed up getting members of the project to consider it for the next 4.X release (4.2 since 4.1 is in feature freeze.)

If you're around and are still willing to deal with rebasing the code, let me know. Otherwise, I could help you with the rebase process if necessary.

Finally, once all of the changes are made, make sure to condense your commits down to 1 commit message following our commit message standard if possible.

Thanks!

Eoin-ONeill-Yokai · 2023-06-01T14:01:27Z

scene/gui/tab_container.h

+	void set_rollover(bool p_enabled);
+	bool get_rollover() const;
 	TabContainer();


If you don't mind, please try to cluster likewise functions together with a space between them. E.G:

Suggested change

void set_rollover(bool p_enabled);

bool get_rollover() const;

TabContainer();

void set_rollover(bool p_enabled);

bool get_rollover() const;

TabContainer();

Eoin-ONeill-Yokai · 2023-06-01T15:59:04Z

scene/gui/tabs.cpp

+		if (p_event->is_action_pressed("ui_right")) {
+			int next_tab = get_current_tab() + 1;
+			bool valid = true;
+			while (valid) {


Any reason why this needs to be a while loop?

It seems to me like a for loop which acts as an offset from an origin is more appropriate and easier to read.

int origin_tab = get_current_tab(); for (int offset = 0; offset < get_tab_count(); offset++) { int next_tab = (origin_tab + offset) % get_tab_count(); bool valid = get_tab_disabled(next_tab); if (!rollover) { valid &= next_tab > origin_tab; if (!valid) { break; } } if (valid) { set_current_tab(next_tab); accept_event(); break; } }

This might not cover all use cases or compile completely as I'm writing it mostly pseudo code, but something along those lines seems a bit easier to read to me. It also doesn't require checking for self as it will naturally wrap around with the use of modulo.

Not really, I just found while loop being easiest way to implement searching for a valid tab and didn't thought of for loop.

Eoin-ONeill-Yokai · 2023-06-01T16:03:10Z

scene/gui/tabs.cpp

+		} else if (p_event->is_action_pressed("ui_left")) {
+			int prev_tab = get_current_tab() - 1;
+			bool valid = true;
+			while (valid) {


Same as the last comment but inverse direction. Modulo won't work here, so you'll have to appropriately index wrap as you see fit.

DrRevert · 2023-06-08T13:15:47Z

Hi, sure I can look intro rebasing/refactoring this weekend if I have time.
Thanks for the feedback.

DrRevert · 2023-06-25T16:06:43Z

Hey, so I started working on refactoring as there was actually a lot of changes in codebase between prerelease and current version.
master...DrRevert:godot:focusable_tabs_refactor
I didn't have time to replace "while" loops with "for" so I'm not updating PR yet, however the main feature should be working with no problem. I will try to get back to this as soon as possible.

DrRevert · 2023-07-06T15:11:46Z

There are too many changes between pre-alpha and stable versions so I think the best way is to start new PR instead of trying to rebase this one.
I created new PR for refactored approach here #79104

DrRevert added 6 commits June 26, 2021 19:12

Focusable Tabs and TabContainer, tabs can be now changed using ui_lef…

4c8405a

…t and ui_right actions

Fix for file formatting and outdated documentation

807af8e

Fix for file formatting and outdated documentation

02154f2

Merge branch 'focusable_tabs' of https://github.com/DrRevert/godot in…

c9656e9

…to focusable_tabs

Merge branch 'focusable_tabs' of https://github.com/DrRevert/godot in…

d0bc23e

…to focusable_tabs

Style fix

874e954

Calinou added enhancement topic:editor topic:gui topic:input usability labels Jun 26, 2021

Calinou added this to the 4.0 milestone Jun 26, 2021

DrRevert added 4 commits June 27, 2021 14:20

Skip over disabled tabs;

d469dc7

Rollover can be turned on and off

Documentation changes

3f7198e

TabContainer ui_down no longer calls "find next valid focus"

96d392a

Removed accidental new line

1421880

DrRevert changed the title ~~[WIP] Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad~~ Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad Jun 27, 2021

DrRevert marked this pull request as ready for review June 27, 2021 14:36

DrRevert requested review from a team as code owners June 27, 2021 14:36

duianto mentioned this pull request Jul 22, 2022

Enable Options button on TitleScreen a-little-org-called-mario/a-little-game-called-mario#562

Merged

akien-mga modified the milestones: 4.0, 4.x Jan 19, 2023

Eoin-ONeill-Yokai suggested changes Jun 1, 2023

View reviewed changes

DrRevert mentioned this pull request Jul 6, 2023

Allow to focus individual tabs in TabBar/TabContainer #79104

Merged

DrRevert closed this Jul 6, 2023

AThousandShips added the archived label Jul 6, 2023

YuriSizov removed this from the 4.x milestone Dec 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad #49928

Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad #49928

DrRevert commented Jun 26, 2021 •

edited

Loading

DrRevert commented Jun 27, 2021

Calinou commented Jun 27, 2021

DrRevert commented Jun 27, 2021

Eoin-ONeill-Yokai left a comment

Eoin-ONeill-Yokai Jun 1, 2023

Eoin-ONeill-Yokai Jun 1, 2023

DrRevert Jun 8, 2023 •

edited

Loading

Eoin-ONeill-Yokai Jun 1, 2023

DrRevert commented Jun 8, 2023

DrRevert commented Jun 25, 2023

DrRevert commented Jul 6, 2023

Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad #49928

Focusable Tabs and TabContainer, tabs can be changed using keyboard and gamepad #49928

Conversation

DrRevert commented Jun 26, 2021 • edited Loading

DrRevert commented Jun 27, 2021

Calinou commented Jun 27, 2021

DrRevert commented Jun 27, 2021

Eoin-ONeill-Yokai left a comment

Choose a reason for hiding this comment

Eoin-ONeill-Yokai Jun 1, 2023

Choose a reason for hiding this comment

Eoin-ONeill-Yokai Jun 1, 2023

Choose a reason for hiding this comment

DrRevert Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Eoin-ONeill-Yokai Jun 1, 2023

Choose a reason for hiding this comment

DrRevert commented Jun 8, 2023

DrRevert commented Jun 25, 2023

DrRevert commented Jul 6, 2023

DrRevert commented Jun 26, 2021 •

edited

Loading

DrRevert Jun 8, 2023 •

edited

Loading