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

Align sidebar widget values to the left by default #54905

Merged
merged 2 commits into from
Jan 31, 2022

Conversation

wapcaplet
Copy link
Contributor

@wapcaplet wapcaplet commented Jan 29, 2022

Summary

Features "Align sidebar widget values to the left by default"

Purpose of change

Widget values are often too far away from their labels with the default alignment (labels to the left, values to the right).

This also causes widgets to look quite different from the old hardcoded ones players may be more familiar with.

Describe the solution

  • Make text_align be left by default, the same as label_align. This is a 1-line change.
  • Adjust the documentation to match. This is also a 1-line change.
  • Adjust the many test cases that were alignment-dependent. This part is... more lines.

Describe alternatives you've considered

I do think the right-aligned (full-justified) values are more aesthetically pleasing and balanced-looking, but when used indiscriminately down the whole sidebar (as they currently are) it leads to this big chasm of black-space that is difficult for the eye to jump across.

If the sub-sections were more carefully crafted to fill out their horizontal space better, then the full-justified alignment could be applied to those sections for a better overall result. But with an arbitrary amount of space between label and value, this kind of alignment makes a poor default.

Testing

  • Configure sidebar with } and switch to "custom"
  • Enable sections and see values aligned to the left (closer to their labels) by default

Additional context

Before and after:

image

@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 29, 2022
@wapcaplet wapcaplet added [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. Info / User Interface Game - player communication, menus, etc. labels Jan 29, 2022
@Faalagorn
Copy link
Contributor

I see that in your screenshot some values are also cut off (speed to be exact). Is there an option to fix that in that PR or is it out of the scope of it? I also mentioned the issue briefly in Discord's #localization channel.
screenshot_2022-01-29-191521
(top and bottom)

@wapcaplet
Copy link
Contributor Author

I see that in your screenshot some values are also cut off (speed to be exact). Is there an option to fix that in that PR or is it out of the scope of it? I also mentioned the issue briefly in Discord's #localization channel.

Good catch - I noticed something similar going on with the "Vital Numbers" section; I think it's an unwanted side-effect of #54628 which added the alignment options, but haven't looked into the cause/solution. I think it's out of the scope of this PR, but definitely needs fixing. @dseguin let me know if you have any ideas.

@Faalagorn
Copy link
Contributor

Faalagorn commented Jan 29, 2022

Yeah, vitals are another one. There's also a bug where translations sometimes move items around, even if there should be a space, e.g. look at "Nastrój" (Mood) in Polish, it moves the face around causing "Skup." (Focus) to move, even if I change language back (compare to your screenshots when it's all fine)

screenshot_2022-01-29-200819
screenshot_2022-01-29-200913

While restarting the game "fixes" it, it results in another error, causing the game to reset the sidebar

 DEBUG    : Invalid current panel layout, defaulting to classic

 FUNCTION : panel_layout& panel_manager::get_current_layout()
 FILE     : src/panels.cpp
 LINE     : 1978
 VERSION  : 0.C-67768-gfaf8d153d7

(ignore the 0.C version, it's from a PKGBUILD which I forgot to update)

Actually, when the game is first launched in Polish, then changed to English the bug happens. When opposite, i.e. the game is launched in English and then language is switched to Polish, then it's all fine:
screenshot_2022-01-29-201459

@dseguin
Copy link
Member

dseguin commented Jan 29, 2022

So some of this is a side-effect of the alignment, since widgets in the same "rows" layout will try to align with each other using the longest label as a guide:

rows_layout

That also means that if the label is really long, left-aligned widgets in the "rows" layout are pushed even further into the next column's space:

rows_layout2

I think that a reasonable fix would be to cut off the widget if it crosses it's max width, ex:

rows_layout3

The other stuff is probably due to some jankiness in the padding code (which is kinda messy at the moment). I refactored a lot of it in #54693, but I can break that out into a separate PR if it fixes the issue.

@dseguin
Copy link
Member

dseguin commented Jan 29, 2022

Well, that was easy :P

fix_layout

diff --git a/src/widget.cpp b/src/widget.cpp
index 9c708caff7..ee7fe5bac4 100644
--- a/src/widget.cpp
+++ b/src/widget.cpp
@@ -1302,6 +1302,6 @@ std::string widget::layout( const avatar &ava, const unsigned int max_width, int
                                 row_num == 0 ? label_width : 0, _text_align, _label_align );
         }
     }
-    return ret;
+    return ret.find( '\n' ) != std::string::npos ? ret : trim_by_length( ret, max_width );
 }

@Faalagorn
Copy link
Contributor

So some of this is a side-effect of the alignment, since widgets in the same "rows" layout will try to align with each other using the longest label as a guide:

rows_layout

That also means that if the label is really long, left-aligned widgets in the "rows" layout are pushed even further into the next column's space:

rows_layout2

I think that a reasonable fix would be to cut off the widget if it crosses it's max width, ex:

rows_layout3

The other stuff is probably due to some jankiness in the padding code (which is kinda messy at the moment). I refactored a lot of it in #54693, but I can break that out into a separate PR if it fixes the issue.

It would also be nice to mark all the strings for translation with the upper character limit for translators, if possible, that would ease up the translations for sure

@dseguin
Copy link
Member

dseguin commented Jan 29, 2022

Also, I was able to fix the general issue of values not having enough space, due to the alignment of the longest label:

vital_nums

I made it so that values start to eat the free space next to the label if they don't have enough room (leaving at least one space after the label).

I'll create a separate PR with the fixes.

@wapcaplet wapcaplet force-pushed the w-widget-align-left branch from 6b855b7 to 91e2aca Compare January 30, 2022 15:56
@wapcaplet
Copy link
Contributor Author

Revised "After" screenshot, after merging the fix for issue #54915:

image

@wapcaplet wapcaplet marked this pull request as ready for review January 30, 2022 16:01
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 30, 2022
@kevingranade kevingranade merged commit f210e96 into CleverRaven:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants