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

frebib · 2021-02-28T12:32:20Z

This is an almost direct rebase of #1671

What type of PR is this? (check all applicable)

Disclaimer: I merely rebased this from the original PR. I changed as little as possible to make it work. I expect there will be plenty of style and similar comments in the review, which I intend to fix before this is merged. Created as a draft to collect feedback before final round of reviewing

Checklist:

Compile & Run
clang-format
Unit tests for unit.hpp
Decide if negative spacing is allowed
Go through all config options that use extents or percentage_with_offset and see how they deal with negative values (and if they're even allowed).
Address the weirdness in base.inl: Add units support for polybar #1651 #951 #1671 (comment)
Community testing
Probably some other things

patrick96 · 2021-03-02T11:56:54Z

Awesome! Thanks for putting in all this effort 🥳

The unit tests don't compile yet, probably because the dispatch test still uses the old offsets.

I have some time to review this, this afternoon. I'm very excited about this, this will likely be the biggest change in the next version (unless we can get #1752 in, but I think that would take too long).

frebib · 2021-03-02T22:32:45Z

Thanks for fixing up the tests. I had a go at fixing them but ran into weirdness I didn't understand.

If there's anything else you'd like me to change please holla

patrick96 · 2021-03-02T23:20:39Z

Yeah, the tests took some time because the equality operator for extent_val needed to be defined in a very particular way.

I'm writing up the review right now, there are only a few things left, I think.

patrick96

I went through all my reviews in the old PR and tried to address them here. I also ran clang-format over all changed files.

There are still some things that need to be done:

We need to decide whether we want to allow negative spacing
We (I) need to address the weirdness in base.inl: #1671 (comment)
We need to go through all config options that use extents or percentage_with_offset and see how they deal with negative values (and if they're even allowed)

include/utils/unit.hpp

src/components/bar.cpp

src/components/config.cpp

src/drawtypes/label.cpp

- 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)

The old names didn't really capture the purpose of the structs and function. space_type -> spacing_type space_size -> spacing_val size_type -> extent_type geometry -> extent_val geometry_format_values -> percentage_with_offset

No longer needed. The convert<spacing_val> function in config.cpp already does all the work for us and always setting the type to pixel was wrong. In addition, line-size should not be of type spacing_val but extent_val.

I tried to address most of my comments on the old PR

We can't just blindly add the x difference to the width because for example the width should increase if x < width and the increase keeps x < width. Similarly, we can't just add the offset to the width.

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

codecov · 2021-09-02T16:45:35Z

Codecov Report

Merging #2394 (08ff6d5) into master (5f34622) will increase coverage by 0.10%.
The diff coverage is 12.90%.

@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
+ Coverage   10.13%   10.24%   +0.10%     
==========================================
  Files         147      148       +1     
  Lines       10181    10290     +109     
==========================================
+ Hits         1032     1054      +22     
- Misses       9149     9236      +87

Flag	Coverage Δ
unittests	`10.24% <12.90%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
include/components/renderer_interface.hpp	`100.00% <ø> (ø)`
include/components/types.hpp	`3.03% <ø> (ø)`
include/modules/fs.hpp	`0.00% <ø> (ø)`
include/modules/meta/base.hpp	`0.00% <ø> (ø)`
include/modules/meta/base.inl	`0.00% <0.00%> (ø)`
include/tags/parser.hpp	`94.73% <ø> (ø)`
include/tags/types.hpp	`100.00% <ø> (ø)`
src/components/bar.cpp	`0.00% <0.00%> (ø)`
src/components/builder.cpp	`0.00% <0.00%> (ø)`
src/components/config.cpp	`0.00% <0.00%> (ø)`
... and 16 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 5f34622...08ff6d5. Read the comment docs.

mainrs · 2022-01-21T01:22:44Z

Is it possible for non-write access users to even see a merge conflict? I wanted to help out but there doesn’t seem to be a way to see the conflicts.

patrick96 · 2022-01-21T11:53:22Z

@mainrs If you don't see them, I guess non-members can't see them in the UI. If you really want to see the conflicts, you can clone frebib's repo locally and try to merge into upstream master.

However, the merge conflicts are not a blocker here. There are some decisions I have to make with regards to semantics, as I mentioned in my review: #2394 (review)
This is in the 3.6.0 milestone and I definitely plan to have this in the next release. I will try to work on this after I have finished #2539.

I appreciate your initiative though :)

There are a bunch of issues tagged with "help wanted" that are worth looking at.
From a quick glance, some that might be interesting and easier for first time contributors are:

The last one will probably touch quite a few components and may require a lot more time than the other three.

frebib mentioned this pull request Feb 28, 2021

Add units support for polybar #1651 #951 #1671

Closed

patrick96 added this to the 3.6.0 milestone Mar 2, 2021

patrick96 force-pushed the feat/units branch 4 times, most recently from f7c8c8f to 9883b90 Compare March 2, 2021 22:04

patrick96 requested changes Mar 3, 2021

View reviewed changes

patrick96 force-pushed the feat/units branch from 6c6f1b3 to e2a42da Compare March 3, 2021 00:24

frebib force-pushed the feat/units branch from 4bf4ab3 to 7db9729 Compare April 11, 2021 10:51

patrick96 force-pushed the feat/units branch from 9fac77d to 325bef8 Compare June 29, 2021 23:02

Lomadriel and others added 17 commits August 1, 2021 18:13

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

4577858

- 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 test compilation

d18af49

clang-format

88692f8

Better names

693d993

The old names didn't really capture the purpose of the structs and function. space_type -> spacing_type space_size -> spacing_val size_type -> extent_type geometry -> extent_val geometry_format_values -> percentage_with_offset

Remove parse_size_with_unit

fdc9c48

No longer needed. The convert<spacing_val> function in config.cpp already does all the work for us and always setting the type to pixel was wrong. In addition, line-size should not be of type spacing_val but extent_val.

Cleanup

efdc938

I tried to address most of my comments on the old PR

Fix renderer width calculation

42664d3

We can't just blindly add the x difference to the width because for example the width should increase if x < width and the increase keeps x < width. Similarly, we can't just add the offset to the width.

Rename geom_format_to_pixels to percentage_with_offset_to_pixel

9616bf9

Cleanup

d5de6c4

Apply suggested changes from Patrick on GitHub

7668780

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Update src/components/bar.cpp

7bdcf7f

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Update src/components/config.cpp

ca90065

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Update src/components/builder.cpp

06860ca

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Update src/components/builder.cpp

e8d4084

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

config: Use stod for parsing percentage

077c040

Use stof instead of strtof

b25d6a8

units: Fix test edge cases

c93a4b1

Remove unnecessary clang-format toggle

0e0081e

frebib force-pushed the feat/units branch from 325bef8 to 0e0081e Compare August 1, 2021 18:21

patrick96 marked this pull request as ready for review August 31, 2021 12:22

polybar deleted a comment from codecov bot Sep 2, 2021

patrick96 self-requested a review September 2, 2021 16:37

Use percentage_with_offset for margin-{top,bottom}

7849a4e

Support negative extent values

08ff6d5

patrick96 self-assigned this Jan 21, 2022

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

frebib deleted the feat/units branch February 20, 2022 20:24

Uh oh!

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

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

Uh oh!

Conversation

frebib commented Feb 28, 2021 • edited by patrick96 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this? (check all applicable)

Checklist:

Uh oh!

patrick96 commented Mar 2, 2021

Uh oh!

frebib commented Mar 2, 2021

Uh oh!

patrick96 commented Mar 2, 2021

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!

codecov bot commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mainrs commented Jan 21, 2022

Uh oh!

patrick96 commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

frebib commented Feb 28, 2021 •

edited by patrick96

Loading

codecov bot commented Sep 2, 2021 •

edited

Loading