Refactor notification init #428

bebehei · 2017-11-02T10:46:11Z

Refactors notification_init to split it into multiple methods.

tsipinakis · 2017-11-02T11:05:43Z

src/notification.c

 */
 notification *notification_create(void)
 {
-        return g_malloc0(sizeof(notification));
-}
+        //FIXME: handle OOM


From glibs docs

If any call to allocate memory fails, the application is terminated. This also means that there is no need to check if the call succeeded.

Means we don't need to handle OOM.

Thanks. This wasn't clear for me. Only g_try_malloc0 mentions the behavior of g_malloc0.

tsipinakis · 2017-11-02T11:08:39Z

src/notification.c

+        /* Color hints */
+        n->colors[ColFG] = n->colors[ColFG]
+                         ? n->colors[ColFG]
+                         : xctx.colors[ColFG][n->urgency];


I really don't like using xctx inside non-X related functions. Though I guess this will have to stay as-is until the drawing refactor is done.

I agree with you. But previously, notification_init also had to access this struct. It's bad, but it's not getting worse (and hopefully gets fixed in refactor drawing).

Also, I can't help but feel like all these ternary operators are a bit ugly - what's wrong with a usual if?

tsipinakis · 2017-11-02T11:10:25Z

src/notification.c

+static void notification_extract_urls(notification *n)
+{
+        // DO markup urls processing here until we split this out correctly
+        n->urls = notification_extract_markup_urls(&(n->body)); //WHAT THE FUCK!?


I'm not sure what the WTF is here for? - Sure not the best code but not really weird? The contents of that function are many times worse.

Thank you for mentioning this again.

For me, this function call is against every logic. Why do we put a double pointer to that (and not the notification and turn it into a method like every other method).

I also don't see, why we do have to make changes to the body field there. Markup should be extracted somewhere else.

There is a discrepancy between pango and notification spec. Notification spec requires img and a tags, while pango cannot render these (obviously). As a tags happen quite regularly, these have to get replaced. Guess, what this method did, too!?

Let's have these two notifications:

notify-send asdf '<a href="https://github.com/">testname</a>'

n->urls = 0x5555558675f0 "[#1] https://github.com/"

notify-send https://summary-url.de https://internal.com

1: n->urls = 0x555555803980 "https://summary-url.de\nhttps://internal.com"

Mhmm, same field, different format, 👎

bebehei · 2017-11-03T02:15:39Z

@tsipinakis As I'm tired right now and may have introduced some bugs by myself with the lastest commits, I will review this by myself at first. I will "ping" you by re-requesting a review.

OT: I would be able to click on merge for my own PRs, while you requested changes in your current review. Would you mind changing this to make a positive review a neccessity?

tsipinakis · 2017-11-03T10:46:33Z

OT: I would be able to click on merge for my own PRs, while you requested changes in your current review. Would you mind changing this to make a positive review a neccessity?

I tweaked the branch restrictions a bit and set it to require a positive review and a passing build to push to master. Thought technically you'd be able to review and approve your own pr, right?

bebehei · 2017-11-03T10:47:51Z

Thought technically you'd be able to review and approve your own pr, right?

I guess not. Let's test it!

Edit: No, I cannot approve/request changes on my own pull request. I can only add new comments.

bebehei · 2017-11-06T16:18:16Z

@tsipinakis has enlightened me about the previous functionality of notification_extract_markup_urls right now in IRC.

I heavily want to drop this ugly function and replace it with something similar. The specific functionality of adding [ #linkno] in front of the URL is quite bogus for me. I don't see why this is neccessary. IMHO, there is show_indicators for such functionality. And also having the linknumber included in the urls field makes it quite ugly, as there is no consistency to extract_urls.

(Edit: I also found, that notification spec mentions to show it in standard blue underlined text.)

Currently I have also removed it in this PR and I'd like to evaluate which path we want to go.

bebehei · 2017-11-20T16:29:34Z

[...] and I'd like to evaluate which path we want to go.

@tsipinakis Could you please give your opinion here?

tsipinakis · 2017-11-22T16:39:30Z

src/notification.h

@@ -30,34 +30,44 @@ typedef struct _actions {
 } Actions;

 typedef struct _notification {
+        int id;


I'd group this differently: id and dbus_client together and appname, summary, body, category and urgency in another group since they're the main notification attributes.

Grouped it accordingly now.

tsipinakis · 2017-11-22T16:45:35Z

The specific functionality of adding [ #linkno] in front of the URL is quite bogus for me. I don't see why this is necessary.

As I've explained on IRC, as I view it this is because there is no clear indication of which url belongs to which piece of text in the notification when it comes to a href tags. For plain urls this is not necessary since it's displayed as text anyway.

IMHO, there is show_indicators for such functionality.

show_indicators shows that there's a url in the notification - it doesn't provide any information as to which piece of text contains which url.

(Edit: I also found, that notification spec mentions to show it in standard blue underlined text.)

We should probably comply with this but it might be a bit misleading since clicking on it won't work with the current implementation.

bebehei · 2017-11-22T17:13:58Z

We should probably comply with this but it might be a bit misleading since clicking on it won't work with the current implementation.

Question to understand this: Does pango support $something to know, that the user clicked on a specific textbit?

tsipinakis · 2017-11-22T17:18:25Z

Does pango support $something to know, that the user clicked on a specific textbit?

Quick google didn't turn up anything - I have no clue.

bebehei · 2017-11-23T12:36:34Z

it doesn't provide any information as to which piece of text contains which url.

what's better than a linknumber? Put in the actual text! So, if you open up dmenu it will display [linktext] URL.

e.g:

notify-send '<a href="https://github.com/dunst-project/dunst/pull/428">GitHub</a>' 'Pull Request updated'

will result in:

n->urls = "[GitHub] https://github.com/dunst-project/dunst/pull/428"

This is also more comfortable.

tsipinakis · 2017-11-25T07:58:24Z

what's better than a linknumber? Put in the actual text! So, if you open up dmenu it will display [linktext] URL.

While it seems like a good suggestion it could break in some scenarios like having a super long link text, or having two links with the same text

The first example is this and the second is this, which one do you prefer?

would produlce

[this] URL [this] URL

While the order would give it away somewhat, it's still confusing.

This is an antipattern and makes linelength increase for nothing

bebehei · 2017-11-26T02:07:25Z

[...] or having two links with the same text [...] While the order would give it away somewhat, it's still confusing.

I agree with you. It's actually confusing. But is it dunst's job to make confusing URLs clear? There is no information lost on dunst's behalf.

While it seems like a good suggestion it could break in some scenarios like having a super long link text,

Is a long URL currently handled better?

[ Hi Mike, I've got you a super long URL,
which renders faulty on your system but
it's good! Check it out! #1]

How long do you need to parse that the URL number 1 takes the whole notification?
What's the probability of having a second URL in a notification with a superlong URL?

I have to admit, I have only searched for a replacement of the current URL replacement, but I think the solution I came up with is actually much better than the current approach.

Unboil the spaghetti code and split it into separate methods.

bebehei · 2017-12-04T23:26:27Z

@tsipinakis I fixed all issues with the URL extraction methods. I added pelnty of tests. Have fun with "Remove a and img tags from msg". ;-)

tsipinakis · 2017-12-10T15:24:05Z

src/notification.c

        if (n->actions) {
-                n->actions->dmenu_str = NULL;
+                g_clear_pointer(&n->actions->dmenu_str, g_free);


dmenu_str is empty at this point so this g_clear_pointer call is superfluous, and must be 0 since we allocate using g_malloc0 in the case that it contains junk this can cause a crash.

There is no guard against calling this function a second time. Setting dmenu_str solely to NULL is irresponsible and makes absolutely no sense.

I've done this in every function, which rewrites some fields.

tsipinakis · 2017-12-10T16:13:50Z

src/markup.c

+                }
+
+                // text between a tags
+                int text_len = tag2


I feel like you're overusing the ternary operator. Another case that can be a lot more readable with a simple if.

You marked the exact line to let this code look as confusing as possible. 😂

I fully agree with you.

tsipinakis · 2017-12-11T20:16:13Z

src/markup.c

+                        src_e = strstr(src_s + strlen("src=\""), "\"");
+
+                // Move pointer to the actual start
+                alt_s = alt_s ? alt_s+5 : NULL;


You're using strlen above but +5 here, why the inconsistency?

Oh, yes. That's right.

tsipinakis

As for d38d747 and 56648a5 I think I prefer it the way it's implemented in 07bce72 IF it's officially supported behaviour by the testing framework.

tsipinakis · 2017-12-13T17:46:40Z

src/markup.c

+                        // invisible in the replaced text
+                        if (!text_alt || strlen(text_alt) == 0) {
+                                g_free(text_alt);
+                                text_alt = g_strdup("Invisible Image");


Instead of using Invisible Image for everything let's fall back to our previous system, numbers.

Notification with inline image [image #1]
And in dmenu
[#1] url

I agree. Setting the text to "Invisible image" sounds rather mysterious than helpful.

Sorry, I've seen this just now.

Notification with inline image [image #1]
And in dmenu
[#1] url

Please, no. As a user, I'd question the rationale removing it for links and then adding it again for images. As a user, I'd really insult this dev (myself).

Simply changing it to [Hidden Image] makes more sense IMO.

tsipinakis · 2017-12-13T17:52:35Z

test/markup.c

+
+TEST test_markup_strip_img(void)
+{
+        RUN_TESTp(helper_markup_strip_img, "v <img> img",                                         "v  img",       NULL);


Can tests really be nested like that? I don't see anything indicating so in greatest docs.

Yes and no. The code is indicating it, but it's nowhere documented.

It works and it's no problem. In functionality it's far better than calling an extra function.

I wish, there would be an additional layer between test and assertion.

Ok a small background info on this: When an assertion fails, it immediately returns the current function (which should be a test).

As we test multiple strings (done via multiple assertions), a failed assertion breaks the test function. By calling another subtest, the supertest does not fail and will test further for all strings.

While the notification spec allows tags like <a href="...">...</a> and <img src="..." alt="...">, pango cannot parse these tags and therefore these tags should be removed before passed to pango. Also the method notification_extract_markup_urls is not needed anymore, as markup_strip_a can return URLs optionally. This implies, that URL replacement is now indicated via show_indicators for URLs and the dmenu string is in the format of '[text between a tags] URL\n'. This is similarly handled for images, too.

[linktext] can contain arbitary data and also a possible URL, which would get passed to the browser, making the actual URL invalid.

The hints given via DBus are not constants. Therefore the color fields have to get freed during notification cleanup. As it's not possible to disinguish, which field is constant, we have to duplicate the settings on assignment.

Handle replaces_id now via n->id

bebehei · 2017-12-14T23:27:43Z

@tsipinakis I've rebased the fixes already into their commits. If you want to see the actual diff, please see here: bebehei/dunst@refactor-notification_init...refactor-notification_init-backup

Edit: Oh, err. The "compare" thing of GitHub isn't working as expected. I'll summarize: Everything, what's changed, is the test_helper test structure.

tsipinakis

Couldn't find anything else, LGTM 👍

bebehei · 2017-12-17T23:56:48Z

Finally!

…on_init Refactor notification init

bebehei added the refactor label Nov 2, 2017

bebehei added this to the v1.3.0 milestone Nov 2, 2017

bebehei requested a review from tsipinakis November 2, 2017 10:46

tsipinakis requested changes Nov 2, 2017

View reviewed changes

bebehei force-pushed the refactor-notification_init branch from 4ac023a to 8aae64d Compare November 3, 2017 10:15

bebehei force-pushed the refactor-notification_init branch from 9c4115e to e3238c6 Compare November 6, 2017 16:24

bebehei force-pushed the refactor-notification_init branch 6 times, most recently from 757fce4 to 20644f3 Compare November 21, 2017 17:49

tsipinakis reviewed Nov 22, 2017

View reviewed changes

bebehei force-pushed the refactor-notification_init branch from 20644f3 to a1db6b2 Compare November 22, 2017 17:17

bebehei force-pushed the refactor-notification_init branch 2 times, most recently from 6c846eb to e0fcbce Compare November 23, 2017 12:31

bebehei added 2 commits November 25, 2017 10:58

Restructure notification struct and add comments

d190af6

Rename color_strings to colors

eae071f

This is an antipattern and makes linelength increase for nothing

bebehei force-pushed the refactor-notification_init branch from e0fcbce to 060d6ee Compare November 25, 2017 10:03

Split and refactor notification_init

4bfae81

Unboil the spaghetti code and split it into separate methods.

bebehei force-pushed the refactor-notification_init branch 2 times, most recently from 3e27bf9 to d171ce1 Compare December 4, 2017 22:06

bebehei force-pushed the refactor-notification_init branch 2 times, most recently from 3941441 to 56648a5 Compare December 6, 2017 12:47

bebehei mentioned this pull request Dec 7, 2017

Additional logging & handling logging during tests #368

Closed

3 tasks

tsipinakis reviewed Dec 11, 2017

View reviewed changes

tsipinakis requested changes Dec 13, 2017

View reviewed changes

bebehei force-pushed the refactor-notification_init branch 5 times, most recently from c7d376a to 5e7df83 Compare December 14, 2017 21:16

bebehei added 5 commits December 15, 2017 00:16

Strip [linktext] before opening URL

98d905b

[linktext] can contain arbitary data and also a possible URL, which would get passed to the browser, making the actual URL invalid.

Fix memory leak colors given via hints

b97a49c

The hints given via DBus are not constants. Therefore the color fields have to get freed during notification cleanup. As it's not possible to disinguish, which field is constant, we have to duplicate the settings on assignment.

Split notification assembly into separate method

3face4a

Handle replaces_id now via n->id

Show multiple URLs pretty with print

aae73d8

bebehei force-pushed the refactor-notification_init branch from 3d3a719 to aae73d8 Compare December 14, 2017 23:16

tsipinakis approved these changes Dec 17, 2017

View reviewed changes

bebehei merged commit 35dbd00 into dunst-project:master Dec 17, 2017

bebehei deleted the refactor-notification_init branch December 18, 2017 03:01

bebehei restored the refactor-notification_init branch January 6, 2018 18:32

bebehei mentioned this pull request Jan 6, 2018

Moving tests to GLib? #473

Closed

bebehei mentioned this pull request Jan 15, 2018

Changing urgency by a rule does not affect color of message #484

Closed

bebehei deleted the refactor-notification_init branch March 15, 2018 13:30

karlicoss pushed a commit to karlicoss/dunst that referenced this pull request Mar 21, 2019

Merge pull request dunst-project#428 from bebehei/refactor-notificati…

9a2aa48

…on_init Refactor notification init

Refactor notification init #428

Refactor notification init #428

Conversation

bebehei commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsipinakis Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bebehei commented Nov 3, 2017

tsipinakis commented Nov 3, 2017 • edited Loading

bebehei commented Nov 3, 2017 • edited Loading

bebehei commented Nov 6, 2017 • edited Loading

bebehei commented Nov 20, 2017

tsipinakis Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsipinakis commented Nov 22, 2017 • edited Loading

bebehei commented Nov 22, 2017

tsipinakis commented Nov 22, 2017

bebehei commented Nov 23, 2017

tsipinakis commented Nov 25, 2017

bebehei commented Nov 26, 2017

bebehei commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsipinakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bebehei commented Dec 14, 2017 • edited Loading

tsipinakis left a comment • edited Loading

Choose a reason for hiding this comment

bebehei commented Dec 17, 2017

tsipinakis Nov 2, 2017 •

edited

Loading

tsipinakis commented Nov 3, 2017 •

edited

Loading

bebehei commented Nov 3, 2017 •

edited

Loading

bebehei commented Nov 6, 2017 •

edited

Loading

tsipinakis Nov 22, 2017 •

edited

Loading

tsipinakis commented Nov 22, 2017 •

edited

Loading

bebehei commented Dec 14, 2017 •

edited

Loading

tsipinakis left a comment •

edited

Loading