Add units support for polybar #1651 #951 #1671

Lomadriel · 2019-03-04T05:39:05Z

Implement feature requested in #1651 and #951.
This pr add supports for px (pixel) and pt (point) for polybar.

I followed the advices given by @patrick96 in #1651, so I add a size_with_unit structure to store the value and the unit. Unlike the size definition, the value is signed to handle negative offset.

To handle the width and height problem, I added a geometry_format_values structure which contains a percentage and the offset (a size_with_unit structure).

builder::space now uses the %{O} formatting tag but this is handled by size_with_unit_to_string helper in the new header "utils/unit.hpp" to avoid duplicate code (this function is also used in controller.cpp since I can't use builder::space here).

DPI initialization is now in the constructor of the bar. However this initialization seems strange, the DPI are only computed if dpi < 0 is explicitly specified in the configuration (see below). If the DPI isn't specified in the configuration, the default value is 96. Is it the correct behavior?

https://github.com/Lomadriel/polybar/blob/cc4e09edc41cfbd5f2f4bf7235470aa1c21bc59a/src/components/bar.cpp#L155-L178

The rest of the PR is just adapting the code to use these structures. I tried to find all keys that could uses units but I am not sure if I added units support for all the key that could benefit of this feature.

EDIT: Fixes #1700

TEAMEDIT:
Closes #1651
Closes #951
Fixes #1265

Lomadriel · 2019-03-04T06:28:09Z

I fixed the unit tests. Should I add a test for the function template <> geometry_format_values config::convert(string&& value) const? (which does the string -> geometry_format_values conversion that isn't checked anymore)

Lomadriel · 2019-03-04T23:14:16Z

I just remove some bugs (negative margin, padding...).
Normally this should also fix #1265.

patrick96

I jotted down a bunch of thoughts I had about this. (almost) nothing here is set in stone, I'd appreciate your thoughts as well.

Thanks a lot for putting your time into this, I'm really excited to get this functionality inside polybar.

DPI initialization is now in the constructor of the bar. However this initialization seems strange, the DPI are only computed if dpi < 0 is explicitly specified in the configuration (see below). If the DPI isn't specified in the configuration, the default value is 96. Is it the correct behavior?

Moving it into bar.cpp is fine. I don't know what the original intention was. But since that part of the code is over 2 years old, I'd say it's the expected behavior now ;)

One thing that concerned me when writing up the instructions in #1651 was that SPACE doesn't make sense in the offset format.

I think we need to separate how we specify sizes and spacing.

Spacing can be in spaces, pixel, or points while size can be percentages, pixel, points or those combined in the offset format.

I first thought we can keep this together because it has lots of things in common (POINT and PIXEL), but I don't think this is worth it.

The more I think about it, the less I am a fan of converting points to pixels only when the value in pixels is needed and passing around point values. I know this is actually what I suggested we do originally because the builder would be the only place where this conversion happens. But this would make things quite inflexible because if we ever extend our code we would need to do the conversion in all places that use those pixel values.

Ideally we would get back and pass around only structs like this for spacing:

enum size_type { SPACE, PIXEL };

struct size {
  enum size_type type;
  ssize_t value;
}

and everything else is handled by the config. I said before that I am not a fan of the config knowing about the dpi but it seems to me this is the only way to get a clean interface for this.

I am planning on separating the config into two parts anyway: One part just accesses the config data structures and returns a string for a given key, the other part takes that string and converts it into whatever type is needed. The current way of using templates for everything in the config seems a bit messy to me. Since we will have such a separation anyway and there the part that converts things may actually know about dpi and other things from the outside, we could store dpi_x in the config as a temporary fix (and mark it as such). We only store dpi_x because spacing will always be in the x-direction.

We don't have the same problem with sizes and the geometry format because there we need to call a special function to get pixel values anyway and so we can just pass the dpi there.

The last thing is about what type the values should have. You made it very generic by using templates.

~~As far as I can tell, the following are the strictest types we can have for the different units:~~

~~spacing:~~
- ~~POINT: float >= 0~~
- ~~PIXEL: size_t~~
- ~~SPACE: size_t~~
~~size:~~
- ~~POINT: float~~
- ~~PIXEL: ssize_t~~

~~If we actually want to use these types, we'd have to use union inside the structs. This may be too cumbersome though, because we would have to have multiple if-else blocks whenever we use it.~~

~~For spacing, if we implement it as I described just above, the config will anyway just return either PIXEL or SPACE, so we can use size_t there.~~

~~For size, we could also use float for everything and when converting to pixels, we just cast to size_t.~~

EDIT: Since we use the %{O tag, the POINT and PIXEL values for spacing can also be negative. So using float everywhere is fine.

include/utils/unit.hpp

Lomadriel · 2019-03-18T22:21:15Z

DPI initialization is now in the constructor of the bar. However this initialization seems strange, the DPI are only computed if dpi < 0 is explicitly specified in the configuration (see below). If the DPI isn't specified in the configuration, the default value is 96. Is it the correct behavior?

Moving it into bar.cpp is fine. I don't know what the original intention was. But since that part of the code is over 2 years old, I'd say it's the expected behavior now ;)

If that's the expected behavior, maybe the documentation/wiki should be modified to state that any value <= 0 cause the DPI to be auto-detected. Moreover, this detection is bugged, since if I change the resolution of my screen (like when I duplicate my laptop screen and the projector is in 4/3), I have to relaunch polybar to update this value. (Maybe an x event should cause the value to be updated)

I think we need to separate how we specify sizes and spacing.

Spacing can be in spaces, pixel, or points while size can be percentages, pixel, points or those combined in the offset format.

I first thought we can keep this together because it has lots of things in common (POINT and PIXEL), but I don't think this is worth it.

I agree, I tried to do my best but this is really confusing.

The more I think about it, the less I am a fan of converting points to pixels only when the value in pixels is needed and passing around point values. I know this is actually what I suggested we do originally because the builder would be the only place where this conversion happens. But this would make things quite inflexible because if we ever extend our code we would need to do the conversion in all places that use those pixel values.

The following paragraph supposes that the DPI might become dynamic otherwise its content should be ignored.

For that point, I think this might be a problem if the DPI changes during the execution.
Since the "point" is here to have a DPI sensitive unit, it is quite strange to convert point sizes into pixel sizes when charging the configuration. If the DPI changes, we would need to update every sizes and it is quite expensive.
Maybe introducing a sort of settings singleton that will hold some important values (like the DPI) could solve the problem even if I don't like introducing singleton.
It will allow to have a space_size struct like that:

struct space_size {
  space_type type;
  // If type == space_type::pixel converts the value in pixel if necessary and returns it.
  // Otherwise returns the size in number of spaces.
  // No need to use float in the return type since pixel or space doesn't need them.
  ssize_t get_value() const {
    if (type == space_type::point)     
      return convert_to_pixel(settings::get_instance().get_dpi_x(), value);
    else
      return value;
  }
private:
  // to be sure that nobody uses it directly
  float value; // could be an union if you want since the get_value is the only place
                    // were the union will be used.
};

The geometry_size struct could be defined in a similar way with an enum type like that:

enum class geometry_size_type { PERCENTAGE, POINT, PIXEL }

enum size_type { SPACE, PIXEL };

Do you want me to use a plain C enum or may I use an enum class? (I prefer using enum class to avoid global namespace pollution)

What do you think of this?

patrick96 · 2019-03-18T22:46:20Z

Everything within the bar except for the modules themselves is quite static. Polybar cannot dynamically change its size or position when changes to the monitor occur. All of this is handled through screenchange-reload which just reloads polybar whenever the screen changes.

It is conceivable that we want to make things more dynamic in the future. But even then, I would not convert to pixels in the builder but rather extend the %{O} tag to also work with points and then let the renderer handle it. Either way seems fine to me, thinking about it, doing this in the renderer may even be a little cleaner. This is up to you how you want to do it, I just don't want it to happen somewhere in the middle where we potentially have to do the conversion in multiple places.

Adding get_value might look very clean in the code that uses it, but it introduces a singleton and couples the concept of units for spacing purposes to the bar class. I want to avoid that.

The geometry_size struct could be defined in a similar way with an enum type like that:
enum class geometry_size_type { PERCENTAGE, POINT, PIXEL }

Wouldn't geometry_size_type only have point and pixel? As far as I know, percentages cannot be used as offsets.

Do you want me to use a plain C enum or may I use an enum class? (I prefer using enum class to avoid global namespace pollution)

You can use enum class. I just used enum because I'm more familiar with C ;)

Lomadriel · 2019-03-18T23:08:24Z

Everything within the bar except for the modules themselves is quite static. Polybar cannot dynamically change its size or position when changes to the monitor occur. All of this is handled through screenchange-reload which just reloads polybar whenever the screen changes.

I didn't know it worked that way, but that's seems logical with the fact that some of my modules are reset when I change my resolution.

It is conceivable that we want to make things more dynamic in the future. But even then, I would not convert to pixels in the builder but rather extend the %{O} tag to also work with points and then let the renderer handle it. Either way seems fine to me, thinking about it, doing this in the renderer may even be a little cleaner. This is up to you how you want to do it, I just don't want it to happen somewhere in the middle where we potentially have to do the conversion in multiple places.

Maybe I can tried to do that to see if it is more clean. (Extends the %{O} tag)

The geometry_size struct could be defined in a similar way with an enum type like that:
enum class geometry_size_type { PERCENTAGE, POINT, PIXEL }
Wouldn't geometry_size_type only have point and pixel? As far as I know, percentages cannot be used as offsets.

Yeah my bad, it is just point and pixel.

Lomadriel · 2019-03-18T23:18:18Z

By the way, do you have an idea for naming the enums and structs associated with that:

spacing:

POINT: float >= 0

PIXEL: ssize_t

SPACE: ssize_t

size:

POINT: float

PIXEL: size_t

Each time I found a name it is either bad or already taken (like size)

patrick96 · 2019-03-20T09:49:17Z

Since we probably put these inside a namespace, naming is not such a big issue, but I think the following names are reasonable. Feel free to improve them.

enum class size_type { POINT, PIXEL }
enum class space_type { POINT, PIXEL, SPACE }

struct spacing {
  space_type type;
  ...
}

struct geometry {
  float percentage;
  size_type offset_type;
  ...
}

Lomadriel · 2019-03-25T05:03:07Z

I just finished some modifications to take your review in account. Just as expected, there are two structures, one for spacing and the other for size.
The %{O} tag now supports units (%{O100px} and %{O100pt}).
There is also a commit that fixes the problem mentioned in #1700.

patrick96

Finally got around to reviewing this. Thanks for your patience ;)

Did some rough testing and things seem to work fine with actually emitting and parsing the offsets. Will do some more thorough testing once the other issues are resolved.

You also need to resolve the merge conflict that was likely caused by us removing icon_t in #1553.

The issue in #1700 is still not totally fixed. Offsets do produce spaces that have the proper background color, but the string I used in #1700 still does not look the same as with lemonbar:

%{O100}Filler%{B#ff0000 O100}First%{O200}Second%{O-100}

With this PR:

With lemonbar:

You see, Second is not displayed with this PR:

include/components/types.hpp

include/modules/fs.hpp

src/components/controller.cpp

src/components/parser.cpp

src/components/renderer.cpp

src/drawtypes/label.cpp

patrick96 · 2019-04-17T18:58:06Z

It would probably be useful if we also allow the use of these units in format-padding, format-offset etc. Those are set in modules/meta/base.cpp

Lomadriel · 2019-04-17T22:56:55Z

The issue in #1700 is still not totally fixed. Offsets do produce spaces that have the proper background color, but the string I used in #1700 still does not look the same as with lemonbar:
%{O100}Filler%{B#ff0000 O100}First%{O200}Second%{O-100}
With this PR:

With lemonbar:

You see, Second is not displayed with this PR:

Fixed by fd7322b

Lomadriel · 2019-04-17T23:59:30Z

It would probably be useful if we also allow the use of these units in format-padding, format-offset etc. Those are set in modules/meta/base.cpp

If I am not wrong format-padding already support units.
ab19b3b adds unit support for format-offset

Lomadriel · 2019-06-17T04:38:44Z

I just added a commit that fixes #1814.
I can't make a separate PR since I modified the renderer :/
Sorry

patrick96 · 2019-06-17T07:16:29Z

I think you should make a separate PR and resolve the merge conflict once one of the two is merged.

Lomadriel · 2019-06-18T18:44:12Z

I think you should make a separate PR and resolve the merge conflict once one of the two is merged.

Commit removed. See my new PR #1817.

- add a size_with_unit struct - add a geometry_format_values struct - move dpi initialisation from renderer.cpp to bar.cpp - add a string to size_with_unit converter - add point support (with pt) - add pixel support (with px)

patrick96

It seems it's taking me longer and longer to find time for these reviews. Hopefully the next time you won't have to wait so long.

I'm very happy with this 😃. I don't see any immediate defects and it does make the whole spacing and size handling more uniform. I mainly commented on small stuff and the naming of these new concepts.

The only thing we need to think about whether or not to allow negative values for spacing. Theoretically we can allow negative values for px and pt spacing but not for actual spaces. In this PR, as far as I can see, all negative values are disallowed (in order to fix #1265).
I think we should allow negative values for points and pixels and only throw an error for negative spaces.

After this we probably just need one last review and some more testing and then we can merge this.

Please also run clang-format on all the files you touched, I think you missed some:

clang-format -i $(git log --name-only --pretty=oneline --full-index 1caed747fd2991d67fee122e3fff1ba01b9dcdaf^..41381ee7038a18678ad88117971f53bbc6a3be47 | \grep -vE '^[0-9a-f]{40} ' | sort | uniq)

include/components/types.hpp

src/components/renderer.cpp

include/components/types.hpp

include/drawtypes/label.hpp

src/components/builder.cpp

src/components/config.cpp

include/modules/meta/base.inl

src/modules/meta/base.cpp

src/components/builder.cpp

frebib · 2020-02-25T12:34:40Z

Can this be rebased against master please

frebib · 2020-04-07T20:32:54Z

I squashed and rebased this against master (I tried to rebase without squashing but kept screwing up the history, so figured I didn't need to keep it anyway just to build and run it)
frebib@7385b89

I rely on this fork to have polybar even remotely usable when changing DPI. Please consider reviving it @Lomadriel

fabianschuiki · 2020-11-05T12:14:14Z

Any way to help get this PR merged? E.g. implement requested code changes? I'd really love to see these units to improve HiDPI support 👍

frebib · 2021-01-24T18:40:12Z

Until now I've been rebasing this PR onto master, but fd55652 was a big change that will not be a trivial change to rebase onto. I may or may not attempt to fix this up again, depending on how hard it seems to be

patrick96 · 2021-01-26T10:58:26Z

@frebib If you do attempt to resolve the conflicts and want to help get this merged, a PR would be very much appreciated :)

And of course, ask if you need help.

I think most conflicts should be quite straighforward (just tedious to get the changes from both sides in), but in the renderer and parser, it might be more difficult because there we actually changed the places where the offset handling happens.

frebib · 2021-02-22T21:27:58Z

@patrick96 I can certainly give it a go, but I'm not at all familiar with C++ or much of the syntax/language features.
If I were to rebase this, it'd be squashed into a single commit- hopefully that's ok.
There were a few unaddressed comments from what I remember about the implementation which I should probably address at the same time.
In use I also noticed that there are still some glaring issues with unit support particularly in the tray area. I managed to hack in support for most of it, but the padding between the icons is still not correctly scaled. Hopefully that shouldn't be too tricky to solve.

patrick96 · 2021-02-22T23:53:30Z

If I were to rebase this, it'd be squashed into a single commit- hopefully that's ok.

Yeah, that's fine. This would have happened with this PR anyway, I think.

I can certainly give it a go, but I'm not at all familiar with C++ or much of the syntax/language features.

In that case, this might get tricky. Let me know, if you need any help.

In use I also noticed that there are still some glaring issues with unit support particularly in the tray area. I managed to hack in support for most of it, but the padding between the icons is still not correctly scaled. Hopefully that shouldn't be too tricky to solve.

Yeah, I don't think this PR touches the tray at all. Feel free to leave out units in the tray if that's easier. First priority should be to get the overall framework working.

frebib · 2021-02-28T12:33:07Z

I gave it a go. Seems to compile & run the same as the old builds did for me.
See #2394

Lomadriel force-pushed the feat/units branch from 8a3fa24 to a1a0a52 Compare March 4, 2019 23:23

patrick96 requested changes Mar 18, 2019

View reviewed changes

include/utils/unit.hpp Outdated Show resolved Hide resolved

Lomadriel mentioned this pull request Mar 19, 2019

Offset tag doesn't work as indicated in the documentation #1700

Closed

Lomadriel force-pushed the feat/units branch from a1a0a52 to c09fe1c Compare March 25, 2019 03:43

patrick96 requested changes Apr 17, 2019

View reviewed changes

Lomadriel force-pushed the feat/units branch 2 times, most recently from b2ec470 to fd7322b Compare April 17, 2019 23:43

Lomadriel mentioned this pull request Apr 18, 2019

Allow to override module-margin-xxx #1320

Open

Lomadriel requested a review from patrick96 May 17, 2019 04:25

Lomadriel mentioned this pull request Jun 17, 2019

Modules with click actions become non-clickable when offset is used #1814

Closed

Lomadriel force-pushed the feat/units branch from 256fea7 to f2c170c Compare June 18, 2019 18:41

Lomadriel mentioned this pull request Jun 18, 2019

fix(renderer): fix click area with negative offset #1817

Closed

Lomadriel added the feature label Jul 28, 2019

Lomadriel added 3 commits August 7, 2019 00:01

add units support (POINT, PIXEL, SPACE) for polybar

1caed74

- add a size_with_unit struct - add a geometry_format_values struct - move dpi initialisation from renderer.cpp to bar.cpp - add a string to size_with_unit converter - add point support (with pt) - add pixel support (with px)

fix unit tests.

3271a03

fix compilation

c2db15d

Lomadriel added 10 commits August 7, 2019 00:01

remove introduced bug

9489d6d

fixing unsigned/signed bugs

7b2f1af

separate space size and geometry size

5781ef6

fix(renderer): draw offset background

9772b6f

feat(renderer): add unit support for %{O} tag

b6d6f92

use variable rather than a reference to it

a9b2d87

add constants

2f93c9a

use geometry for offset event and fix block background rendering

4200c1e

add unit support for format-offset

37427c6

remove duplicate code in builder and controller

41381ee

Lomadriel force-pushed the feat/units branch from f2c170c to 41381ee Compare August 6, 2019 22:01

polybar deleted a comment from codecov-io Jan 21, 2020

patrick96 requested changes Jan 21, 2020

View reviewed changes

patrick96 mentioned this pull request Apr 13, 2020

Allow fractional padding/margins #2056

Closed

patrick96 mentioned this pull request Oct 18, 2020

Use an option to customize how space is inserted in the bars #2214

Closed

patrick96 added this to the 3.6.0 milestone Nov 27, 2020

frebib mentioned this pull request Feb 28, 2021

add units support (POINT, PIXEL, SPACE) for polybar #2394

Closed

14 tasks

patrick96 mentioned this pull request Jan 23, 2022

Add units support (POINT, PIXEL, SPACE) #2578

Merged

27 tasks

patrick96 closed this in #2578 Feb 20, 2022

Uh oh!

Add units support for polybar #1651 #951 #1671

Add units support for polybar #1651 #951 #1671

Uh oh!

Conversation

Lomadriel commented Mar 4, 2019 • edited by patrick96 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lomadriel commented Mar 4, 2019

Uh oh!

Lomadriel commented Mar 4, 2019

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!

Lomadriel commented Mar 18, 2019

Uh oh!

patrick96 commented Mar 18, 2019

Uh oh!

Lomadriel commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lomadriel commented Mar 18, 2019

Uh oh!

patrick96 commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lomadriel commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrick96 commented Apr 17, 2019

Uh oh!

Lomadriel commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lomadriel commented Apr 17, 2019

Uh oh!

Lomadriel commented Jun 17, 2019

Uh oh!

patrick96 commented Jun 17, 2019

Uh oh!

Lomadriel commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frebib commented Feb 25, 2020

Uh oh!

frebib commented Apr 7, 2020

Uh oh!

fabianschuiki commented Nov 5, 2020

Uh oh!

frebib commented Jan 24, 2021

Uh oh!

patrick96 commented Jan 26, 2021

Uh oh!

frebib commented Feb 22, 2021

Lomadriel commented Mar 4, 2019 •

edited by patrick96

Loading

patrick96 left a comment •

edited

Loading

Lomadriel commented Mar 18, 2019 •

edited

Loading

patrick96 commented Mar 20, 2019 •

edited

Loading

Lomadriel commented Mar 25, 2019 •

edited

Loading

Lomadriel commented Apr 17, 2019 •

edited

Loading

Lomadriel commented Jun 18, 2019 •

edited

Loading