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

limit icon size when we have a fixed width #548

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

knopwob
Copy link
Member

@knopwob knopwob commented Sep 17, 2018

This is a proposed fix for #540.

I'm not reallty sure/happy with this fix but it seems to be the easiest.

I'm not sure wether we should overwrite max_icon_size if it's not 0, or should we assume the user knows what he's doing?

Copy link
Member

@bebehei bebehei left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is the right place. Regarding shrink, I guess this has to get calculated more dynamically on every run. Right?

src/settings.c Show resolved Hide resolved
@knopwob
Copy link
Member Author

knopwob commented Sep 18, 2018

Shrink will still work. The image is limited to the max_win_width/2, but when the rest of the notification is smaller than half the width it will still shrink the window. It looks rather weird but that's always the case when you have a huge image.

I agree that there are probably better solutions to this if you factor this into the drawing/icon code but
I'm not sure it is worth it to complicate that code even further.

Another way to see it is that the user created a configuration that is broken.
He wants a window that is limited to a max width but on the other hand wants to see
icons in their full size (or he set max_icon_size to a value that is unreasonably high for his max window width). So one could argue that we're just sanitizing the configuration. :D

It would be nice to have the option to resize the image to a size that's fit the rest of the notification. In other words layout the text first and derive the icon size from the resulting text size.
But I think this is a different feature.

Anyway you know the code better than me at this point so I leave the decision up to you wether you
want this fixed within the settings code or in the drawing/icon code.

@tsipinakis
Copy link
Member

@knopwob seems to be inactive here so I took over in order to unblock 1.4.0.

Can't see a better solution so I just added some warning messages to call it a day.

@bebehei
Copy link
Member

bebehei commented Feb 13, 2019

Could you please rebase on current master, to run through CircleCI, too?

@tsipinakis
Copy link
Member

Done!

@tsipinakis tsipinakis merged commit 7b3d85a into dunst-project:master Feb 14, 2019
@genofire
Copy link

genofire commented Apr 12, 2019

It does not work for me correct,
i run Dunst - A customizable and lightweight notification-daemon 1.4.0 (2019-03-30) with following config:

geometry             = 0x0-4+24
max_icon_size        = 32

But i get:

 WARNING: Max width was set to 0 but got a max_icon_size of 32, too large to use. Setting max_icon_size=0

Does i something wrong?

@tsipinakis
Copy link
Member

@genofire See #614 it's a bug when dynamic width is used. It's fixed in master and I'll put out a point release once the few other bugs that were discovered after 1.4 are fixed.

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.

4 participants