Feature advanced scripting

[xoores]

Continuing from discussion under #906:

This version can override several fields: urgency, icon_path, stack_tags, category, format, body, summary, progress, transient and timeout. It uses some bash incantations to acheve this, mmap to move data around and some pointer-based parsing.

I tried to document as much as possible to make it easier to understand what I was going for.

If script returns anything else than 0, the notification is thrown away. If one script returns non-zero status, no other scripts are run. This is for debate, someone might want to use this as e.g. way to push notifications to Slack or something and it this scenario it would be of course bad.

If script fails to run or if script outputs too much, it will be ignored (warn will be printed). Meaning no changes will be made to the notification - playing it safe here :slightly_smiling_face:

I run this version and I use it to change some fields like this:

#!/bin/bash

if [[ "${DUNST_BODY}" == *mattermost.xxx.yy* ]]; then
       	DUNST_BODY="$(echo "${DUNST_BODY}" | sed -e 's|<a[^@]*||g' -e 's|\(@[^:]*:\)|<b>\1</b>|g' | xargs)"
fi

Notes:

• It might be nice to specify some kind of timeout for the script since it is blocking
• script arguments should be abolished as there was no escaping etc (unpredictable)
• Some methods should be moved around - I wanted to edit only the files I had to (easier merging)
• Still have to finish fields such as colors as well :shrug:

I also changed the following:

• Changed notification_print so it uses LOG_* in more compact way
• Urgency-based colors are applied by default when urgency is changed through script or rule. If something changes color, urgency-based defaults will not be applied automatically.

Possibly related issues that might be addressed/solved by this: #899, #1012

Questions:

• Should I continue altering script or should I create some other option for this so it is 100% backwards compatible?

[xoores]

I fixed the -Wpedantic stuff in e73724ca8c7958d43f5523712c1f2306696f98c4, co next workflow should be fine :slightly_smiling_face:

[xoores]

Heh, nothing like a forgotten free :facepalm:

I'll have to figure out why Valgrind doesn't work on my computer, because I already tried to run it exactly because this... I think I found the leak but without Valgrind cannot be sure that it was the only one I've managed to overlook. Will push momentarily.

[fwsmit]

Hi, thanks for posting the PR. I've added some tips and tricks in HACKING.md. In short, for testing dunst with valgrind, you will need to run make test-valgrind. It's best to make sure all tests work on your local system before pushing to here. Let me know if you manage to get the tests running.

[xoores]

Yeah, the issue was not me not knowing how to use Valgrind but rather AVX512 instructions that my CPU supports and Valgrid does not :slightly_smiling_face:

When you try to run it with these instructions not disabled, it will look something like this:

==14025== Memcheck, a memory error detector
==14025== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==14025== Using Valgrind-3.20.0.GIT and LibVEX; rerun with -h for copyright info
==14025== Command: ./main
==14025== 
vex amd64->IR: unhandled instruction bytes: 0xC5 0xFD 0x47 0xC0 0xC5 0xE5 0x47 0xDB 0xC5 0xDD
vex amd64->IR:   REX=0 REX.W=0 REX.R=0 REX.X=0 REX.B=0
vex amd64->IR:   VEX=1 VEX.L=1 VEX.nVVVV=0x0 ESC=0F
vex amd64->IR:   PFX.66=1 PFX.F2=0 PFX.F3=0
==14025== valgrind: Unrecognised instruction at address 0x401954e.
==14025==    at 0x401954E: _dl_sysdep_start (in /lib64/ld-linux-x86-64.so.2)
==14025==    by 0x401B247: _dl_start (in /lib64/ld-linux-x86-64.so.2)

And while I rather quickly identified those as KMOV, I needed to re-compile my toolchain and libraries - it took mi a while :slightly_smiling_face: I got it running this morning.

5 tests - 5 passed, 0 failed, 0 skipped (12153 ticks, 0.012 sec)

Total: 183 tests (6465904 ticks, 6.466 sec), 2326 assertions
Pass: 183, fail: 0, skip: 0.

That const warning is PITA... I would maybe suggest to add -Werror to test-valgrind target? It would be much easier not having to juggle these flags around, especially if these are required for test to pass :slightly_smiling_face:

Fixed along with proposed change to the test-valgrind in 9acbfefc2a79eec3a6259a8ef40e0aa8f3f663b9

[xoores]

...wonder if you're not better off using dbus (we are already using it extensively, so why not more?). We could add dunstctl commands like dunstctl update ID summary "new summary". I'm not saying that this is neccesarily the best approach, but we should at least think about it.

I thought about it initially but the there are few things that speak against it IMO:

• You still need to somehow hook the notification reception and make dunst wait for response to another DBUS event
• Making DBUS listener+responder is quite hard in simple BASH script, it is not notification based etc.

We could, of course, create whole framework for DUNST interaction, much as i3 has one but I'd say that it would be completely different beast to do :slightly_smiling_face: It would be much more complex to implement (but OK, one-time job) but also much more demanding on someone who just wants to do some dynamic changes of few things in the notification itself. Instead of (relatively) simple BASH that almost everyone can use and hack something together, you'd end up atleast with Python.

I personally would say that BASH interface is much more user inviting - we could create few sample scripts so BFU can hack these for start. Much easier than Python/C/whatever else :slightly_smiling_face:

[xoores]

I've applied some of your suggestions - the rules make sense now... My fault - I thought that since example rules were at the end, I should just continue and leave urgency_* in the middle. Moved them to the end of config and works flawlessly!

Also I removed the crash when alloc fails.

2c17fcd08d0641cc1d32fa613ba9c81ac11ea433: Moved helpers to utils.c & utils.h as you suggested :slightly_smiling_face:

[xoores]

~~Should I address also that clang error?~~ Yes...

In file included from test/draw.c:1:
In file included from test/../src/draw.c:16:
test/../src/log.h:25:69: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
#define MSG(format, ...) "[%16s:%04d] " format, __func__, __LINE__, ## __VA_ARGS__

Ah, I see the problem - it runs make test/test to which I added -Werror... Okay, will think of a better way :laughing:

[fwsmit]

If you want to use -Werror in your own evironment, you can just set EXTRA_CFLAGS=-Werror before running make. I would not mess with the Makefile for that.

[fwsmit]

...wonder if you're not better off using dbus (we are already using it extensively, so why not more?). We could add dunstctl commands like dunstctl update ID summary "new summary". I'm not saying that this is neccesarily the best approach, but we should at least think about it.

I thought about it initially but the there are few things that speak against it IMO:

* You still need to somehow hook the notification reception and make dunst wait for response to another DBUS event

* Making DBUS listener+responder is quite hard in simple BASH script, it is not notification based etc.

We could, of course, create whole framework for DUNST interaction, much as i3 has one but I'd say that it would be completely different beast to do slightly_smiling_face It would be much more complex to implement (but OK, one-time job) but also much more demanding on someone who just wants to do some dynamic changes of few things in the notification itself. Instead of (relatively) simple BASH that almost everyone can use and hack something together, you'd end up atleast with Python.

I personally would say that BASH interface is much more user inviting - we could create few sample scripts so BFU can hack these for start. Much easier than Python/C/whatever else slightly_smiling_face

I thought a bit more about this and I think the bash interface with modifying env variables is fine. DBUS is probably a bit overkill for IPC, but I think pipes might be more suitable as a communication method that shared memory, since this is a one-directional communication scenario. Would you consider implementing pipes to replace the shared memory buffer?

Also, I see that you haven't made any tests yet. Could you do that as well?

[codecov-commenter]

Codecov Report

Merging #1072 (d641dfd) into master (c8e2748) will decrease coverage by 0.92%. The diff coverage is 15.72%.

@@            Coverage Diff             @@
##           master    #1072      +/-   ##
==========================================
- Coverage   65.33%   64.40%   -0.93%     
==========================================
  Files          46       46              
  Lines        7356     7516     +160     
==========================================
+ Hits         4806     4841      +35     
- Misses       2550     2675     +125     

Flag	Coverage Δ
unittests	64.40% <15.72%> (-0.93%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/rules.c	80.41% <ø> (ø)
src/utils.c	84.51% <0.00%> (-4.87%)	:arrow_down:
src/notification.c	50.44% <15.23%> (-9.23%)	:arrow_down:
src/queues.c	91.60% <66.66%> (-0.59%)	:arrow_down:
src/dunst.c	9.40% <0.00%> (-0.09%)	:arrow_down:
test/input.c	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c8e2748...d641dfd. Read the comment docs.

[xoores]

I thought a bit more about this and I think the bash interface with modifying env variables is fine. DBUS is probably a bit overkill for IPC, but I think pipes might be more suitable as a communication method that shared memory, since this is a one-directional communication scenario. Would you consider implementing pipes to replace the shared memory buffer?

Yeah, why not. I opted for the MMAP solely because the double-fork and this was the easiest solution at that moment :slightly_smiling_face:

Btw I also have some plans to implement Dunst module into Polybar so I can easily control Dunst without having to poll all the info through BASH script. For this I'd definitely go with DBUS :slightly_smiling_face: Maybe the interface already exist - did not really check yet.

Also, I see that you haven't made any tests yet. Could you do that as well?

You mean those CI/CD checks? I cannot run them on my own as I'm the first time contributor. I did run your Valgrind test suite locally though and I'm using this version ever since and 90% of my notifications are modified by the script and everything seems to be OK :slightly_smiling_face: Or do you mean enhancing current testsuite?

[fwsmit]

I thought a bit more about this and I think the bash interface with modifying env variables is fine. DBUS is probably a bit overkill for IPC, but I think pipes might be more suitable as a communication method that shared memory, since this is a one-directional communication scenario. Would you consider implementing pipes to replace the shared memory buffer?

Yeah, why not. I opted for the MMAP solely because the double-fork and this was the easiest solution at that moment slightly_smiling_face

I haven't looked at the implications of pipes and double forks, but I'm sure you can figure that out. If it's too complicated you can stick with mmap.

Btw I also have some plans to implement Dunst module into Polybar so I can easily control Dunst without having to poll all the info through BASH script. For this I'd definitely go with DBUS slightly_smiling_face Maybe the interface already exist - did not really check yet.

There are some dbus interfaces already. Most of them are accesible with dunstctl. You can a look at the man page of dunstctl to see what's there.

Also, I see that you haven't made any tests yet. Could you do that as well?

You mean those CI/CD checks? I cannot run them on my own as I'm the first time contributor. I did run your Valgrind test suite locally though and I'm using this version ever since and 90% of my notifications are modified by the script and everything seems to be OK slightly_smiling_face Or do you mean enhancing current testsuite?

You are able to add tests. You can add them in test/notification.c or any other relevant test file. Test that rely on you being able to see if the notification is drawn correctly, you can add in test/functional_test/test.sh.

[xoores]

I haven't looked at the implications of pipes and double forks, but I'm sure you can figure that out. If it's too complicated you can stick with mmap.

I'll take a look. I think I will have to basically read the output twice. Not really a problem per se. :slightly_smiling_face:

There are some dbus interfaces already. Most of them are accesible with dunstctl. You can a look at the man page of dunstctl to see what's there.

I will :slightly_smiling_face: I will be looking for Dunst event that state has changed (paused/unpaused) at the minimum and possibly notification that amount of notifications on hold has changed.

You are able to add tests. You can add them in test/notification.c or any other relevant test file. Test that rely on you being able to see if the notification is drawn correctly, you can add in test/functional_test/test.sh.

I'll take a look at it.

[liskin]

I haven't looked at the implications of pipes and double forks, but I'm sure you can figure that out. If it's too complicated you can stick with mmap.

I'll take a look. I think I will have to basically read the output twice. Not really a problem per se. slightly_smiling_face

Only if you try to keep both the double fork and the popen. If you drop popen and mmap and replace it with some pipe(2) and dup2(2) calls, there'll be no need to read it twice.

[xoores]

I haven't looked at the implications of pipes and double forks, but I'm sure you can figure that out. If it's too complicated you can stick with mmap.

I'll take a look. I think I will have to basically read the output twice. Not really a problem per se. slightly_smiling_face

Only if you try to keep both the double fork and the popen. If you drop popen and mmap and replace it with some pipe(2) and dup2(2) calls, there'll be no need to read it twice.

Yeah, I was also thinking about spawning a thread for this and share a memory directly. Dup2 would also be viable solution, perhaps even better as it will be completely isolated from Dunst. :+1:

[liskin]

Yeah, I was also thinking about spawning a thread for this and share a memory directly. Dup2 would also be viable solution, perhaps even better as it will be completely isolated from Dunst. +1

A thread indeed sounds like unnecessary complexity, especially since it wouldn't help you with the env var problem at all (those are shared across threads). If we didn't need to set all those env vars, a simple

if want blocking:
   popen
   blocking read
   pclose
else:
   double fork and forget

would work just as well.

Unfortunately we need to set a bunch of env vars for the child process only, and popen doesn't provide a way to do this, so you'll probably end up reimplementing popen using pipe/dup2/fork/execve/waitpid. :-/

[fwsmit]

Hi @xoores,

Do you agree with the suggested changes and are you able to make the time for it?

[xoores]

Unfortunately we need to set a bunch of env vars for the child process only, and popen doesn't provide a way to do this, so you'll probably end up reimplementing popen using pipe/dup2/fork/execve/waitpid. :-/

Yeah, but it's worth it to get the functionality :grin:

Do you agree with the suggested changes and are you able to make the time for it?

Of course. I have some work related things that I need to do first so I think I should be able to do this during the weekend. :slightly_smiling_face:

[fwsmit]

@xoores, it's been a while. Do you have time to finish this PR?

[fwsmit]

@xoores Hi, would you still like to finish this PR? Otherwise I'm afraid I'm gonna have to close it.

[xoores]

@fwsmit Hi, yeah! I had quite a lot on my plate (health-wise) so I did not really had an opportunity to clean it up.

Of course I want to finish it, I will just have to refresh what I did & did not. 🙂

[fwsmit]

Unfortunate to hear about your health. I hope you're doing better now. Please only finish this if you feel like it. I just wanted to make sure it wasn't entirely forgotten.

As for the code, I think your approach should work, you just need to get the details right. In your first comment you mentioned something about backward compatibility. Please ensure your solution is 100% backward compatible. The dunst project isn't moving that fast anymore (see #1117 for an update) and I want to make sure it doesn't break on new releases.

[fwsmit]

For organizational purposes I'm going to close this pull request. If you or anyone else wants to work on this, feel free to reopen this pull request or start a new one (referencing this PR).