Implement #392, take 2

[xkr47]

New attempt at #392, with great ideas from @tsipinakis taken into account. Have still not attempted to test/take into account any DPI-related functionality, so cannot say yet how/if it will affect the outcome.

Do you want more tests for the scaling functionality? Current tests pass.

Implementation details: I moved the scaling responsibility to the lowest functions in the call hierarchy involved in loading the images - both when loading from disk and from the notification message so responsibilities are clearer code-wise. Made some functions static that are no longer needed elsewhere.

[xkr47]

Will check those CI errors and try to fix them.. Excellent that you have valgrind in your build process! 👍

[xkr47]

Apparently the function gdk_pixbuf_get_file_info() which I used to be able to get the default image size has a memory leak that is triggered when loading e.g. test/data/icons /path/invalid/icon1.svg. I currently have 2.36.11-1ubuntu0.1 of gdk-pixbuf and at least the version in the CI build also has the same issue.

@tsipinakis I could check with the latest release and with the latest development branch version of gdk-pixbuf to see if it is already fixed or try to get it fixed if not. But nevertheless I guess you'd want me not to use this function as not to cause memory leaks when loading broken images? As an alternative, I could just load the image using the normal gdk_pixbuf_new_from_file(), check its size and in case a different size is wanted, reload it using the gdk_pixbuf_new_from_file_at_scale(). What do you think?

[tsipinakis]

Weirdly enough I can't reproduce this with Debian stable or sid, which leads me to believe that this might've already been fixed and our CI images are simply out of date. It's also not clear if the leak is in gdk-pixbuf or librsvg, my bet is on the latter.

But nevertheless I guess you'd want me not to use this function as not to cause memory leaks when loading broken images?

Eh, that's a bug that'll even if it's not fixed yet it'll be fixed by the time this is released and reaches the distros. There is no need to hold off to using that function IMO.

---

On another note, some small TODOs for adding a new setting:

• Add min_icon_size to the dunstrc with a small explanation similar to max_icon_size
• Similarly, document its function in docs/dunst.pod.

[tsipinakis]

Weirdly enough I can't reproduce this with Debian stable or sid, which leads me to believe that this might've already been fixed and our CI images are simply out of date. It's also not clear if the leak is in gdk-pixbuf or librsvg, my bet is on the latter.

Oh! Duh! This is probably rsvg/147, our suppression file just didn't catch it because the stack trace changed with this PR. In this case you can just update .valgrind.supressions and ignore it.

[xkr47]

Thanks for your feedback & review!

[xkr47]

Do you have any preferred default value for min_icon_size? Should I set it to 0 to avoid changing current operation (except for downscaling of svgs which will unconditionally be improved slightly 😄)?

Also what is the purpose of the "default" values set in config.h?

[xkr47]

Are these out of sync?

• max_icon_size = 0 in config.h
• max_icon_size defaults to 0 in dunst.pod
• max_icon_size = 32 in dunstrc

[xkr47]

I want to add that this solution is not taking DPI into account as @bebehei requested. If we want it to scale icons based on DPI as well, I guess we would need to determine a base DPI like 96 or something, and then scale images by current_dpi / base_dpi. Are images reloaded when notification moves between montiors? I.e. do we have an opportunity to reload & rescale them to the correct (dpi-adjusted) size every time?

[xkr47]

If we go down the path of dpi-awareness I guess at least the following settings should be dpi-aware too:

• geometry
• line_height
• notification_height
• separator_height
• padding
• horizontal_padding

[tsipinakis]

Do you have any preferred default value for min_icon_size? Should I set it to 0 to avoid changing current operation (except for downscaling of svgs which will unconditionally be improved slightly smile)?

Leaving it at 0 is fine, I don't think there's any real reason to have it enabled by default.

Also what is the purpose of the "default" values set in config.h?

The default values in config.h is what will be used if a setting isn't present in the dunstrc. These weren't in sync when I took on maintaining dunst and since then I've never really looked more into fixing that because I don't want to break existing setups without a proper reason. (I've seen many people define only the settings they want in dunstrc and leave everything else use config.h values).

Are images reloaded when notification moves between montiors? I.e. do we have an opportunity to reload & rescale them to the correct (dpi-adjusted) size every time?

Yes, images are reloaded on every redraw (perhaps wrongly) - So it can be done and there's also opportunity for optimization to avoid rescaling every time.

I want to add that this solution is not taking DPI into account as @bebehei requested

IMO this is out of the scope of this PR, let's not delay merging this.
But if you're up to implementing proper DPI support don't let me discourage you by all means I'd love to see such a PR :) (Especially if you have the screen/setup to properly test it!)

---

Either way I'll do the final review either tonight or tomorrow, thanks for implementing this!

[xkr47]

Yeah the dpi scaling is apparently experimental and not even documented in the pod.. I almost started writing a script already that ensures everything is documented but got distracted..

[tsipinakis]

Other than the bug mentioned above everything else looks good. Thanks again for the contribution! :)

[xkr47]

Do you want more tests for the scaling functionality?

[tsipinakis]

Do you want more tests for the scaling functionality?

More tests are always better. I didn't mention anything about tests earlier because it didn't feel right asking for them when there aren't any for max_icon_size either.
If you have the time to add them please go ahead!

[xkr47]

I will try next week. If you are in a hurry I can create a separate PR later.

[tsipinakis]

@xkr47 Ping? No pressure, just wanted to make sure you haven't forgotten about this.

[xkr47]

Thanks! Yeah bit busy but not forgotten :)

[xkr47]

There we go, got it done thanks to some well-timed motivation speech :) I split most of dbus_notification_set_raw_image() test helper to a separate function in a separate file (see test/helpers.[ch]) so I could do better unit testing of images embedded in notifications.

Let me know of any improvements/corrections/etc you'd wish to see.

(The travis builds did not get very far and the failures feel totally unrelated to my code.)

[tsipinakis]

LGTM, merged! Kudos for taking care of the tests as well, it's the part most people dread.

[xkr47]

Excellent news! It was a pleasure really, the codebase was easy to follow and the existing test helpers did most of the dreaded hard work already :)