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

swaybar: disallow left and right position and print error on default #2879

Merged
merged 2 commits into from
Oct 20, 2018

Conversation

Emantor
Copy link
Contributor

@Emantor Emantor commented Oct 19, 2018

The positions "left" and "right" are not allowed by the man page, remove them
from the allowed positions. Also print an error to stderr if we default to the
bottom position.

Fixes #2878

@Emantor
Copy link
Contributor Author

Emantor commented Oct 19, 2018

@ianyfan We could also exit instead of defaulting to bottom, I decided to keep the existing behaviour here. What is your opinion on that?

Copy link
Contributor

@ianyfan ianyfan left a comment

Choose a reason for hiding this comment

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

The check should really occur in commands/bar/position.c, and default to bottom on error. Then, there should be no chance of swaybar starting with an invalid position. So really, the else block shouldn't be needed, but for sanity, you could keep this. But use wlr_log instead of fprintf.

@Emantor
Copy link
Contributor Author

Emantor commented Oct 19, 2018

I think both should be removed. I'll add the wlr_log for commands/bar/position.c, but I don't think that will work for swaybar, since afaik wlr_log is only available to the compositor. I think printing to stderr is the right call there.

@emersion
Copy link
Member

afaik wlr_log is only available to the compositor

It's also available in clients. But it makes clients link with wlroots, which is not great.

@Emantor
Copy link
Contributor Author

Emantor commented Oct 19, 2018

Swaybar does currently not link against wlroots, so i'd like to avoid that dependency.

@Emantor
Copy link
Contributor Author

Emantor commented Oct 19, 2018

I removed the allowed positions from sways command. The command already returns an error for invalid positions, so imo no change is needed there. Should be ready for review.

@RyanDwyer
Copy link
Member

Swaybar does currently not link against wlroots

I don't know where you're getting your information from. It's linked in swaybar/meson.build, and if you search the swaybar directory for wlr_log you'll see it's used dozens of times.

@Emantor
Copy link
Contributor Author

Emantor commented Oct 20, 2018

Swaybar does currently not link against wlroots

I don't know where you're getting your information from. It's linked in swaybar/meson.build, and if you search the swaybar directory for wlr_log you'll see it's used dozens of times.

I did rg "wlr*.h" in the swaybar directory, however * does not match / and since the include path is wlr/util/log.h I assumed that swaybar is not linked against wlroots, since my grep returned no matches.

I replaced the fprintf with wlr_log, sorry for the noise.

The positions "left" and "right" are not allowed by the man page, remove them
from the allowed positions. Also print an error to stderr if we default to the
bottom position.

Fixes swaywm#2878
"left" and "right" are not allowed positions for swaybar, remove them.
@emersion
Copy link
Member

Thanks!

@Emantor Emantor deleted the fix/swaybar_position branch October 20, 2018 06:35
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.

4 participants