fix(builder): Respect label-ellipsis option #1198

patrick96 · 2018-04-28T21:35:49Z

As described in #1194 polybar never actually checks the m_ellipsis flag
in labels.

Right now, I have only refactored the builder to determine the actual
label text inside a function and added failing tests for that bug and
the general expected behaviour of get_label_text. This is why the
tests in the current build will fail.

This will probably decrease test coverage quite drastically, as codecov
doesn't track builder.cpp yet, so this basically adds a bunch of not
covered lines that codecov is now tracking

Also @NBonaparte, do you think the builder should throw an exception,
if the maxlen is less than the length of the ellipsis and ellipsis is
enabled, because it would be impossible to follow both the label-maxlen
and the label-ellipsis directive?

EDIT1: Seems like the new test doesn't build on travis, and for some reason the travis build doesn't fail when this happens.
EDIT2: I have fixed the compilation error, now the test should fail for the right reasons
EDIT3: Fixes #1194

Adds failing tests for the bug described in polybar#1194

codecov-io · 2018-05-06T20:06:26Z

Codecov Report

Merging #1198 into master will decrease coverage by 52.33%.
The diff coverage is 46.66%.

@@            Coverage Diff             @@
##           master   #1198       +/-   ##
==========================================
- Coverage    57.9%   5.57%   -52.34%     
==========================================
  Files          24     163      +139     
  Lines         734    9532     +8798     
==========================================
+ Hits          425     531      +106     
- Misses        309    9001     +8692

Flag	Coverage Δ
#unittests	`5.57% <46.66%> (-52.34%)`	⬇️

Impacted Files	Coverage Δ
include/components/builder.hpp	`100% <ø> (ø)`
src/drawtypes/label.cpp	`2.2% <0%> (ø)`
include/drawtypes/label.hpp	`80% <100%> (ø)`
src/components/builder.cpp	`5.88% <71.42%> (ø)`
include/components/logger.hpp	`43.75% <0%> (-34.03%)`	⬇️
include/utils/file.hpp	`75% <0%> (-25%)`	⬇️
include/utils/factory.hpp	`87.5% <0%> (-12.5%)`	⬇️
include/errors.hpp	`33.33% <0%> (-6.67%)`	⬇️
include/utils/math.hpp	`90% <0%> (-6.43%)`	⬇️
src/components/parser.cpp	`0.94% <0%> (ø)`
... and 141 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 b70d5b5...af1646a. Read the comment docs.

patrick96 · 2018-05-06T21:31:16Z

Hold off on merging this, I think we should just log a warning and disable ellipsis, if the maxlen < 3

patrick96 · 2018-05-06T22:09:17Z

I think this is ready for review now.

NBonaparte

What if we used an ellipsis character ("…") instead of three periods?

patrick96 · 2018-05-26T22:54:03Z

What if we used an ellipsis character ("…") instead of three periods?

This will be what I implement next, in a separate PR, a config option to define the ellipsis.

ghost · 2018-05-29T12:39:20Z

Configurable ellipsis = #1195

ghost · 2018-05-29T12:59:04Z

I tested this patch as a user, I didn't find any issue. Beware that it changes behaviour (that's intended, and I requested this change in #1195): maxlen 4 used to yield abcd..., now it yields a...

patrick96 · 2018-05-29T19:22:06Z

The behavior change is quite minor and one could argue that it was a bug that with a maxlen of 4 the actual maxlen was 7. I don't think this warrants more than a note in the commit message gonna ammend right now.

This slightly changes the existing behavior of maxlen. Before with a maxlen of 4 'abcde' used to yield 'abcd...' now it yields 'a...'

The check of the maxlen and ellipsis condition was also moved to the label creation, this way get_label_text doesn't need to care about the restrictions placed on maxlen and ellipsis

NBonaparte · 2018-06-19T03:10:37Z

src/drawtypes/label.cpp

+    bool ellipsis = conf.get(section, name + "-ellipsis", true);
+
+    if(ellipsis && maxlen > 0 && maxlen < 3) {
+      logger::make().err(sstream() << "Label " << section << "." << name


Couldn't we throw an application_error or some other type of error to avoid creating a new logger?

logger::make uses the singleton pattern, so only the first call to logger::make() will create a new instance, all others always return the same instance. I used it directly because I didn't want to add the m_log field just for that.. Btw. I really don't like the use of a singleton for the logger but that is a discussion for another time.

If we threw an application_error here, wouldn't it be caught in the controller and would disable the whole module? I was debating, if we wanted invalid maxlen be a fatal error, would you prefer that?

Yeah, I think that would be much neater.

Alright, gonna revert back to throwing an error when I have the time

This now makes label-maxlen < len(ellipsis) (= 3) a fatal error disabling the whole module that uses the offending label

Breaking Changes: * `0 < label-NAME-maxlen < 3` will now throw an exception and disable the containing module, if ellipsis is enabled for that label. (#1198) Changelog: Deprecations: * `internal/volume` is now called `internal/alsa` (#967) * temperature: The `%temperature%` is deprecated in favor of `%temperature-c%`(#897) * mpd: `icon-repeatone` is deprecated in favor of `icon-single` (#1295), see #1279 Features: * feat(mpd): Add support for icon-consume (#861) * feat(bspwm): Add workspace separator (#942) * feat(i3): Add workspace separator (#938), see #929 * feat(build): Make polybar build on FreeBSD (#931, polybar/xpp#8), see #239 * feat(volume): Add pulseaudio backend (#779) * feat(script): Add %pid% token for tail commands (#934) * feat(temp): Add temperature tokens without unit (#897) * feat(memory): Add memory used/free ramp (#1038), see #1037 * feat(memory): Add swap tokens (#1018) * feat(net): Add unknown-as-up option (#1077), see #457 * feat(config): Support fractional size and offset (#972), see #953 * feat(xwindow): Add label-empty (#1136) * feat(battery): Add animation-discharging (analog to animation-charging) (#1190) * feat(config): Support pixel offset for bar size and offset values (#1224) * feat(mpd): Add `%album-artist%` token (#1263) * feat(net): Add local_ip6 token (#1239), see #1234 * feat(net): Add nl80211 support (#1009), see #277 Fixes: * fix(mpd): Wrong elapsed time when after standby (#921), see #915 * fix(config): Wrong min, maxlen when using the same token multiple times (#974), see #971 * fix(battery): use power_now correctly (#958), see #928 * fix(mpd): Crash when mpd isn't running (#983), see #979 * fix(xworkspaces): Respect 'enable-scroll' (#1002) * fix(xbacklight): Respect 'enable-scroll' (#1014) * fix(build): support xcb-proto >=1.13 (polybar/xpp#11), see #973 * fix(mpd): Respect MPD_HOST env variable (#1025), see #1007 * fix(i3): Reconnect i3 IPC socket on restart/error (#1099), see #762 * fix(cursor): Occasional crash on mouseover (#1124), see #1117 * fix(net): Mark 'not connected' on querying failure (#1171), see #1163 * fix(gcc): Fix -Wstringop-truncation warning (#1216, polybar/i3ipcpp#7), see #1215 * fix(builder): Don't truncate colors with same channels (#1217), see #1183 * fix(bspwm): Consistent behavior when scrolling through multiple desktops (#986), see #981 * fix(builder): Respect label-ellipsis option (#1198), see #1194

patrick96 force-pushed the issue/1194 branch 2 times, most recently from 8c18e50 to e7bb4ce Compare April 28, 2018 22:01

refactor(builder): Add failing test for ellipsis

3242692

Adds failing tests for the bug described in polybar#1194

patrick96 force-pushed the issue/1194 branch 3 times, most recently from ae6b9f2 to 32e9aab Compare April 28, 2018 22:37

fix(travis): Fail tests if build fails

59e3022

patrick96 force-pushed the issue/1194 branch from 32e9aab to 59e3022 Compare April 28, 2018 22:38

patrick96 changed the title ~~[WIP] fix(builder): Respect label-ellipsis option~~ fix(builder): Respect label-ellipsis option May 6, 2018

patrick96 requested a review from NBonaparte May 6, 2018 20:07

NBonaparte reviewed May 26, 2018

View reviewed changes

ghost mentioned this pull request May 29, 2018

feat(builder) minlen #1261

Closed

1 task

patrick96 added 2 commits May 29, 2018 21:24

fix(builder): Respect label-ellipsis option

4933e6b

This slightly changes the existing behavior of maxlen. Before with a maxlen of 4 'abcde' used to yield 'abcd...' now it yields 'a...'

refactor(builder): No exception when maxlen < 3

5b6e00a

The check of the maxlen and ellipsis condition was also moved to the label creation, this way get_label_text doesn't need to care about the restrictions placed on maxlen and ellipsis

patrick96 force-pushed the issue/1194 branch from 6f0a1df to 5b6e00a Compare May 29, 2018 19:25

NBonaparte reviewed Jun 19, 2018

View reviewed changes

refactor(label): Throw exception with small maxlen

af1646a

This now makes label-maxlen < len(ellipsis) (= 3) a fatal error disabling the whole module that uses the offending label

patrick96 requested a review from NBonaparte June 23, 2018 12:28

patrick96 added this to the 3.2.0 milestone Jul 22, 2018

patrick96 mentioned this pull request Jul 22, 2018

Release 3.2.0 #1338

Merged

7 tasks

NBonaparte approved these changes Jul 22, 2018

View reviewed changes

NBonaparte merged commit 4a494e6 into polybar:master Jul 22, 2018

patrick96 mentioned this pull request Jul 22, 2018

Zero padding #1332

Closed

kronn mentioned this pull request Sep 19, 2018

Make ellipsis string configurable #1195

Open

4 tasks

patrick96 deleted the issue/1194 branch November 15, 2018 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(builder): Respect label-ellipsis option #1198

fix(builder): Respect label-ellipsis option #1198

patrick96 commented Apr 28, 2018 •

edited

codecov-io commented May 6, 2018 •

edited

patrick96 commented May 6, 2018

patrick96 commented May 6, 2018

NBonaparte left a comment

patrick96 commented May 26, 2018

ghost commented May 29, 2018

ghost commented May 29, 2018

patrick96 commented May 29, 2018

NBonaparte Jun 19, 2018

patrick96 Jun 19, 2018

NBonaparte Jun 21, 2018

patrick96 Jun 21, 2018

patrick96 Jun 23, 2018

fix(builder): Respect label-ellipsis option #1198

fix(builder): Respect label-ellipsis option #1198

Conversation

patrick96 commented Apr 28, 2018 • edited

codecov-io commented May 6, 2018 • edited

Codecov Report

patrick96 commented May 6, 2018

patrick96 commented May 6, 2018

NBonaparte left a comment

Choose a reason for hiding this comment

patrick96 commented May 26, 2018

ghost commented May 29, 2018

ghost commented May 29, 2018

patrick96 commented May 29, 2018

NBonaparte Jun 19, 2018

Choose a reason for hiding this comment

patrick96 Jun 19, 2018

Choose a reason for hiding this comment

NBonaparte Jun 21, 2018

Choose a reason for hiding this comment

patrick96 Jun 21, 2018

Choose a reason for hiding this comment

patrick96 Jun 23, 2018

Choose a reason for hiding this comment

patrick96 commented Apr 28, 2018 •

edited

codecov-io commented May 6, 2018 •

edited