Add tab spacing modifier for tabs in TabBar and TabContainer

[aaronjyoder]

Adds support for spacing between tabs.

• LTR support
• RTL support
• Left/Center/Right Alignment support
• Rearrange tab support
• Clipping support
• Allows both positive and negative values for design flexibility

Closes proposal #10057.

[KoBeWi]

• The separation should probably be clamped, because it can be negative.
  
• The tabs are shifting when changing pages.

godot.windows.editor.dev.x86_64_QhJnLjtGcE.mp4

[aaronjyoder]

@KoBeWi Negative values are intended to be allowed, as at least one of the examples in the original proposal had a UI where negative values would be required for visual cohesion (think tabs with slanted/diagonal edges). What kind of clamp were you thinking?

I will look at the tab shifting issue--likely know what is causing that.

Should the tab separation also be considered for determining when to move tabs to the next page, or no? I didn't do it because it seemed out of scope and it causes lots of shifting when tabs move to a new page, but let me know what you think.

[KoBeWi]

We usually do clamp separation to make sure it's non-negative:

godot/scene/gui/button.cpp

Line 254 in 39c201c

const int h_separation = MAX(0, theme_cache.h_separation);

Though I guess the main reason is that it looks bad/glitched in most cases. Maybe allowing it is fine, if it works properly with clicks etc.

Should the tab separation also be considered for determining when to move tabs to the next page, or no?

Separation pushes the tabs further, so this should be taken into account. Otherwise they could stick out of the panel etc. (I didn't test it thoroughly though)

[aaronjyoder]

I'm struggling to figure out the correct way to account for pagination. If anyone has any insights, it would be appreciated.

void TabBar::_update_cache(bool p_update_hover) {
	if (tabs.is_empty()) {
		buttons_visible = false;
		return;
	}

	int limit = get_size().width;
	int limit_minus_buttons = limit - theme_cache.increment_icon->get_width() - theme_cache.decrement_icon->get_width();

	int w = 0;

	max_drawn_tab = tabs.size() - 1;

	for (int i = 0; i < tabs.size(); i++) {
		tabs.write[i].text_buf->set_width(-1);
		tabs.write[i].size_text = Math::ceil(tabs[i].text_buf->get_size().x);
		tabs.write[i].size_cache = get_tab_width(i);

		tabs.write[i].truncated = max_width > 0 && tabs[i].size_cache > max_width;
		if (tabs[i].truncated) {
			int size_textless = tabs[i].size_cache - tabs[i].size_text;
			int mw = MAX(size_textless, max_width);

			tabs.write[i].size_text = MAX(mw - size_textless, 1);
			tabs.write[i].text_buf->set_width(tabs[i].size_text);
			tabs.write[i].size_cache = size_textless + tabs[i].size_text;
		}

		if (i < offset || i > max_drawn_tab) {
			tabs.write[i].ofs_cache = 0;
			continue;
		}

		tabs.write[i].ofs_cache = w;

		if (tabs[i].hidden) {
			continue;
		}

		w += tabs[i].size_cache;
		if (i < max_drawn_tab) { // Add separation between tabs
			w += theme_cache.tab_separation;
		}

		// Check if all tabs would fit inside the area.
		if (clip_tabs && i > offset && (w > limit || (offset > 0 && w > limit_minus_buttons))) {
			tabs.write[i].ofs_cache = 0;

			w -= tabs[i].size_cache;
			if (i < max_drawn_tab) { // Remove separation between tabs
				w -= theme_cache.tab_separation;
			}

			max_drawn_tab = i - 1;

			while (w > limit_minus_buttons && max_drawn_tab > offset) {
				tabs.write[max_drawn_tab].ofs_cache = 0;

				if (!tabs[max_drawn_tab].hidden) {
					w -= tabs[max_drawn_tab].size_cache;
					w -= theme_cache.tab_separation; // Remove separation between tabs
				}

				max_drawn_tab--;
			}
		}
	}

	missing_right = max_drawn_tab < tabs.size() - 1;
	buttons_visible = offset > 0 || missing_right;

	if (tab_alignment == ALIGNMENT_LEFT) {
		if (p_update_hover) {
			_update_hover();
		}
		return;
	}

	//int total_tab_separation = max_drawn_tab * theme_cache.tab_separation;
	if (tab_alignment == ALIGNMENT_CENTER) {
		//w = ((buttons_visible ? limit_minus_buttons : limit) - w - total_tab_separation) / 2;
		w = ((buttons_visible ? limit_minus_buttons : limit) - w) / 2;
	} else if (tab_alignment == ALIGNMENT_RIGHT) {
		//w = (buttons_visible ? limit_minus_buttons : limit) - w - total_tab_separation;
		w = (buttons_visible ? limit_minus_buttons : limit) - w;
	}

	for (int i = offset; i <= max_drawn_tab; i++) {
		tabs.write[i].ofs_cache = w;

		if (!tabs[i].hidden) {
			w += tabs[i].size_cache;
			if (i < max_drawn_tab) { // Add separation between tabs
                              w += theme_cache.tab_separation;
                        }
		}
	}

	if (p_update_hover) {
		_update_hover();
	}
}

That's what I'm trying right now. It works without pagination (i.e. when it first decides there are too many tabs to fit and it must create a new page, it correctly prunes that tab at the right time), but the problem is that as you increase tab_separation, it seems to remove tabs too early, and the point at which it removes them seems to be equal to tab_separation, like it is still counting an extra tab_separation on the end). I've spent several hours on it now and can't really figure out why, but maybe I am missing something simple.

[KoBeWi]

You are adding and subtracting separation twice. There might be some case when the separation is counted twice without subtracting it.
The code would be more reliable if you added separation only once, without subtracting.

[aaronjyoder]

Don't you need to subtract the separation, though, when you remove tabs? Otherwise it will still be considering extra separation as part of the total width without the tabs actually present. I just tried adding separation only once without subtracting and, as expected, it is incorrectly pruning tabs even earlier than before. Adding/subtracting only once has the same effect as the above code. Am I adding/subtracting in the wrong spot?

[KoBeWi]

If you push the code with your attempted fix I'll see what's wrong.

[aaronjyoder]

I messed with it some more and yeah I am still not really able to figure it out. I pushed a change where tabs are at least pruned, but the touch targets are messed up and as you increase tab_separation, it prunes earlier and earlier.

[KoBeWi]

btw another problem is that tab_separation is not accounted for minimum size when clip_tabs is disabled.

[KoBeWi]

Ok I figured out why your code is wrong.
You are adding tab_separation in front of the tab. So e.g. in this situation: [ 1 ] [ 2 ] [ 3 ] the separation between tab 2 and 3 is treated as part of tab 2, so the tab 2 disappears when its front separation goes out of bounds. Also this condition

			w -= tabs[i].size_cache;
			if (i < max_drawn_tab) {
				w -= theme_cache.tab_separation;
			}
			max_drawn_tab = i - 1;

makes it worse, because when the tab 3 goes out of bounds, the buttons appear and tab 2 is clipped based on the extended size, so it disappears even earlier.

[aaronjyoder]

btw another problem is that tab_separation is not accounted for minimum size when clip_tabs is disabled.

Yeah I noticed that, I will fix it once I get the clipped stuff working. I think it may be a good idea to clamp the tab separation in 2 ways:

(1) Negative clamp such that total tab width (accounting for separation) cannot be lower than the the minimum width of the TabBar container (so all buttons overlap each other, but no further)
(2) Positive clamp such that total tab width (accounting for separation) cannot exceed the maximum width of the TabBar container.

This kind of clamp should fit all envisioned use-cases and prevent exiting the bounds of the TabBar container.

Alternatively, in the positive direction, it could dynamically increase the width of the TabBar container if not clipped? Or is that not relevant?

[aaronjyoder]

With regard to your other comment, my understanding is this:

if (tabs[i].hidden) {
	continue;
}

w += tabs[i].size_cache;
if (i < max_drawn_tab) {
	w += theme_cache.tab_separation; // Prevents adding separation after last drawn tab--we still want to account for it otherwise
}

// Check if all tabs would fit inside the area.
if (clip_tabs && i > offset && (w > limit || (offset > 0 && w > limit_minus_buttons))) { // If you pass this check, you exceed the length, and have to clip a tab
	tabs.write[i].ofs_cache = 0;

	w -= tabs[i].size_cache;
	w -= theme_cache.tab_separation; // We're definitely drawnig at least one fewer tab, so remove the tab separation we just added

	max_drawn_tab = i - 1;

	while (w > limit_minus_buttons && max_drawn_tab > offset) { // remove tabs (and their width from accumulated width w) until we are below the width limit
		tabs.write[max_drawn_tab].ofs_cache = 0;

		if (!tabs[max_drawn_tab].hidden) {
			w -= tabs[max_drawn_tab].size_cache;
			w -= theme_cache.tab_separation; // We're repeatedly removing drawn tabs, so we have to remove their associated tab separation too
		}

		max_drawn_tab--;
	}
}

I put comments discussing my understanding. Is that understanding wrong?

[aaronjyoder]

I figured it out.

if (tabs[i].hidden) {
	continue;
}

w += tabs[i].size_cache;
if (i > 0) { // Don't add tab_separation for the first tab.
	w += theme_cache.tab_separation; // If i=1, we have 2 tabs, so add tab separation once. Going forward, this will add the correct amount of separation.
}

// Check if all tabs would fit inside the area.
if (clip_tabs && i > offset && (w > limit || (offset > 0 && w > limit_minus_buttons))) { // If you pass this check, you exceed the length, and have to clip a tab
	tabs.write[i].ofs_cache = 0;

	// We know i cannot be 0 if we are in here
	w -= tabs[i].size_cache;
	w -= theme_cache.tab_separation; // We're definitely drawing at least one fewer tab, so remove the tab separation we just added

	max_drawn_tab = i - 1;

	while (w > limit_minus_buttons && max_drawn_tab > offset) { // remove tabs (and their width from accumulated width w) until we are below the width limit
		tabs.write[max_drawn_tab].ofs_cache = 0;

		if (!tabs[max_drawn_tab].hidden) {
			w -= tabs[max_drawn_tab].size_cache;
			w -= theme_cache.tab_separation; // We're repeatedly removing drawn tabs, so we have to remove their associated tab separation too
		}

		max_drawn_tab--;
	}
}

Pushing now.

[aaronjyoder]

I will look at the other issues now and update the MR as I get to them:
(1) Enforcing a clamp on values when clip_tabs is false such that tabs cannot exceed container width.
(2) Tabs moving when they shouldn't as you scroll through pages.
(3) Tab touch targets not being updated correctly (and are therefore incorrect, despite the tabs visually being correct)
(4) Center/Right alignment are no longer quite correct--unselected tabs are displayed correctly for both, but the currently-selected tab is off by a bit (seems to subtract too much from x pos) and this also affects hover target.

If there is anything else, please let me know.

May be worth refactoring what I did in the cache to update the offset and see if that fixes some issues in one fell swoop.
I now primarily adjust the values through tabs[i].ofs_cache so things are handled more gracefully throughout. There's a few things where that didn't work, so in those places I manually inserted tab separation adjustments.

[aaronjyoder]

Alright, got most of the kinks sorted out. Last thing to do is enforcing a clamp on tab_separation values when clipping is turned off, and figuring out why tabs are shifting right when you switch pages with clipping turned on. If anyone can help with either, I am all ears.

[aaronjyoder]

Alright, so I have tab clipping fully working with right alignment and center alignment. However, I cannot figure out how to get it to work with left alignment. It should just be a matter of figuring out the right math for tab_separation_offset. I messed around with it a bit but I need to give it a rest for now.

[aaronjyoder]

Okay nvm I figured it out immediately after. Everything works now! Last thing to do would be a potential clamp, but let me know exactly how you might want to handle that. I could set some kind of clamp everywhere that I use theme_cache.tab_separation but that seems wrong to me.

I also realized that the clamp should not be based on tab width, as tabs can be variable width, and having each tab have its own unique minimum separation value leads to a bit of strangeness. I attempted to base it off of the minimum container width (divided by number of visible tabs), but it seems like that just caused crashes on load despite no compilation errors, so not sure what that's about.

I'd also be happy to just clamp it to something like -16 to <insert suggested max positive value here> for now and figure out a better solution later. It is not ideal because the negative clamp you'd want would depend on the size of your tabs to a degree, but something like -16 to -32 would cover most people's use-cases for why they'd want a negative tab separation value. With reasonable negative values, tabs still work exactly as you'd expect.

[AThousandShips]

Please avoid pushing so many times in a row, you currently require approval to run CI but if you had made contributions in the past you would have run CI several times

[aaronjyoder]

Sorry about that, that makes sense.

[aaronjyoder]

Alright, I think I've got it sorted now. I ran pre-commit locally before pushing, so hopefully that alleviates issues there.

I made an adjustment to the drag-and-drop calculations since it wasn't accounting for RTL correctly before, but should be now. I also changed the sqrt function call to Math::sqrt instead of std::sqrt.

With regard to a clamp: I tested several other UI parameters, and it appears most of them do not have clamps of any kind, and let you go into the negatives with abandon, so I decided not to implement any kind of clamp in order to maintain consistency with that.

[Calinou]

Tested locally, it works as expected (including drag-and-drop rearranging). Code looks good to me.

Testing project: test_pr_103214.zip

Note that clicking empty spaces between tabs won't select any tabs, but it'll focus the TabContainer/TabBar. This also occurred when you click space at the right of the last tab before this PR.

I agree with not preventing negative spacing, as we allow it for other theme constants like line spacing.

[KoBeWi]

Separation still has problems:

godot.windows.editor.dev.x86_64_XgLNbLC4UY.mp4

There is no wrong offsets, but the TabBar should display 2 tabs on every page.

[aaronjyoder]

There is a regression that was introduced at some point where tabs beyond the first page (with clipped tabs on) are not obeying center/right alignment. I swear I checked that just about every single commit but nonetheless. Looking into it now.

[aaronjyoder]

Regression fixed. I am unsure how best to ensure 2 tabs are always visible, but I'll think on it for a bit. Currently, it is guaranteed at least one tab will display on each page (prior to me fixing that regression, it was not guaranteed).

The problem in your above example is that due to the text, tabs can be variable width from each other.

The most straightforward solution that comes to mind is to, when clipping is on, calculate the maximum separation possible (based on container width) between any 2 pairs of consecutive tabs, and then clamp the separation to the smallest of these values. This would ensure that even in the worst case scenario, 2 tabs are visible, and it would keep the separation consistent on each page. However, I could also see the user intending for there to instead be maximum separation on each page (thus leading to inconsistent separation values on each page), which means we would only have to clamp the separation to the maximum width of the container minus the width of the visible tabs.

[KoBeWi]

The bug I mentioned before is fixed now.
But I found another one 🙃

jDtcF2M2lH.mp4

[aaronjyoder]

Whoops! Looks like fixing that other regression also requires me to adjust the exception for how left alignment is handled around line 470 (and line 1715 to match). 😄 Fixed locally, will wait to push until I implement a solution for the 2 tab on each page requirement.

[KoBeWi]

until I implement a solution for the 2 tab on each page requirement.

I already mentioned that it seems to be fixed already (last time I tested). Can you still reproduce it?

[aaronjyoder]

Can you still reproduce it?

Well, yes, if you increase separation to a very large value, you can ensure there is only 1 tab on each page, rather than 2. Is that not what you meant?

[aaronjyoder]

Aside from the 2 tabs on every page issue you mentioned earlier, everything should be functioning correctly. Let me know if there is anything I missed. If you are good with this as-is, then I suppose we will be good to go.

[KoBeWi]

I meant that there was less tabs per page than could fit. If separation is big enough that no other tab can be added then it's fine.
Sorry if I was unclear, I didn't mean that there should be strictly 2 tabs. I was only referring to the case shown in my video.

[aaronjyoder]

Ah okay I understand. I think that should not be an issue, but again it does depend on the text inside of each tab, as tabs can be variable width. It is entirely possible to create a scenario where there are 2 tabs on one page, but only 1 on the next, due to variable width tabs. For example, create 3 tabs, and increase the separation until only tab 1 and 2 are visible. Ensure tab 3 is extra-long with lots of text.

The user-side solution would be to decrease tab separation in that case.

[KoBeWi]

Looks good now.

[Repiteo]

Thanks! Congratulations on your first merged contribution! 🎉