Inconsistent margin spacing in widgets

[thorlucas]

Describe the bug
Widgets have inconsistent spacing on the margins in the blocks. Some widgets, for example the Tabs, have one column of spacing, while others like Table have no spacing. This just doesn't look right.

Expected behavior
All widgets should have consistent spacing (one or none) and optionally should be defined by the user for greatest flexibility.

Screenshots

[fdehau]

I understand the issue. I think that a zero margin may be the best convention as it is adopted by most widgets at the moment. To my knowledge, only the Tabs widget has a default horizontal margin. The fix would be to:

• Add a Margin property to the widgets and default to zero margin in the Default implementation.
• Make it configurable through the builder.
• Take those margins into account in Tabs::render.
• Add one or two tests in a tests/widgets_tabs.rs.

Do you have an interest in tackling this particular issue ? Anyone whose reading could also take the lead if they want until I manage to get some time to look at it.

[thorlucas]

I can certainly give it a shot! Relatively new at Rust, but I think I'll manage just fine reading the implementation in the other widgets.

[thorlucas]

I've added in left margins and right margins are somewhat working, but there's a bit of an issue with right margins. Currently, tabs (and it appears other widgets too maybe?) simple write off of the buffer, not truncating the string if it's too long.

This causes the tab titles to write onto the edge of the block, like so (screenshot taken without any modifications):

Which is undesirable, and is made visually even worse when using right margins (screenshot taken after margin modifications, with margin size of 2):

The difficulty is due to how we want to handle truncation. With unicode some characters can occupy variable size, so it's not as simple as string slicing, as I'm sure you know. We could use something like unicode_truncate to handle this, but it could add a little bloating and it's certainly slower. The speed impact shouldn't be too bad if we first check if truncation is required before doing so, but I don't want to add dependencies without approval.

[fdehau]

The functionality you are asking for is already provided by Buffer::set_stringn which take an additional width parameter that is your width "budget" to draw the string. If the budget is exhausted the function stops printing. This width budget is usually the diff between the right most bound of the area you are drawing too and your current horizontal offset.

[thorlucas]

Great! All tests are passing and it looks good.

Would you like me to submit a PR of just the tabs widget, or should I implement margins on all other widgets before submitting PR?

[teohhanhui]

Margins (actually padding in CSS terms) on any widget would be so useful! ❤️

[thorlucas]

@teohhanhui Well you're in luck because I'm working on it actively and it should be done fairly soon! Adding vertical and horizontal margins. Done so far on tabs, working on the rest of the widgets now.

[thorlucas]

I've ran into an ambiguity and I'd like to poll the community to see how it should be handled:

Tables and lists have a selector. Right now, the default functionality is to provide a sort of margin on the left to accommodate the width of the selector. It looks like this:

The question is what to do when it comes to margins. There are a few options:

1. Enforce margins. Throw an error if the horizontal margin provided is not sufficient to fit the selector.

2. Add margins. Simply add the width of the selector and the margin. If you have a margin width of 2, and a selector width of 1, the final left margin would be 3.

3. Max margins (probably best). If you have a margin width of 2, and a selector of 1, the margin is 2. If your selector is instead 3 wide (bigger than 2), then the left margin will be 3, but the right margin will remain 2.

Additionally, there's a question of what to do about the header. Currently, the selector margin only affects the data, not the header. This looks pretty bad IMO. If we went with a solution like 3, do you think that the header margin should also reflect the resulting margin? I.e., if you set a margin of 0, but your selector width is 2, should the header also have a margin of 2? Or should the header margin only be the specified horizontal margin?

[teohhanhui]

Option 4: Add the selector width to the content width, i.e. just treat the selector as if it's part of the row's content. Personally, I think this is the most intuitive and predictable behaviour. Options 2 and 3 are a bit surprising (too much magic, which makes it harder to have precise control).

Currently, the selector margin only affects the data, not the header. ... If we went with a solution like 3, do you think that the header margin should also reflect the resulting margin?

We can add the selector width to the content width, when going with option 4.

[fdehau]

@thorlucas I would prefer separate PRs as I don't think that we have the same problem (if any) on all widgets. Regarding your observation on the table margins (or padding, it's true that the name can be confusing for people used to css), an error should never be thrown on narrow areas (panics are bugs) the rendering should simply be skipped and the margin may not take the selector into account if we want a similar behavior across widgets. The first thing that comes to my mind would be (excuse the ugly diagram):

[teohhanhui]

@fdehau That diagram is the same as what I've described as option 4.

[fdehau]

@teohhanhui Yeah sorry I went through your comment a bit too quickly and understood something slightly different. I'm glad we agree :).

[thorlucas]

Option 4 it is! That’s actually equivalent to what I said in my option 2 — just add the two widths for the left margin.

@fdehau you would prefer separate PR’s for each widget? I was just going to make all widgets work consistently in terms of specifying margins.

I can make the bug fix that closes this issue one PR (changes tabs margins to 0), and a separate PR to add customizable margins to all widgets, if that’s what you mean?