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

Partial icon support #115

Merged
merged 17 commits into from Feb 20, 2019

Conversation

Projects
None yet
5 participants
@algmyr
Copy link
Contributor

algmyr commented Feb 16, 2019

image

Adds preliminary support for icons, assuming they are given as an image path. Support for the other options in https://developer.gnome.org/notification-spec/#icons-and-images could be worked on later.

Image loading code is mostly borrowed from swaybg and adds a dependency on gdk-pixbuf. This could be made an optional dependency, I removed that aspect for the sake of simplicity when writing the code.

Some changes to the present rendering code had to be done

  • If no icon is present (or if it failed to load) then the behavior would be unchanged
  • If an icon is present a spacing of padding.left is given on both sides (padding, icon, padding, text)
  • The icon and text are both vertically centered, so it looks nice if text is taller than the icon and vice versa

Additional icon logic

  • The icon is fitted (maintaining aspect ratio) inside a square of max_icon_size

Adding support for the other icon formats in the spec wouldn't change the rendering logic, so hopefully this can be merged independently. Some code would have to change to pass relevant parameters, but that's about it.

Partially resolves #37.

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 16, 2019

That CI fail is from the missing gdk-pixbuf, as expected. Not entirely sure what exact line has to be added to get the right dependency. Would guess gdk-pixbuf or gdk-pixbuf-2.0.

@algmyr algmyr changed the title Partical icon support Partial icon support Feb 16, 2019

@emersion emersion requested a review from vilhalmer Feb 16, 2019

@emersion

This comment has been minimized.

Copy link
Owner

emersion commented Feb 16, 2019

Thanks for your pull request! Can you rebase it, I've just merged another one and there is a conflict?

Also, before this is merged, I'd prefer to have a Meson option to disable gdk-pixbuf.

icon.c Outdated

#include "icon.h"

cairo_surface_t* gdk_cairo_image_surface_create_from_pixbuf(const GdkPixbuf *gdkbuf) {

This comment has been minimized.

@emersion

emersion Feb 16, 2019

Owner

Hmm, I'm not a fan of this function.

This comment has been minimized.

@algmyr

algmyr Feb 16, 2019

Author Contributor

Name-wise or functionalify-wise? The name is unchanged from swaybg, and the function itself is the same apart from logging changes and some ifdefs.

This comment has been minimized.

@vilhalmer

This comment has been minimized.

@emersion

emersion Feb 16, 2019

Owner

Yeah, using gdk_cairo_image_surface_create_from_pixbuf (or similar) would be a lot better :)

This comment has been minimized.

@vilhalmer

vilhalmer Feb 16, 2019

Collaborator

There's also gdk_cairo_set_source_pixbuf which I use in oguri. I'm not familiar enough to know what the particular upsides and downsides of each method are.

This comment has been minimized.

@algmyr

algmyr Feb 16, 2019

Author Contributor

I guess the downside of both those is a dependence also on gdk (although that can be made optional along with gdk-pixbuf). For the latter I assume that would be used in render.c? If so it might be nicer to do the former (assuming it works properly) and return a cairo surface as I do now. That way all the ifdefs related to optional gdk stuff is constrained to icon.c.
Edit: Scratch that part about the former one. There is a reason people have written their own replacements, that function will segfault if run before a gdk_init. Let's avoid that. I'll look into the second option, I think I have an idea.

@vilhalmer

This comment has been minimized.

Copy link
Collaborator

vilhalmer commented Feb 16, 2019

(I'll hopefully be able to review this later today, at the very least sometime this weekend.)

@algmyr algmyr force-pushed the algmyr:icon_support branch from 5eae1b3 to ae5d072 Feb 16, 2019

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 16, 2019

That should be the rebase done. Code rewritten to use gdk_cairo_set_source_pixbuf. All ifdefs are contained in icon.c and icon.h. Flag added to compile with/without icons, defaults to compile with icons if the dependencies are there. It's worth noting that using the gdk function drags in gtk3 as a dependency as well (same as in oguri). I guess that's why sway and other project decided to roll their own converter instead of having gtk3 as a dependency.

Also, the drawing logic is now in icon.c since it uses gdk.

@emersion

This comment has been minimized.

Copy link
Owner

emersion commented Feb 16, 2019

Instead of having guards in icon.{c,h}, is it possible to move them in render.c?

@emersion

This comment has been minimized.

Copy link
Owner

emersion commented Feb 16, 2019

Would be nice to have an option to disable completely icons too.

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 16, 2019

Would be nice to have an option to disable completely icons too.

As a boolean provided at runtime?

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 16, 2019

Ok, guards moved and runtime option added.

Also, when noticing I had missed adding an option to the help I also saw --ignore-timeout is not listed in there. Not sure if it's intentional or not.

@vilhalmer
Copy link
Collaborator

vilhalmer left a comment

A few small things, and one bigger one. Thanks for your work on this, I'm excited to have icon support!

Show resolved Hide resolved include/icon.h Outdated
Show resolved Hide resolved include/icon.h Outdated
Show resolved Hide resolved main.c Outdated
Show resolved Hide resolved meson.build Outdated
Show resolved Hide resolved render.c Outdated
Show resolved Hide resolved render.c Outdated
Show resolved Hide resolved icon.c Outdated
Show resolved Hide resolved include/icon.h Outdated
Show resolved Hide resolved icon.c Outdated
Show resolved Hide resolved icon.c Outdated
@vilhalmer

This comment has been minimized.

Copy link
Collaborator

vilhalmer commented Feb 17, 2019

Also, when noticing I had missed adding an option to the help I also saw --ignore-timeout is not listed in there. Not sure if it's intentional or not.

This is not intentional.

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 17, 2019

Is SHOW_ICONS a good name for the define? I just named it whatever at the time. Maybe something like ICON_SUPPORT would be nicer? Also, I'll start pushing small commits and resolving comments. Can squash them later if you feel that's preferable.

algmyr added some commits Feb 17, 2019

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 17, 2019

A bit conflicted about where to do the ifdefs. Original thought was to put them in icon.{c,h} and let a neutered icon struct be left in the code. After a suggestion I moved them to render.c.

Now, moving things to handle_notify and into the notifications that means quite a few more ifdefs. It's probably better to go back to a neutered icon struct (and icon functions).

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 17, 2019

Made some larger changes when moving creation and destruction into notification code and such.

Quick overview:

  • a mako_icon is now part of the notification as a pointer
  • functions in icon.c now operate on icon pointers
  • all ifdefs in the code are gone except for one in icon.c
  • the icons images are loaded, painted (at the correct size) to a cairo surface, and then released

I think (hope) this cleaned up a lot of code (fewer ifdefs, better structure in icon.c). But this essentially means looking over most of the code another round to check that I didn't introduce anything bad again. Sorry about that.
Edit: Just realized, the early resizing does not take output scaling into account, which means images will be upscaled and blurry. Is output scale known when handle_notify happens? If so I can fix that.

@vilhalmer

This comment has been minimized.

Copy link
Collaborator

vilhalmer commented Feb 18, 2019

But this essentially means looking over most of the code another round to check that I didn't introduce anything bad again. Sorry about that.

On the contrary, thanks for working through the change requests. :)

Is output scale known when handle_notify happens? If so I can fix that.

Hmm, this is a good point that I hadn't considered. The output scale is only known if we know which output we're displaying on, which in the default mode can change whenever there are zero notifications open and thus no surface. Perhaps it should scale to the largest factor of any of the outputs connected when loading the image, then scale down in render if the chosen output is smaller than this? (Does this happen automatically if you draw the cairo surface into a smaller target area?) That still prevents needing to scale from the full source size each time, and in the happy case only does it once. Sorry to give you the run-around on this, but I'm glad you caught it.

This still isn't perfect, as if another display is connected it could have a larger scale factor. @emersion any thoughts on how clever we want to be here?

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 20, 2019

I think that should cover it. I've tried running with a different output scale and the notifications look fine.

Now, a remark that is not related to this PR:
I think the border rendering is a bit off. This is an issue I kinda felt was there from the beginning (the inside of the border sometimes look see through) and the border didn't look like it aligned with the rest of the notification (the border seems left shifted slightly). Looking at the upscaled version it's a lot more apparent (excuse the fonts, pixel fonts doesn't scale well with antialiasing):
image
This is not an issue with this PR, it's there in master as well.

Edit: I found the issue. I made a new PR for it: #117

Show resolved Hide resolved meson.build Outdated
Show resolved Hide resolved meson.build Outdated
Show resolved Hide resolved meson_options.txt Outdated
Show resolved Hide resolved main.c Outdated
Show resolved Hide resolved icon.c Outdated
Show resolved Hide resolved icon.c
Show resolved Hide resolved dbus/xdg.c Outdated
@emersion
Copy link
Owner

emersion left a comment

Here are a few more comments, nothing major. This looks good otherwise!

@emersion

This comment has been minimized.

Copy link
Owner

emersion commented Feb 20, 2019

Oh, if style->icons is true, we should probably advertise the icon-static capability.

algmyr added some commits Feb 20, 2019

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 20, 2019

Oh, if style->icons is true, we should probably advertise the icon-static capability.

What about the case where we have don't have support for icons, but when style->icons is still true?

Is adding an ifdef around the capability part a reasonable solution? I think it's either that or forcing style->icons to be false if there is no icon support. First option feels like treating a symptom rather than the sickness, second option feels like it could be ugly to implement (a lot of ifdefs in config.c).

@emersion

This comment has been minimized.

Copy link
Owner

emersion commented Feb 20, 2019

Is adding an ifdef around the capability part a reasonable solution?

Maybe we should disallow setting style->icons to true when built without icons support?

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 20, 2019

Maybe we should disallow setting style->icons to true when built without icons support?

So, in the case of no icon support: Make the default value of style->icons false and then remove all other places where it can be set?

@emersion

This comment has been minimized.

Copy link
Owner

emersion commented Feb 20, 2019

Make the default value of style->icons false and then remove all other places where it can be set?

Yeah, but I think there's only one such place. Also, we could assert(false) in create_icon.

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 20, 2019

Something like this?

#ifdef HAVE_ICONS
	return spec->icons =
		parse_boolean(value, &style->icons);
#else
	bool b;
	bool success = parse_boolean(value, &b);
	if (b) {
		fprintf(stderr, "Icon support not built in, ignoring icons setting.\n");
	}
	return success;
#endif

Feels a bit clumsy.

Edit: Maybe just

#ifdef HAVE_ICONS
	return spec->icons =
		parse_boolean(value, &style->icons);
#else
	fprintf(stderr, "Icon support not built in, ignoring icons setting.\n");
	return true;
#endif
config.c Outdated
@@ -287,6 +302,8 @@ bool apply_superset_style(
max(style->padding.bottom, target->padding.bottom);
target->padding.left = max(style->padding.left, target->padding.left);
target->border_size = max(style->border_size, target->border_size);
target->icons = style->max_icon_size || target->max_icon_size;

This comment has been minimized.

@algmyr

algmyr Feb 20, 2019

Author Contributor

I'm impressed no one noticed this gem of a copy-paste error. Oh well, better late than never.

This comment has been minimized.

@emersion

emersion Feb 20, 2019

Owner

Eh, good catch

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 20, 2019

I commited the changes I felt were reasonable. Will amend if needed.

@emersion
Copy link
Owner

emersion left a comment

LGTM, nice work!

@emersion emersion merged commit e3a69aa into emersion:master Feb 20, 2019

1 check passed

builds.sr.ht: .build.yml builds.sr.ht job completed successfully
Details
@emersion

This comment has been minimized.

Copy link
Owner

emersion commented Feb 20, 2019

Thanks!

@algmyr

This comment has been minimized.

Copy link
Contributor Author

algmyr commented Feb 20, 2019

Thanks to you two as well! It took some time, but the merged feature is vastly improved compared to what it started out as.

@smlx

This comment has been minimized.

Copy link
Contributor

smlx commented Feb 20, 2019

This is great, thank you everyone!

@lhanson

This comment has been minimized.

Copy link

lhanson commented Feb 20, 2019

Thanks from a happy user!

@vilhalmer

This comment has been minimized.

Copy link
Collaborator

vilhalmer commented Feb 21, 2019

Thanks for your work on this @algmyr!

Oh, if style->icons is true, we should probably advertise the icon-static capability.

Good catch, I totally forgot about capabilities.

@algmyr algmyr deleted the algmyr:icon_support branch Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.