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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hidpi support to swaybar #1722

Merged
merged 4 commits into from
Apr 4, 2018
Merged

Add hidpi support to swaybar #1722

merged 4 commits into from
Apr 4, 2018

Conversation

ddevault
Copy link
Contributor

@ddevault ddevault commented Apr 4, 2018

Fixes #1137

Ref #797

swaybar/bar.c Outdated
}

assert(pointer->cursor_theme = wl_cursor_theme_load(
NULL, 16 * (max_scale * 2), bar->shm));
Copy link
Member

Choose a reason for hiding this comment

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

* 2?

Copy link
Contributor Author

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(
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

@emersion emersion left a 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.

@ddevault
Copy link
Contributor Author

ddevault commented Apr 4, 2018

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) {
Copy link
Member

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);
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha

Copy link
Contributor Author

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) {
Copy link
Member

Choose a reason for hiding this comment

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

surface_width

Copy link
Member

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?

Copy link
Contributor Author

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) {
Copy link
Member

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) {
Copy link
Member

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;
Copy link
Member

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

Copy link
Contributor Author

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);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

@emersion emersion merged commit 3ea7d2d into wlroots Apr 4, 2018
@ddevault ddevault deleted the swaybar-hidpi branch April 4, 2018 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants