-
Notifications
You must be signed in to change notification settings - Fork 12
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
patch: vertical mode #14
base: master
Are you sure you want to change the base?
Conversation
xnotify.c
Outdated
@@ -931,7 +931,7 @@ additem(struct Queue *queue, struct Itemspec *itemspec) | |||
if(config.vertical) { | |||
item->textw = queue->w - config.padding_pixels * 2; | |||
} else { | |||
item->textw = queue->w - config.image_pixels - config.padding_pixels * 3; | |||
item->textw = MAX(1, queue->w - config.image_pixels - config.padding_pixels * 3); |
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 noticed this issue, which was already there on master. Basically, if you specify an image wider than the width of the notification, item->textw
goes negative, and XCreatePixmap
fails. I work around this by adding an extra 1 pixel for this scenario, but feel free to suggest a more elegant solution!
8782df0
to
d210a26
Compare
So I think I'll do like herbe and keep big patches as pull requests. I'll fix that bug, thanks for finding it! |
I did another change and I could not reproduce the bug here, can you try if the bug persists? |
d210a26
to
601c59a
Compare
Seems fixed. I've resolved the conflict as well. |
This PR introduces a flag (
-v
) and Xresource (xnotify.vertical
), which causes the image to be placed on top of the text instead of to the left of it.This introduces quite some changes, I hope I didn't break anything. I have tried to test it in all combinations I could think of (with/without image, vertical/not vertical, shrink/not shrink).