-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add hidpi support to swaybar #1722
Conversation
swaybar/bar.c
Outdated
} | ||
|
||
assert(pointer->cursor_theme = wl_cursor_theme_load( | ||
NULL, 16 * (max_scale * 2), bar->shm)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x cursor themes are weird, I dunno
swaybar/bar.c
Outdated
} | ||
} | ||
|
||
assert(pointer->cursor_theme = wl_cursor_theme_load( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't put a side effect in an assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do this all the time, it's all over sway et al. This is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind duh I see why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed fix to this file but will file a follow-up ticket to go fix my fuck-ups elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this code works but is hard to read. Converting between surface-local and buffer-local coordinates at the beginning and the end of functions doesn't help. I'd prefer everything to take coordinates in surface-local coordinates (especially hotspots), and mark variables using buffer-local coordinates with a buffer_
prefix.
Part of the problem is that we need to communciate the scale to pango so it can up the font size/etc accordingly. I think that the approach used here is reasonably good considering this constraint. |
swaybar/render.c
Outdated
uint32_t ideal_height = text_height + ws_vertical_padding * 2 | ||
+ border_width * 2; | ||
if (height < ideal_height) { | ||
return ideal_height; | ||
if (height < ideal_height / output->scale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example of why this PR is unreadable: here you compare height
(in buffer coordinates) with ideal_height / output->scale
(in surface coordinates). That doesn't make sense.
swaybar/bar.c
Outdated
} | ||
|
||
pointer->cursor_theme = wl_cursor_theme_load( | ||
NULL, 16 * (max_scale * 2), bar->shm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor size changes when hovering the bar. Should be 24 * max_scale
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the output scale is dynamically changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the output scale is dynamically changed we're SOL, I want to file a ticket and deal with it never
swaybar/render.c
Outdated
bool focused, double *x, uint32_t width, uint32_t height) { | ||
struct swaybar_output *output, struct swaybar_config *config, | ||
const char *text, bool focused, double *x, | ||
uint32_t width, uint32_t surface_height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surface_width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unused actually, does it make sense to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup
swaybar/render.c
Outdated
uint32_t _ideal_height = _height + ws_vertical_padding * 2; | ||
if (height < _ideal_height) { | ||
return _height; | ||
if (height < _ideal_height / output->scale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be _height
?
swaybar/render.c
Outdated
@@ -242,33 +272,41 @@ static uint32_t render_status_line_i3bar(cairo_t *cairo, | |||
static uint32_t render_status_line(cairo_t *cairo, | |||
struct swaybar_config *config, struct swaybar_output *output, | |||
struct status_line *status, bool focused, | |||
double *x, uint32_t width, uint32_t height) { | |||
double *x, uint32_t width, uint32_t surface_height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto: surface_width
?
swaybar/render.c
Outdated
@@ -330,14 +368,22 @@ static uint32_t render_workspace_button(cairo_t *cairo, | |||
box_colors = config->colors.inactive_workspace; | |||
} | |||
|
|||
uint32_t height = surface_height *output->scale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my, missing a space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my
wl_surface_attach(output->surface, | ||
output->current_buffer->buffer, 0, 0); | ||
wl_surface_damage(output->surface, 0, 0, output->width, output->height); | ||
wl_surface_damage(output->surface, 0, 0, | ||
output->width, output->height); | ||
wl_surface_commit(output->surface); | ||
wl_display_roundtrip(bar->display); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get why this roundtrip is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't dispatch in a loop like some of our other clients do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, makes sense
Fixes #1137
Ref #797