Make text module a label #1342

memeplex · 2018-07-22T22:43:41Z

Reviewer Note: This PR introduces breaking changes to the text module. It should be merged in the merge window where breaking changes can be introduced for the next major release (generally directly before the next major release).

refactor(text): Make content a label (#1342)

This adds the `format` key to the text module with `<content>` as its 
only (label) tag.

This change is not backwards compatible for all configs that make use of 
the `-margin`, `-spacing`, `-prefix` or `-suffix` properties of 
`content`, because `content` is converted from a format to a label.
It would be otherwise because by default `format = <content>` and if 
only `content` is set, the module will behave as before.
This will allow users to use all the label formatting options in the 
text module as well.

Sample configuration:
[module/text]
type = custom/text
format = <content>
content = Content of the text module

Fixes #1331

codecov-io · 2018-07-22T22:49:12Z

Codecov Report

Merging #1342 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #1342      +/-   ##
=========================================
+ Coverage    5.94%   5.94%   +<.01%     
=========================================
  Files         161     161              
  Lines        8929    8928       -1     
=========================================
  Hits          531     531              
+ Misses       8398    8397       -1

Flag	Coverage Δ
#unittests	`5.94% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
include/modules/text.hpp	`0% <ø> (ø)`	⬆️
src/modules/text.cpp	`3.44% <0%> (-0.13%)`	⬇️
include/modules/meta/static_module.hpp	`0% <0%> (ø)`	⬆️

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 4cb0b18...6bc932f. Read the comment docs.

memeplex · 2018-07-22T22:49:44Z

I have a couple of questions about this:

I don't really know what would be m_label->replace_token(" ", BUILDER_SPACE_TOKEN); for, I just adapted it from the previous code, maybe it's superfluous.
Should I raise an error if the format is not exactly <content>? Or, less strictly, if the format doesn't contain <content> although it might not be exactly <content>? Or never rise an error, despite the contents of format?

memeplex · 2018-07-23T00:41:41Z

Well, I ended up implementing build since it seems the best way to avoid assuming too much about what will be in format. Nevertheless, the user is not intended to modify the default <content> but in case he/she does it everything should still work fine if:

The label content is defined
The format format includes <content>

Anything else will be reported as an error.

patrick96

Good work. This does make the text module more consistent with the others.

There seems to be some legacy code in the current text module, I have added a few comments about it.

The rest looks good and it shouldn't break any existing configs (unless someone has already defined the format key in their text module).

patrick96 · 2018-07-28T13:48:36Z

src/modules/text.cpp

-
-  string text_module::get_format() const {
-    return "content";
+    format->value = string_util::replace_all(format->value, " ", BUILDER_SPACE_TOKEN);


I have wondered about this before. I think we can remove it because the builder also does this.

patrick96 · 2018-07-28T14:02:20Z

src/modules/text.cpp

-    m_formatter->add("content", "", {});
-
-    if (m_formatter->get("content")->value.empty()) {
-      throw module_error(name() + ".content is empty or undefined");


This looks like very old code. No other module actually does that anymore. I don't think this needs to be adapted and can be removed. What all the other modules do, is to load the format, as you did, and if the label tag occurs in the format string, load the label. For example the script modules does it like this:

modules/script.cpp: ------------------- m_formatter->add(DEFAULT_FORMAT, TAG_LABEL, {TAG_LABEL}); if (m_formatter->has(TAG_LABEL)) { m_label = load_optional_label(m_conf, name(), "label", "%output%"); }

This avoids throwing unnecessary errors because one should be able to also define a format without including any tags (even if it's not that useful).

In your last example

if (m_formatter->has(TAG_LABEL))

I'm unsure about removing the DEFAULT_FORMAT argument, as in:

if (m_formatter->has(TAG_CONTENT, DEFAULT_FORMAT))

Because the formatter could contain more than one format and the only API I found to check if a tag is inside a format is to request the formatter for that tag in one of its formats. But maybe the dafault format is considered when no other is explicitly passed?

From looking at the code (base.cpp:143), if you pass only one argument to module_formatter::has, then it searches through all the defined formats. So in this case it shouldn't matter, because the text module only defines a single format format.

Ok, I've already updated and rebased everything according to your comments.

Fixes #1331

patrick96 · 2018-07-29T13:30:40Z

I just realised that this breaks compatibility for all configs that use content-prefix, -suffix, -spacing, or -margin because right now content is a format and afterwards it would be a label, which doesn't have all those properties. We will keep this PR open and will merge it for the 4.0.0 release. This may be a while though.

memeplex · 2018-07-29T13:37:50Z

Yeah, I commented a bit about that in the issue, so I was expecting this to enter the next major version, if any. It's ok, since I posted I discovered many workarounds like prefix/suffix labels and font tags.

…

On Sun, Jul 29, 2018, 10:30 AM Patrick Ziegler ***@***.***> wrote: I just realised that this breaks compatibility for all configs that use content-prefix, -suffix, -spacing, or -margin because right now content is a format and afterwards it would be a label, which doesn't have all those properties. We will keep this PR open and will merge it for the 4.0.0 release. This may be a while though. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1342 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACtq-Z7HFtufcyRQ5DLoOQ0qJDtUUWjqks5uLbkCgaJpZM4VaLg8> .

Closes polybar#1331 Closes polybar#1342 Closes polybar#2673

Closes polybar#1331 Closes polybar#1342 Fixes polybar#2673

Closes #1331 Closes #1342 Fixes #2673

memeplex changed the title ~~Make text module a label [wip]~~ Make text module a label Jul 23, 2018

patrick96 requested changes Jul 28, 2018

View reviewed changes

Make text module a label

6bc932f

Fixes #1331

patrick96 added this to the 4.0.0 milestone Jul 29, 2018

patrick96 added the configuration label Jul 29, 2018

patrick96 mentioned this pull request Apr 4, 2022

[Bug]: Angle brackets escaped in text module #2673

Closed

5 tasks

patrick96 added a commit to patrick96/polybar that referenced this pull request Apr 4, 2022

feat(text): Add standard format with label

d80aa0e

Closes polybar#1331 Closes polybar#1342 Closes polybar#2673

patrick96 added a commit to patrick96/polybar that referenced this pull request Apr 4, 2022

feat(text): Add standard format with label

9581c02

Closes polybar#1331 Closes polybar#1342 Fixes polybar#2673

patrick96 mentioned this pull request Apr 4, 2022

feat(text): Add standard format with label #2676

Merged

9 tasks

patrick96 added a commit to patrick96/polybar that referenced this pull request Apr 4, 2022

feat(text): Add standard format with label

28fbc0f

Closes polybar#1331 Closes polybar#1342 Fixes polybar#2673

patrick96 closed this in #2676 Apr 4, 2022

patrick96 added a commit that referenced this pull request Apr 4, 2022

feat(text): Add standard format with label

973b1fa

Closes #1331 Closes #1342 Fixes #2673

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Make text module a label #1342

Make text module a label #1342

Uh oh!

memeplex commented Jul 22, 2018 •

edited by patrick96

Loading

Uh oh!

codecov-io commented Jul 22, 2018 •

edited

Loading

Uh oh!

memeplex commented Jul 22, 2018 •

edited

Loading

Uh oh!

memeplex commented Jul 23, 2018

Uh oh!

patrick96 left a comment

Uh oh!

patrick96 Jul 28, 2018

Uh oh!

patrick96 Jul 28, 2018

Uh oh!

memeplex Jul 28, 2018

Uh oh!

patrick96 Jul 28, 2018

Uh oh!

memeplex Jul 28, 2018

Uh oh!

patrick96 commented Jul 29, 2018

Uh oh!

memeplex commented Jul 29, 2018 via email

Uh oh!

Uh oh!

Uh oh!

Make text module a label #1342

Make text module a label #1342

Uh oh!

Conversation

memeplex commented Jul 22, 2018 • edited by patrick96 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

memeplex commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

memeplex commented Jul 23, 2018

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

patrick96 Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

patrick96 Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

memeplex Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

patrick96 Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

memeplex Jul 28, 2018

Choose a reason for hiding this comment

Uh oh!

patrick96 commented Jul 29, 2018

Uh oh!

memeplex commented Jul 29, 2018 via email

Uh oh!

Uh oh!

memeplex commented Jul 22, 2018 •

edited by patrick96

Loading

codecov-io commented Jul 22, 2018 •

edited

Loading

memeplex commented Jul 22, 2018 •

edited

Loading