Add label minlen and alignment. #1546

infokiller · 2018-12-15T09:12:39Z

No description provided.

codecov-io · 2018-12-15T09:20:43Z

Codecov Report

Merging #1546 into master will decrease coverage by 0.04%.
The diff coverage is 56.31%.

@@            Coverage Diff            @@
##           master   #1546      +/-   ##
=========================================
- Coverage    9.29%   9.24%   -0.05%     
=========================================
  Files         180     180              
  Lines       10816   10888      +72     
=========================================
+ Hits         1005    1007       +2     
- Misses       9811    9881      +70

Flag	Coverage Δ
#unittests	`9.24% <56.31%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
include/components/builder.hpp	`0% <ø> (-100%)`	⬇️
src/components/builder.cpp	`0% <0%> (-10.51%)`	⬇️
include/drawtypes/label.hpp	`85.71% <100%> (ø)`	⬆️
src/drawtypes/label.cpp	`13.58% <54.05%> (+11.3%)`	⬆️
tests/unit_tests/drawtypes/label.cpp	`57.14% <57.14%> (ø)`
include/utils/factory.hpp	`71.42% <0%> (-28.58%)`	⬇️
include/components/types.hpp	`0% <0%> (-7.5%)`	⬇️
src/modules/pulseaudio.cpp	`0% <0%> (ø)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f09784...02e290f. Read the comment docs.

infokiller · 2018-12-15T09:28:48Z

Demo of how the different alignments look like:

patrick96

Hey, I can see this being useful, thanks!

Could you also add some tests for your code in label::get() to test that the minlen functionality works properly.

We unfortunately don't have any documentation yet for tests, but here is a quick rundown:

We use googletest as our testing framework.
You can build tests by configuring polybar with cmake -DBUILD_TESTS=ON .. and then running make all_unit_tests. You can run the whole test suite with ctest or you can also only run single tests by executing the corresponding executable in build/tests.

Have a look at the other tests and tests/CMakeLists.txt to see how to create new tests.

src/drawtypes/label.cpp

patrick96 · 2018-12-15T13:04:21Z

src/drawtypes/label.cpp

+      aligned_label = m_tokenized + string(num_fill_chars, ' ');
+    } else {
+      const string sided_fill_chars(std::ceil(num_fill_chars / 2.0), ' ');
+      aligned_label = sided_fill_chars + m_tokenized + sided_fill_chars;


for an uneven num_fill_chars, this would create a label that has length m_minlen + 1

Yes, I wasn't sure if I should round up or down in order to respect the center alignment.
If I round down, that means the length will be m_minlen - 1. Do you prefer that?

I think the following should always hold:

minlen <= len(effective label) <= maxlen

and if len(m_tokenized) < minlen, len(effective label) == minlen

In this case this would mean that we fill one additional space character, either on the left or on the right when num_fill_chars is odd. I would suggest adding it to the left side.
This could be generalized to say that the left side gets filled with std::ceil(num_fill_chars / 2.0) and the right side with std::floor(num_fill_chars / 2.0) spaces. If num_fill_chars is even those numbers are equal and if it's odd those numbers added together give you num_fill_chars again.

I think the following should always hold:

minlen <= len(effective label) <= maxlen

I agree.

and if len(m_tokenized) < minlen, len(effective label) == minlen

I think this is more debatable. The main issue I see is that even though a user requested center alignment, they will not get it. At least in my case, I would be more surprised to see non-centered alignment than len(m_tokenized) == minlen + 1, assuming minlen + 1 <= maxlen.
Also, doing it this way means that the user can't really guarantee center alignment, since they can't restrict the labels to an even/odd length (only restrict the min/max length).

In this case this would mean that we fill one additional space character, either on the left or on the right when num_fill_chars is odd. I would suggest adding it to the left side.
This could be generalized to say that the left side gets filled with std::ceil(num_fill_chars / 2.0) and the right side with std::floor(num_fill_chars / 2.0) spaces. If num_fill_chars is even those numbers are equal and if it's odd those numbers added together give you num_fill_chars again.

The main issue I see is that even though a user requested center alignment, they will not get it.

Fair point. I guess as long as the case where maxlen == minlen or maxlen == minlen + 1 is handled correctly, we could do it this way.

Just did some testing and minlen with center alignment causes some trouble.
In particular a label with text abc, minlen = 4, maxlen = 4, center alignmend, maxlen = 4 and ellipsis enabled will produce ... instead of abc or abc. I would suggest that in this special case the extra space is located at the end.

I would also suggest that we merge the functionality of this method and of builder::get_label_text to handle minlen, maxlen and ellipsis all in one function. Like this it's also easier to test this and how minlen and maxlen interact. We should probably move everything into label::get because that function is called in several places and should return the effective label in all those cases.

src/drawtypes/label.cpp

include/drawtypes/label.hpp

infokiller · 2018-12-15T18:55:48Z

Thanks for the good review!
Added unit tests.

patrick96

That was very quick!

Just the one small thing with center alignment, everything else looks good.

Will do some testing on my own and may write some more test cases after this is finalized.

patrick96

Props on adding tests :)

The main issue is now how minlen and maxlen interact when ellipsis is enabled.

patrick96 · 2019-01-18T15:38:02Z

src/drawtypes/label.cpp

+      aligned_label = m_tokenized + string(num_fill_chars, ' ');
+    } else {
+      const string sided_fill_chars(std::ceil(num_fill_chars / 2.0), ' ');
+      aligned_label = sided_fill_chars + m_tokenized + sided_fill_chars;


The main issue I see is that even though a user requested center alignment, they will not get it.

Fair point. I guess as long as the case where maxlen == minlen or maxlen == minlen + 1 is handled correctly, we could do it this way.

src/drawtypes/label.cpp

patrick96 · 2019-01-18T15:56:05Z

src/drawtypes/label.cpp

+      aligned_label = m_tokenized + string(num_fill_chars, ' ');
+    } else {
+      const string sided_fill_chars(std::ceil(num_fill_chars / 2.0), ' ');
+      aligned_label = sided_fill_chars + m_tokenized + sided_fill_chars;


Just did some testing and minlen with center alignment causes some trouble.
In particular a label with text abc, minlen = 4, maxlen = 4, center alignmend, maxlen = 4 and ellipsis enabled will produce ... instead of abc or abc. I would suggest that in this special case the extra space is located at the end.

I would also suggest that we merge the functionality of this method and of builder::get_label_text to handle minlen, maxlen and ellipsis all in one function. Like this it's also easier to test this and how minlen and maxlen interact. We should probably move everything into label::get because that function is called in several places and should return the effective label in all those cases.

Fix build

Co-Authored-By: infokiller <infokiller@users.noreply.github.com>

Also add a test for a test case brought up in the PR discussion.

infokiller · 2019-10-12T02:24:25Z

Apologies for the huge delay! I promise to respond fast this time :)
I've rebased the commits on top of the up to date master since a lot of time has passed.
I've also addressed the remaining comments.

infokiller · 2019-11-27T17:31:50Z

Is there anything I can do to get this PR merged?
Thanks!

patrick96

Sorry for the delay.

So the code looks good at first glance and it seems to work without issues, will dive deeper later and try to break it ;)

For now just some nitpicking:
Could you run clang-format on all the files you have changed to make them unformly formatted:
clang-format -style=file -i <FILE>

infokiller · 2019-11-28T13:19:27Z

Thanks for the quick response!
I formatted the files with clang-format and pushed a new commit.

patrick96 · 2019-11-28T16:39:55Z

include/drawtypes/label.hpp

     */
    size_t m_maxlen{0_z};
+    // The default is set to RIGHT for backwards compatibility.
+    alignment m_alignment{alignment::RIGHT};


We didn't have minlen before, why do we need to be backwards compatible here?

Am I forgetting something?

builder::get_label_text doesn't really do anything anymore

patrick96

I moved the unit tests for maxlen from the builder into the label and also used the parameterized style for your unit tests.

I couldn't find any bugs 👍

I have also cleaned up the get() code a little bit and removed builder::get_label_text.

The only thing left is, I don't understand why the default is right alignment, otherwise this is ready to be merged.

include/drawtypes/label.hpp

infokiller · 2019-11-28T22:36:15Z

Thanks a lot for the review and cleanup!
Looking at the comment "The default is set to RIGHT for backwards compatibility.", I actually don't remember why I wrote it, and it doesn't make any sense, because alignment is only relevant when there is a minimum length, and before this PR there was no minimum length...
So, should I set the default alignment to left?

patrick96 · 2019-11-28T23:21:20Z

The only place I could imagine this coming from is the minlen property on tokens where they get right aligned. That property was mainly intended for numbers I think where right alignment makes more sense (the ones, tens, hundreds, etc. always stay in the same place). But for labels left alignment makes more sense I think, so changing it to left would be better.

infokiller · 2019-11-28T23:37:07Z

Yes, I agree. I changed it to left.

patrick96

You forgot the default value in some places ;)

We really need to do this better at some point and have a single place where defaults are defined.

include/drawtypes/label.hpp

src/drawtypes/label.cpp

Co-Authored-By: Patrick Ziegler <p.ziegler96@gmail.com>

infokiller · 2019-12-01T00:21:16Z

Sorry I missed that, and thanks for fixing it.

patrick96

Alright. Looks good now. Thanks a lot 😃

Breaking Changes: * The new config parser imposes some restrictions on which characters can be used in section and key names. Users shouldn't be affected by this unless they use one of the following characters in any section or key name: `"'=;#[](){}:.$\%` Please consult [`man 5 polybar`](https://polybar.readthedocs.io/en/latest/man/polybar.5.html) for a full reference of the new config rules. * `internal/temperature`: The first and last ramp element are now only used for `base-temperature` and below and `warn-temperature` and above respectively. This only slightly changes the ranges for which the different ramp levels are responsible for. (#2197) Changelog **Deprecations** * `[settings]`: `throttle-input-for` has been removed. It wasn't a useful option and could cause certain actions (e.g. click commands) to be ignored. (#2117) * All action names used by modules to handle click and scroll events are depercated (#1907). This change mainly affects users of the menu module. Please read the [documentation](https://polybar.readthedocs.io/en/latest/user/actions.html) for instructions on how to migrate. **New Config Options** The `include-directory` key can be used the same as `include-file` and includes all regular files in the given directory. In labels: * `label-NAME-minlen`, `label-NAME-alignment` can be used to pad labels with spaces to the right (alignment set to `left`), left (alignment set to `right`), and both sides (alignment set to `center`). In `internal/backlight`: * `enable-scroll` enables changing the brightness through scrolling. In `internal/github`: * `format-offline` is used when the module cannot connect to the server. * `label-offline` can be used in `format-offline`. * `api-url` can be used to connect to a custom github enterprise instance In `internal/pulseaudio`: * `click-right` and `click-middle` can be used to execute commands when right/middle clicking. * `%decibels%` token can be used in `label-volume` and `label-muted` to show the volume in decibels. **Changes To The Build System** * Allow users to specify python executable when building. (polybar/xpp#27, #2125) * The i3ipcpp submodule no longer rebuilds jsoncpp and just uses whatever version is available. (#2015, polybar/i3ipcpp#9) **Features** * New commandline argument: `-M` / `--list-all-monitors`. Will display all available monitors (even cloned ones). * New log level: `notice`. Used as the default and is used for non-warning messages the user should nevertheless be aware of. (#2027) * config: * New config parser (#1377) * `include-directory` key (#2196), see #1946 * Add error message when an entire section is missing. (#2195) * `-minlen` and `-alignment` properties for labels. (#1546) * Make the `seperator` key in the bar section a label. (#1918) * Better color validation. (#1290) * timer modules: Schedule module updates to be aligned with the update interval. For example, the date module now updates on the minute instead of in the middle of a minute if the interval is set to 60 seconds. (#2123), see #2064 * `custom/menu`: Multiple menu modules per bar (#1907) * `internal/backlight`: Support for changing the brightness through scrolling. This may require additional changes to the system running polybar. (#1957) * `internal/github`: * `format-offline` for when the module cannot connect to the server (#1825), see #1815 * Support for github enterprise instances. (#1841), see #1812 * `internal/network`: Support `Gbit/s` for `%linkspeed%` token. (#2055) * `internal/pulseaudio`: * `click-right` and `click-middle` keys (#1941) * `%decibels%` token (#1894), see #1886 * `internal/xworkspaces`: Proper implementation for `label-occupied`. (#822), see #874, #1444, #1033 **Fixes** * Polybar not executing commands that produce output. (#1680), see #916 * Polybar froze until click commands finished executing (#2248) * Polybar not properly working with mirrored monitors. (#1823), see #1192 and #1794 * Unstable animation framerate (#1683), see #568 * Multiple modules of the same type caused click events not to be delivered to the rigth one (#1907), see #1172 * config: * Seemingly unrelated error messages when BOM character appears in config. (#2166), see #2075 * Fall back to next possible config location if config file doesn't exist (except if `--config` was used). (#2026), see #2016 * iconset: `fuzzy-match` chose first match, even if exact match was available. (#2042), see #2041 * `custom/menu`: Spacing issue (#1656) * `internal/alsa`: Volume didn't go over 100% (#2184), see #2173 * `internal/backlight`: Use amdgpu workaround for all devices starting with `amdgpu_bl`. (#2122) * `internal/battery`: Battery not marked as full if over `full-at` percent. (#2019), see #1622 * `internal/cpu`: More accurate cpu load calculation. (#1955) * `internal/github`: Outdated GitHub API authentication. (#2029), see #2002 * `internal/memory`: Use the correct size prefixes (#2211), see #2023 * `internal/network`: * Wrong up- and downspeed values for non-integer intervals. (#2219) * tun/tap interfaces didn't query their IP addresses. (#1990), see #1986 * Don't crash module if linkspeed cannot be queried. (#1772), see #1211 * `internal/temperature`: * `format-warn` was not used if the temperature was *exactly* `warn-temperature`. (#1897) * `internal/xworkspaces`: * Multi-monitor issue (#1929), see #1764, #1849 * Module sometimes showed too many workspaces (#1984), see #1983 * build: * xpp submodule doesn't work with python 3.9 (polybar/xpp#26) * CMake 3.17+ developer warnings (#2089, polybar/xpp#24, polybar/xpp#25) * gtest compilation failure (#1993), see google/googletest#2678 * Compilation issue in GCC 6. (#1953)

Breaking Changes: * The new config parser imposes some restrictions on which characters can be used in section and key names. Users shouldn't be affected by, this unless they use one of the following characters in any section or key name: `"'=;#[](){}:.$\%` Please consult [`man 5 polybar`](https://polybar.readthedocs.io/en/latest/man/polybar.5.html) for a full reference of the new config rules. * `internal/temperature`: The first and last ramp element are now only used for `base-temperature` and below and `warn-temperature` and above respectively. This only slightly changes the ranges for which the different ramp levels are responsible for. (#2197) Changelog **Deprecations** * `[settings]`: `throttle-input-for` has been removed. It wasn't a useful option and could cause certain actions (e.g. click commands) to be ignored. (#2117) * All action names used by modules to handle click and scroll events are deprecated (#1907). This change mainly affects users of the menu module. Please read the [documentation](https://polybar.readthedocs.io/en/latest/user/actions.html) for instructions on how to migrate. **New Config Options** The `include-directory` key can be used the same as `include-file` and includes all regular files in the given directory. In labels: * `label-NAME-minlen`, `label-NAME-alignment` can be used to pad labels with spaces to the right (alignment set to `left`), left (alignment set to `right`), and both sides (alignment set to `center`). In `internal/backlight`: * `enable-scroll` enables changing the brightness through scrolling. In `internal/github`: * `format-offline` is used when the module cannot connect to the server. * `label-offline` can be used in `format-offline`. * `api-url` can be used to connect to a custom github enterprise instance In `internal/pulseaudio`: * `click-right` and `click-middle` can be used to execute commands when right/middle clicking. * `%decibels%` token can be used in `label-volume` and `label-muted` to show the volume in decibels. **Changes To The Build System** * Allow users to specify python executable when building. (polybar/xpp#27, #2125) * The i3ipcpp submodule no longer rebuilds jsoncpp and just uses whatever version is available. (#2015, polybar/i3ipcpp#9) **Features** * New commandline argument: `-M` / `--list-all-monitors`. Will display all available monitors (even cloned ones). * New log level: `notice`. Used as the default and is used for non-warning messages the user should nevertheless be aware of. (#2027) * config: * New config parser (#1377) * `include-directory` key (#2196), see #1946 * Add error message when an entire section is missing. (#2195) * `-minlen` and `-alignment` properties for labels. (#1546) * Make the `seperator` key in the bar section a label. (#1918) * Better color validation. (#1290) * timer modules: Schedule module updates to be aligned with the update interval. For example, the date module now updates on the minute instead of in the middle of a minute if the interval is set to 60 seconds. (#2123), see #2064 * `custom/menu`: Multiple menu modules per bar (#1907) * `internal/backlight`: Support for changing the brightness through scrolling. This may require additional changes to the system running polybar. (#1957) * `internal/github`: * `format-offline` for when the module cannot connect to the server (#1825), see #1815 * Support for github enterprise instances. (#1841), see #1812 * `internal/network`: Support `Gbit/s` for `%linkspeed%` token. (#2055) * `internal/pulseaudio`: * `click-right` and `click-middle` keys (#1941) * `%decibels%` token (#1894), see #1886 * `internal/xworkspaces`: Proper implementation for `label-occupied`. (#822), see #874, #1444, #1033 **Fixes** * Polybar not executing commands that produce output. (#1680), see #916 * Polybar froze until click commands finished executing (#2248) * Polybar not properly working with mirrored monitors. (#1823), see #1192 and #1794 * Unstable animation framerate (#1683), see #568 * Multiple modules of the same type caused click events not to be delivered to the rigth one (#1907), see #1172 * config: * Seemingly unrelated error messages when BOM character appears in config. (#2166), see #2075 * Fall back to next possible config location if config file doesn't exist (except if `--config` was used). (#2026), see #2016 * iconset: `fuzzy-match` chose first match, even if exact match was available. (#2042), see #2041 * `custom/menu`: Spacing issue (#1656) * `internal/alsa`: Volume didn't go over 100% (#2184), see #2173 * `internal/backlight`: Use amdgpu workaround for all devices starting with `amdgpu_bl`. (#2122) * `internal/battery`: Battery not marked as full if over `full-at` percent. (#2019), see #1622 * `internal/cpu`: More accurate cpu load calculation. (#1955) * `internal/github`: Outdated GitHub API authentication. (#2029), see #2002 * `internal/memory`: Use the correct size prefixes (#2211), see #2023 * `internal/network`: * Wrong up- and downspeed values for non-integer intervals. (#2219) * tun/tap interfaces didn't query their IP addresses. (#1990), see #1986 * Don't crash module if linkspeed cannot be queried. (#1772), see #1211 * `internal/temperature`: * `format-warn` was not used if the temperature was *exactly* `warn-temperature`. (#1897) * `internal/xworkspaces`: * Multi-monitor issue (#1929), see #1764, #1849 * Module sometimes showed too many workspaces (#1984), see #1983 * build: * xpp submodule doesn't work with python 3.9 (polybar/xpp#26) * CMake 3.17+ developer warnings (#2089, polybar/xpp#24, polybar/xpp#25) * gtest compilation failure (#1993), see google/googletest#2678 * Compilation issue in GCC 6. (#1953)

infokiller force-pushed the label-alignment branch from 0855a8f to 677d8f0 Compare December 15, 2018 09:14

patrick96 requested changes Dec 15, 2018

View reviewed changes

patrick96 requested changes Dec 16, 2018

View reviewed changes

patrick96 requested changes Jan 18, 2019

View reviewed changes

patrick96 mentioned this pull request Feb 8, 2019

feat(builder) minlen #1261

Closed

1 task

infokiller force-pushed the label-alignment branch 2 times, most recently from 31aae3b to 4e3bcd1 Compare July 8, 2019 15:10

infokiller and others added 6 commits October 12, 2019 02:40

Add label minlen and alignment.

81cb496

Fix build

Update src/drawtypes/label.cpp

9ccee0f

Co-Authored-By: infokiller <infokiller@users.noreply.github.com>

Use existing alignment type.

d012b2d

Remove redundant max_len handling in label::get.

38c4d63

Fix shadowing.

7ff1d5d

Add label alignment tests.

749a6ac

infokiller force-pushed the label-alignment branch from 4e3bcd1 to 749a6ac Compare October 11, 2019 23:59

Handle minlen/maxlen and alignment in same function.

b8a459b

Also add a test for a test case brought up in the PR discussion.

infokiller requested a review from patrick96 October 24, 2019 08:17

patrick96 requested changes Nov 27, 2019

View reviewed changes

infokiller added 2 commits November 28, 2019 15:16

Merge remote-tracking branch 'upstream/master' into label-alignment

0626ee2

Format files with clang-format

294f276

patrick96 reviewed Nov 28, 2019

View reviewed changes

patrick96 added 3 commits November 28, 2019 18:40

Move builder::get_label_text tests into label tests

4415276

builder::get_label_text doesn't really do anything anymore

builder: remove get_label_text

e4b8ce7

label: Clean up label::get()

996349c

patrick96 requested changes Nov 28, 2019

View reviewed changes

include/drawtypes/label.hpp Outdated Show resolved Hide resolved

Fix comment style.

8f147dc

Set default label alignment to left.

9082ee9

patrick96 requested changes Nov 29, 2019

View reviewed changes

include/drawtypes/label.hpp Outdated Show resolved Hide resolved

src/drawtypes/label.cpp Outdated Show resolved Hide resolved

infokiller and others added 2 commits December 1, 2019 02:11

Update src/drawtypes/label.cpp

096cbfa

Co-Authored-By: Patrick Ziegler <p.ziegler96@gmail.com>

Update include/drawtypes/label.hpp

02e290f

Co-Authored-By: Patrick Ziegler <p.ziegler96@gmail.com>

patrick96 approved these changes Dec 1, 2019

View reviewed changes

patrick96 merged commit fb6e874 into polybar:master Dec 1, 2019

patrick96 mentioned this pull request Nov 29, 2020

Release 3.5.0 #2253

Merged

28 tasks

Uh oh!

Add label minlen and alignment. #1546

Add label minlen and alignment. #1546

Uh oh!

Conversation

infokiller commented Dec 15, 2018

Uh oh!

codecov-io commented Dec 15, 2018 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

infokiller commented Dec 15, 2018

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

infokiller commented Dec 15, 2018

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infokiller commented Oct 12, 2019

Uh oh!

infokiller commented Nov 27, 2019

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

infokiller commented Nov 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrick96 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

infokiller commented Nov 28, 2019

Uh oh!

patrick96 commented Nov 28, 2019

Uh oh!

infokiller commented Nov 28, 2019

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

infokiller commented Dec 1, 2019

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

codecov-io commented Dec 15, 2018 •

edited by codecov bot

Loading

patrick96 left a comment •

edited

Loading