Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Rounded bar border #2359

Merged
merged 8 commits into from Feb 16, 2021
Merged

Conversation

h45h74x
Copy link
Contributor

@h45h74x h45h74x commented Jan 19, 2021

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

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Other: Replace this with a description of the type of this PR

Description

This PR exists to change the behaviour when applying a radius together with border-size with a minimal amount of code changes. Polybar is now able to display a round border (also supporting the functionality added in #2297) 馃帀

In order to avoid bigger changes, I've implemented it so that the rounded border gets its size and color from the top border configuration (border-size / border-top-size and border-color / border-top-color respectively).
While I am not sure how to implement the configuration in a more user-friendly way without changing the current configuration options, I would be glad to add any changes you suggest.

Related Issues & Documents

While the idea to implement this came up whilst styling my system, this issue is a good representation: #1566

Several quick screenshots as visual demonstration (with and without compositor) can be found here

Documentation (check all applicable)

  • This PR requires changes to the Wiki documentation (describe the changes)
  • This PR requires changes to the documentation inside the git repo (please add them to the PR).
  • Does not require documentation changes

It would be necessary to remove the incompatibility notice regarding the radius and border-size configuration options.

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #2359 (26e6b81) into master (4154943) will increase coverage by 0.09%.
The diff coverage is 17.56%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2359      +/-   ##
=========================================
+ Coverage    7.96%   8.05%   +0.09%     
=========================================
  Files         143     143              
  Lines       10369   10436      +67     
=========================================
+ Hits          826     841      +15     
- Misses       9543    9595      +52     
Flag Coverage 螖
unittests 8.05% <17.56%> (+0.09%) 猬嗭笍

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

Impacted Files Coverage 螖
include/cairo/context.hpp 0.00% <0.00%> (酶)
include/components/config_parser.hpp 66.66% <酶> (酶)
include/modules/xworkspaces.hpp 0.00% <酶> (酶)
src/components/renderer.cpp 0.00% <0.00%> (酶)
src/modules/xworkspaces.cpp 0.00% <0.00%> (酶)
src/components/config_parser.cpp 49.32% <100.00%> (+4.87%) 猬嗭笍
include/components/logger.hpp 28.57% <0.00%> (+14.28%) 猬嗭笍

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 4154943...90ecf29. Read the comment docs.

@patrick96
Copy link
Member

This looks really cool, especially the one where only two opposing corners are rounded 馃ぉ

I think taking the top border value is fine. I'm not sure it makes sense to have different border sizes when using rounded corners. A corner is part of two borders, so the two borders should be identical otherwise we can't really draw a rounded corner for it.

We could make it slightly more configurable by using the bottom border for the bottom corners and the top border for the top ones. We then either tell the user in the documentation that the two adjacent borders for a rounded corner should be equal or we even enforce this when loading the config.

Thanks for implementing this. I didn't think it would look this good 馃帀

I'll try to review this tomorrow. But I have exams coming up, so no promises.

@patrick96 patrick96 self-requested a review January 19, 2021 18:18
@h45h74x
Copy link
Contributor Author

h45h74x commented Jan 19, 2021

Thanks for the positive feedback!
(I almost thought this PR would be rejected because of the wontfix tag in the issue...)

We could make it slightly more configurable by using the bottom border for the bottom corners and the top border for the top ones.

I think that's a great idea, but is cairo able to merge the upper and lower section (thinking of color and line width) together seamlessly?

Also, take your time with the review. I am not in a hurry with implementing this, I already have it on my machine :D

@patrick96
Copy link
Member

(I almost thought this PR would be rejected because of the wontfix tag in the issue...)

At the time I didn't know it would be this easy ;)

(And I didn't want to invest the time to figure it out)

I think that's a great idea, but is cairo able to merge the upper and lower section (thinking of color and line width) together seamlessly?

I doesn't have to be able to do that. When there is a requirement (or expectation) that the border settings for the two adjacent borders to a rounded corner have to be the same, then we never have to blend stuff together. For example, if someone has the top left and bottom left corner rounded, by this requirement the top border must equal the left border and the left border must equal the bottom border, so no blending required on the left border.

@h45h74x
Copy link
Contributor Author

h45h74x commented Jan 20, 2021

I've changed the implementation so that the border is way more customizable now.
(Previously the border was one single subpath in cairo which did not allow for much customisation)

Circle segments with separate, adjustable radii are used for the corners, the offset and length of the regular horizontal and vertical borders is adjusted accordingly. Also, the color of the corners is now always matching the left / right vertical border color.

There are two more screenshots in my nextcloud folder now, depicting the result of the following two configurations:

colors

border-top-color=#0000ff        ; blue
border-bottom-color=#00ffff     ; cyan
border-left-color=#ff0000       ; red
border-right-color=#ffff00      ; yellow

polybar_test_01.png

border-top-size=10
border-bottom-size=10
border-left-size=10
border-right-size=10

radius-top-left=10
radius-top-right=20
radius-bottom-left=30
radius-bottom-right=0

polybar_test_02.png

border-top-size=10
border-bottom-size=20
border-left-size=10
border-right-size=10

radius-top-left=10
radius-top-right=20
radius-bottom-left=0
radius-bottom-right=0

@h45h74x
Copy link
Contributor Author

h45h74x commented Jan 25, 2021

Hi, @patrick96. As I said, I'm not in a rush to merge this PR, but did you already have a look at my changes? :D

@patrick96
Copy link
Member

I did look a bit, just not enough to do a full review. I noticed that you can quite weird looking things (border going to the edge with a rounded edge beneath it), so it might be a good idea to print a warning if the border settings of two borders adjacent to a rounded edge are not the same.

Will do a full review later.

Copy link
Member

@patrick96 patrick96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Everything looks good now 馃槂

Just a few small comments to resolve.

I think having a note on the wiki about using the same border setting for adjacent corners should be enough. I can no longer reproduce the truly weird behavior, probably because I was using an older version back then, so I don't think it is necessary to write a bunch of code to give a warning message.

include/cairo/context.hpp Show resolved Hide resolved
include/cairo/context.hpp Outdated Show resolved Hide resolved
Copy link
Member

@patrick96 patrick96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :)

Thanks a lot, this is really cool 馃槂

@patrick96 patrick96 merged commit cd71b96 into polybar:master Feb 16, 2021
@h45h74x
Copy link
Contributor Author

h45h74x commented Feb 17, 2021

Thank you for letting me contribute :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants