Added: support for round corners by dj95 · Pull Request #420 · dunst-project/dunst

dj95 · 2017-10-27T10:28:06Z

As requested in issue #358 here's the support for round corners via the x11 shape extension.

bebehei

I can't say anything about x_win_corners. @tsipinakis has to review this.

For the look and feel of this PR: IMHO the rounded corners are very ugly and should somewhat smoothed. I don't see this as in improvement yet.

bebehei · 2017-10-27T10:45:48Z


 #include <X11/X.h>
 #include <X11/Xlib.h>
+#include <X11/extensions/shape.h>


h > c. Please sort it.

bebehei · 2017-10-27T10:47:20Z

                    xrandr \
-                    xscrnsaver
+                    xscrnsaver \
+					xext


Keep track of your indentation please. For alignment you have to use spaces.

Also e < s. Please sort the libs correctly.

bebehei · 2017-10-27T10:51:06Z

    # automatically after a crash.
    startup_notification = false

+    # window corners


Please add this stuff to the manpage, too and elaborate it a bit more and give a unit. For a passive user, this could be anything (em, inch, pixels).

bebehei · 2017-10-27T11:14:55Z

I tweaked now the frame and corner options a bit to make dunst very ugly. I don't claim, that there is a user, who is so stupid, to configure dunst this way, but this explains the stuff, which should get fixed first:

For some reasons, compton (or in general the compositing manager) does not get informed, that the window is round.
Also when having frame > 0, the corners of the frame get rounded, but not the actual notification window. So frame and corner cannot get combined.

tsipinakis

Here's a quick review for x_win_round_corners, haven't gotten around to looking at the cairo stuff yet.

I've been having trouble finding good documentation on the X11 Shape APIs so reviewing this is a bit of a guess on their behaviour.

tsipinakis · 2017-10-27T15:00:10Z

    startup_notification = false

+    # window corners
+    corner = 0


rounded_corner_radius would be a better name for this setting in my opinion.

Edit: or just corner_radius

tsipinakis · 2017-10-27T15:04:24Z

+
+        XSetForeground(xctx.dpy, shape_gc, 1);
+
+        XFillArc(xctx.dpy, mask, shape_gc, 0, 0, dia, dia, 0, 23040);


2304 is an ugly way to say 360*64, it'll be optimized away by the compiler anyway and it's much better for readability to write it out.

This is the first time I'm looking into the X drawing API so correct me if I'm wrong but is there a reason you're drawing the entire 360 degrees instead of just the ones needed? Is there a downside to it?

To the second point: There's no specific reason to draw the entire 360 degrees. I just did it for simplicity. I'm going to test the behaviour with just filling the need arc.

tsipinakis · 2017-10-27T15:05:11Z

+        XSetForeground(xctx.dpy, shape_gc, 1);
+
+        XFillArc(xctx.dpy, mask, shape_gc, 0, 0, dia, dia, 0, 23040);
+        XFillArc(xctx.dpy, mask, shape_gc, width-dia-1, 0, dia, dia, 0, 23040);


Style nitpick: Please put a space before and after each - to make it obvious that it's an expression.

tsipinakis · 2017-10-27T15:22:21Z

+        GC shape_gc = XCreateGC(xctx.dpy, mask, 0, &xgcv);
+
+        int rad = settings.corner_radius;
+        int dia = 2 * rad;


The diameter of the corner should be capped to the window width/height so that it doesn't cause rendering weirdness in case it's bigger than the window size.

bebehei · 2017-10-29T13:15:22Z

Better: Now, there is frame in the corner. But the frame of the corner and the frame of the side do not match.

dj95 · 2017-10-29T13:53:54Z

Fixed the behaviour that frame and corner frame doesnt match in the following commit. This only happens, when the corner radius is bigger than the height of the notification. I restricted it to the maximum height as @tsipinakis stated out in his review.

bebehei · 2017-10-29T14:49:42Z

@dj95 Your screenshot looks correct.

I'm using dc10dce

There's a frame missing in the corners.

Ah, there's the missing frame. But, well 😕

Have you pushed the commit, you made the screenshot with?

dj95 · 2017-10-29T15:22:31Z

@bebehei Yes, I did. Can you post your configuration for the frame and corner radius so I can investigate? I played with the config values and for me it seem to work fine. 🤔

bebehei · 2017-10-29T15:28:35Z

@dj95 Here it is:dunstrc.txt

bebehei · 2017-10-29T15:30:42Z

I have to admit, I've set horrible values. But IMHO dunst should be able to handle (or sanitize) them.

Interestingly, I got now 3 notifications from thunderbird. There, the corner frame was correct. But after a short time, the lower one began to flap.

dj95 · 2017-10-29T15:56:27Z

I know. But to ensure the software quality dunst should be able to sanitize those values :)

dj95 · 2017-10-29T18:18:58Z

Now the problem of a too big radius is fixed and capped to the half of one notification 😊

bebehei

I've marked all unnecessary changes in your commits.

Also, as one of us will definitely come up and ask you to squash the commits. Could you do it please?

I have reviewd while you pushed you new commit.

About your latest commit: I see no problems in style anymore. These are looking quite good.

The only thing what's left are the round corners by the compositor. In my case, compton does not recognize round corners (albeit --detect-rounded-corners is set). This may be an issue related to chjj/compton#388.

bebehei · 2017-10-29T17:52:34Z

 # check if we need libxdg-basedir
 ifeq (,$(findstring STATIC_CONFIG,$(CFLAGS)))
-	pkg_config_packs += libxdg-basedir
+    pkg_config_packs += libxdg-basedir


Sorry, but this is unrelated to your intent of this PR. Please remove the space change. (I'll mark further changes with "unrelated".)

I did this one because the rest of the indentation in this file uses spaces, not tabs. But I'm going to revert this if you want it.

bebehei · 2017-10-29T17:53:01Z

 # dunstify also needs libnotify
 ifneq (,$(findstring dunstify,${MAKECMDGOALS}))
-	pkg_config_packs += libnotify
+    pkg_config_packs += libnotify


bebehei · 2017-10-29T17:54:22Z

        cairo_destroy(c);
        cairo_surface_destroy(image_surface);
        r_free_layouts(layouts);
-


bebehei · 2017-10-29T17:54:23Z

        c = cairo_create(image_surface);

        x_win_move(width, height);
+


bebehei · 2017-10-29T17:54:30Z


 void x_win_draw(void)
 {
-


bebehei · 2017-10-29T18:05:14Z

                        if (ev.xexpose.count == 0 && xctx.visible) {
+                                x_win_round_corners();
+
                                x_win_draw();


There are many more calls of x_win_draw(). One for example is in run() of dunst.c.

For my understanding the rounded corners have to be called every time the window gets drawn. Why don't you put it into x_win_draw directly?

I do it in the following commit. I first thought, that its only necessary on the expose event.

tsipinakis

I was also about to point out that negative values in the corner_radius setting aren't handled but when I tried it it actually produced a pretty nifty effect

It's a ~~bug~~ feature!

This PR is getting close to completion so as @bebehei said can you squash all fixups in order to make the final review easier?

tsipinakis · 2017-10-29T19:25:14Z

        int bg_height = MAX(settings.notification_height, (2 * settings.padding) + h);
        double bg_half_height = settings.notification_height/2.0;
        int pango_offset = (int) floor(h/2.0);
+        double degrees = M_PI / 180.0;


This should be a const, we don't need to modify it.

@dj95 He meant to solely mark it with const (instead of defining a new macro).

tsipinakis · 2017-10-29T19:31:07Z

+        if (settings.frame_width > 0) {
+                if (first && last) {
+                        cairo_new_sub_path (c);
+                        cairo_arc (c, bg_x + bg_width - settings.corner_radius, bg_y + settings.corner_radius, settings.corner_radius, -90 * degrees, 0 * degrees);


Another style nitpick but I think this is borderline unreadable. I'd prefer if it was one parameter per line for long function calls like so:

cairo_arc (c, bg_x + bg_width - corner_radius, bg_y + corner_radius, corner_radius, -90 * degrees, 0 * degrees);

Same goes for XFillArc and XFillRectangle.

Though I'm not 100% on this decision. @bebehei What do you think? (I'm aware that this is the case for other calls in the same function but since it came up now might as-well discuss it and maybe start fixing it.)

@bebehei What do you think?

Well, I'd like to have parameters in one line. I prefer it.

bebehei · 2017-10-29T20:25:43Z

        else bg_height += settings.separator_height;

-        cairo_set_source_rgb(c, cl->frame.r, cl->frame.g, cl->frame.b);
+        cairo_set_source_rgba(c, cl->bg.r, cl->bg.g, cl->bg.b, 255);


Please use RGB. Everything else is also based on RGB

dj95 · 2017-10-29T20:37:45Z

I hope I haven't fucked up the commit history... Maybe you are more experienced to squash those commits.

dj95 · 2017-10-29T20:56:04Z

Or shall I squash all commits I made in this PR to one commit?

bebehei · 2017-10-29T20:57:55Z

Or shall I squash all commits I made in this PR to one commit?

Yes please.

bebehei · 2017-10-29T21:39:03Z

@tsipinakis has shown me this once: https://chris.beams.io/posts/git-commit/

dj95 · 2017-10-29T21:39:05Z

And I destroyed the functionality... damn. I'll take a look tomorrow

dj95 · 2017-11-01T14:02:45Z

Nevermind. It was just a wrong variable type. Amazing how different the graphical behaviour is 😄 I hope all fixes are now present :)

tsipinakis · 2017-11-01T20:29:22Z

Simply from looking at the diff I see that it still includes irrelevant changes from other commits (looks like parts of #421 are in there).

Can you rebase on master and remove them?

Also it might be beneficial to trim the commit message - we don't need to know what commits got squashed.

bebehei · 2017-11-02T10:36:10Z

@dj95 Before you work further, do this:

git clone https://github.com/dj95/dunst dunst-new
cd dunst-new
git checkout master
git pull --ff-only http://github.com/dunst-project/dunst/ master
git checkout feature/round_corners
git reset --hard master
git pull --ff-only http://github.com/bebehei/dunst/ 420-rebase
git push --force -u origin feature/round_corners

bebehei · 2017-11-06T16:00:45Z

ping @dj95

dj95 · 2017-11-08T12:25:25Z

I'm sorry that you need to wait that long, was a busy last week. I think the PR now contains just the changes that are needed for the round corners feature. Tests on my machine (and hopefully yours, too) are working fine. I'm a bit afraid to squash the commits because last time I made a mistake and squashed a merge from your master branch. I need to practice it a bit with a test-project first(This is my first pull request I've ever made :D ).

bebehei · 2017-11-08T12:30:29Z

@dj95 The two commits are intended.

bebehei · 2018-05-27T11:38:12Z

Looks much better. I'll review it later.

bebehei · 2018-05-27T17:37:51Z

Same configuration as before. Additonally a corner_radius set to 2000.

I think the approach to limit the corner radius to half the height of the notification is a bit counterintuitive, what do you think?

tsipinakis · 2018-05-27T17:42:36Z

I think the approach to limit the corner radius to half the height of the notification is a bit counterintuitive, what do you think?

It is but I don't see a reasonable alternative. If we force it to use the higher radius it just looks broken.

bebehei · 2018-05-28T00:18:26Z

Instead of mocking about the continuously repeating code, I had a try to touch X11/cairo code for myself. And it was a great learning experience.

It is but I don't see a reasonable alternative. If we force it to use the higher radius it just looks broken.

I guess we have to document this and give out the hint, that the user shall not choose a radius, which is bigger than half the height (or maybe even the width if the user has got a superweird geometry) of the smallest expected notification.

tsipinakis

Instead of mocking about the continuously repeating code, I had a try to touch X11/cairo code for myself. And it was a great learning experience.

That's awesome to hear and your changes do bring a big improvement in code readability.

I guess we have to document this and give out the hint, that the user shall not choose a radius, which is bigger than half the height (or maybe even the width if the user has got a superweird geometry) of the smallest expected notification.

We could but that has the disadvantage that if someone does want a higher radius when available they won't be able to use it without cutting off smaller notifications.

A middle ground might be to limit the radius to the minimum of the height of the first and last layout if there are more than 1 notifications being displayed and to height/2 if only one is.

With the latest commits separator_color = frame is broken but I cannot figure out what broke it, bisect says 040be9a. Keep in mind we don't redraw the separator on top if it's the same color as the frame, we just leave a gap when rendering the background (at least that's how it used to work).

tsipinakis · 2018-05-28T09:02:50Z

        const int width = win->dim.w;
        const int height = win->dim.h;
+        const int dia = 2 * rad;
+        const int degrees = 64;


I'd add a comment here to explain why 64, or just point to the docs.

Err, this is because of the docs ™️.

I'm aware, but someone looking at it a few months or years in the future is going to be really confused at this random value if they're not very familiar with the extension.

Good point. It definitely warrants a comment.

bebehei · 2018-05-28T09:47:55Z

With the latest commits separator_color = frame is broken but I cannot figure out what broke it, bisect says 040be9a. Keep in mind we don't redraw the separator on top if it's the same color as the frame, we just leave a gap when rendering the background (at least that's how it used to work).

Fuck it. Sorry for the strong language. Let's draw the separator actively. I'm forging the commit right now.

tsipinakis · 2018-05-28T10:03:14Z

+                          degrees * 180);
+        } else {
+                cairo_line_to(c, x + width, y+width);
+                cairo_line_to(c, x, y+width);


Let's draw the separator actively. I'm forging the commit right now.

While true it's simple to just draw over it and be done. Things not working like this can indicate some type of problem in the implementation. Like here, you're adding the width to the vertical axis, which is what broke the separator.

tsipinakis · 2018-05-28T10:39:33Z

I documented that corner_radius is automatically lowered in the man page and dunstrc. I'd call this one ready to be merged if you agree, with one suggestion being to drop 20b3c67, if everything is implemented properly that optimisation should work as intended and if not there's a good chance something else was broken alongside so it's worth keeping in IMO.

bebehei · 2018-05-28T10:44:06Z

with one suggestion being to drop c475a0b,

Simultaneously yes and no. I appreciate dropping this commit. But we're definitely missing here a test. AFAIK, I'm the third person stumbling over this bit in a PR and the only person, who found out about this, had been you.

tsipinakis · 2018-05-28T10:56:22Z

But we're definitely missing here a test.

We're definitely are, I'm looking into converting out tests to glib right now and after that I'll look into adding tests for the drawing part.

tsipinakis · 2018-05-28T10:59:20Z

Dropped it which dismissed your review, oops!

tsipinakis · 2018-05-28T11:03:27Z

Thanks @dj95 for the initial implementation it's been a while since we've implemented a major feature and this brings a nice change of pace.

(And as usual thanks @bebehei for the help/review!)

…unst#420)

bebehei requested changes Oct 27, 2017

View reviewed changes

tsipinakis requested changes Oct 27, 2017

View reviewed changes

bebehei requested changes Oct 29, 2017

View reviewed changes

tsipinakis reviewed Oct 29, 2017

View reviewed changes

bebehei reviewed Oct 29, 2017

View reviewed changes

dj95 force-pushed the feature/round_corners branch from 200994a to 8d3c36e Compare October 29, 2017 20:36

dj95 force-pushed the feature/round_corners branch 2 times, most recently from 75aa307 to f760ba8 Compare October 29, 2017 21:33

dj95 force-pushed the feature/round_corners branch from f83a95d to 3109610 Compare November 1, 2017 14:01

dj95 force-pushed the feature/round_corners branch from 3109610 to 92f9ee4 Compare November 8, 2017 12:15

tsipinakis force-pushed the feature/round_corners branch from 7c7f0b3 to 60cbc86 Compare May 27, 2018 12:33

bebehei force-pushed the feature/round_corners branch from 7612039 to aef05a9 Compare May 28, 2018 08:20

tsipinakis requested changes May 28, 2018

View reviewed changes

bebehei force-pushed the feature/round_corners branch from aef05a9 to c9052b2 Compare May 28, 2018 09:49

tsipinakis reviewed May 28, 2018

View reviewed changes

bebehei force-pushed the feature/round_corners branch from a70ad2f to ce65fa2 Compare May 28, 2018 10:07

tsipinakis force-pushed the feature/round_corners branch from ce65fa2 to eb1b5ca Compare May 28, 2018 10:35

dj95 and others added 2 commits May 28, 2018 13:36

Implement support for round corners

a3034d5

Simplify corner arcs in cairo

d5fd674

tsipinakis force-pushed the feature/round_corners branch from eb1b5ca to c475a0b Compare May 28, 2018 10:36

Simplify corner arcs in X11 code

aab9fef

tsipinakis force-pushed the feature/round_corners branch from c475a0b to 20b3c67 Compare May 28, 2018 10:41

bebehei previously approved these changes May 28, 2018

View reviewed changes

tsipinakis dismissed bebehei’s stale review via aab9fef May 28, 2018 10:58

tsipinakis force-pushed the feature/round_corners branch from 20b3c67 to aab9fef Compare May 28, 2018 10:58

tsipinakis approved these changes May 28, 2018

View reviewed changes

tsipinakis merged commit 15c252a into dunst-project:master May 28, 2018

bebehei mentioned this pull request May 28, 2018

Rounded Corners Implementation #358

Closed

bebehei mentioned this pull request Jun 22, 2018

Fix X11 memoryleak for round corners #523

Merged

Holzhaus added a commit to Holzhaus/dotfiles that referenced this pull request Jul 28, 2018

dunst: Add setting for rounded corners (introduced in dunst-project/d…

c7a8dab

…unst#420)


		XSetForeground(xctx.dpy, shape_gc, 1);

		XFillArc(xctx.dpy, mask, shape_gc, 0, 0, dia, dia, 0, 23040);

Conversation

dj95 commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bebehei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bebehei commented Oct 27, 2017

Uh oh!

tsipinakis left a comment

Choose a reason for hiding this comment

Uh oh!

tsipinakis Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsipinakis Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bebehei commented Oct 29, 2017

Uh oh!

dj95 commented Oct 29, 2017

Uh oh!

bebehei commented Oct 29, 2017

Uh oh!

dj95 commented Oct 29, 2017

Uh oh!

bebehei commented Oct 29, 2017

Uh oh!

bebehei commented Oct 29, 2017

Uh oh!

dj95 commented Oct 29, 2017

Uh oh!

dj95 commented Oct 29, 2017

Uh oh!

bebehei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsipinakis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsipinakis Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

dj95 commented Oct 27, 2017 •

edited

Loading

tsipinakis Oct 27, 2017 •

edited

Loading

tsipinakis Oct 27, 2017 •

edited

Loading

tsipinakis left a comment •

edited

Loading

tsipinakis Oct 29, 2017 •

edited

Loading

bebehei Oct 29, 2017 •

edited

Loading

bebehei commented Nov 2, 2017 •

edited

Loading