Added dunst xresources theme wrapper #525

lulivi · 2018-06-28T11:02:02Z

Hello.

I've been reading some discussions about allowing Command Substitution $(...) and X resources access from the configuration file (#357).

Then, I headed to the dunst channel in freenode and nikos came up with the idea of creating a script to solve this problem.

So I created a wrapper to change colors and some other attributes according to the Xresources theme. Any tips or improvements?

bebehei · 2018-06-28T11:06:07Z

Any tips or improvements?

First glance:

Run shellcheck over it and quote everything.
AFAIK declare -A isn't POSIX, so the shebang doesn't match this properly. (have to research it).

Code looks quite good for the rest.

lulivi · 2018-06-28T11:08:24Z

Ok! I'm on it.

lulivi · 2018-06-28T11:57:34Z

@bebehei I've been searching and the simplest idea I have right now is to create a function with a case from this answer something like this:

theme_vars () {
    case "$1" in
        "key1") printf '%s' "value1";;
        "key2") printf '%s' "value2";;
        ....
    esac
}

printf 'dict[key1]=%s' "$(theme_vars 'key1')"

I've searched for sh POSIX standard and declare appears there (don't know it that means anything). It doesn't talk about arrays or associative arrays so... I don't know.

EDIT: Other option is to change the shebang to #!/usr/bin/env/ bash and forget about POSIX.

EDIT1: Another obstacle, there are no arrays in POSIX, so goodbye to my line-by-line file access.

EDIT2: Fixed misspelling.

tsipinakis

Another obstacle, there are no arrays in POSIX, so goodbye to my line-by-line file access.

Arrays give a huge advantage in this so with that we're probably never going to get 100% POSIX compliant. In this case I don't think it matters anyway it runs on bash which covers 99% of the market and for others they can always run it in a bash subshell.

I found a few nitpicks but I have not written more than a few toy shell scripts so I am not really that qualified to review this. I'd be interested to know if @bebehei has any further input.

tsipinakis · 2018-06-28T14:45:21Z

contrib/dunst_xr_theme_changer.sh

+def_conf=$def_conf_dir/dunstrc
+
+# User config dir and file
+user_conf_dir=$XDG_CONFIG_HOME/dunst


$XDG_CONFIG_HOME should default to $HOME/.config if unset.

tsipinakis · 2018-06-28T15:01:25Z

contrib/dunst_xr_theme_changer.sh

+# configuration to $user_themed_conf file.
+#
+###############################################################################
+


Using set -e would probably be wise in such scripts to catch any errors.

tsipinakis · 2018-06-28T15:04:22Z

contrib/dunst_xr_theme_changer.sh

+[ -f $user_conf ] || {
+    printf '%s does not exist.\n.' "$user_conf"
+    printf 'Copying default config to %s...\n' "$user_conf_dir"
+    cp $def_conf $user_conf_dir


Should ensure that $def_conf exists in the first place, not every distro has the default dunstrc in /usr/share/dunst/dunstrc, in Debian for example it's in /usr/share/doc/dunstrc.gz

Is there any other possible default directories to include in the search? Or should I just ask the user to change $def_conf_dir path?

I did some research and realized about the following:

Distribution Directory file

Archlinux /usr/share/dunst dunstrc

Debian < 1.3.0-2 / Ubuntu < 18.04 /usr/share/doc/dunst/ dunstrc.example.gz

Debian >= 1.3.0-2 / Ubuntu >= 18.04 /usr/share/doc/dunst/ dunstrc.gz

tsipinakis · 2018-06-28T15:08:56Z

contrib/dunst_xr_theme_changer.sh

+
+# Regular expressions
+re_section_line='^\[(.*)\]$'
+re_empty_comment_line='(^$)|(^[[:space:]]*\#)'


Semicolons can also be used as comments.

tsipinakis · 2018-06-28T15:14:08Z

contrib/dunst_xr_theme_changer.sh

+# Regular expressions
+re_section_line='^\[(.*)\]$'
+re_empty_comment_line='(^$)|(^[[:space:]]*\#)'
+re_attribute_line='^[[:space:]]{4}([_[:alnum:]]+)'


Why just 4 spaces? Let's support all indentation types. Something like ^(?:[[:space:]]+)?([_[:alnum:]]+) should do the trick.

tsipinakis · 2018-06-28T15:22:21Z

contrib/dunst_xr_theme_changer.sh

+    # Current line
+    curr_line="${conf[$idx]}"
+    # If we are in a new section:
+    [[ "$curr_line" =~ $re_section_line ]] && {


Why use these weird contraptions from logical operators rather than the usual if/fi pair? They are confusing at first glance.

tsipinakis · 2018-06-28T15:27:42Z

contrib/dunst_xr_theme_changer.sh

@@ -0,0 +1,115 @@
+#!/usr/bin/env sh


$ sh ./dunst_xr_theme_changer.sh ./dunst_xr_theme_changer.sh: 56: ./dunst_xr_theme_changer.sh: mapfile: not found ./dunst_xr_theme_changer.sh: 63: ./dunst_xr_theme_changer.sh: Syntax error: "(" unexpected

It should probably use bash rather than sh.

lulivi

Oops, variable expansion...

bebehei

High quality in code! I only found some small nitpicks. I love the changes and look forward to merge them.

Thanks for heavily using printf. I never knew, that echo is such a bad command.

Could you please squash your commits, too?

bebehei · 2018-07-01T13:43:58Z

contrib/dunst_xr_theme_changer.sh

+        example_conf="$example_conf_dir/dunstrc"
+    else
+        printf 'Could not find the example config file in %s.
+               \nPlease, change \$example_conf variable in the script' "$example_conf_dir"


As you're using single quotes in the format string, you'll print \$example_conf and not solely $example_conf.

bebehei · 2018-07-01T13:47:03Z

contrib/dunst_xr_theme_changer.sh

+        example_conf="$example_conf_dir/dunstrc.gz"
+    else
+        printf 'Could not find the example config file in %s.
+               \nPlease, change \$example_conf variable in the script' "$example_conf_dir"


As you're using single quotes in the format string, you'll print \$example_conf and not solely $example_conf.

bebehei · 2018-07-01T13:47:46Z

contrib/dunst_xr_theme_changer.sh

+
+else
+    printf 'Could not find the example config directory.
+           \nPlease, change \$example_conf_dir variable in the script'


As you're using single quotes in the format string, you'll print \$example_conf and not solely $example_conf.

bebehei · 2018-07-01T13:49:05Z

contrib/dunst_xr_theme_changer.sh

+
+set -e
+
+script_name="$(basename $0)"


bebehei · 2018-07-01T13:49:17Z

contrib/dunst_xr_theme_changer.sh

+
+# Function to get resource values
+xrdb_get () {
+    output=$(xrdb -q | grep -e "$1" | cut -f2)


Quotes for $(...)

bebehei · 2018-07-01T13:52:04Z

contrib/dunst_xr_theme_changer.sh

+
+# Check if the user config directory exists
+if ! [ -d "$user_conf_dir" ]; then
+    printf '%s\n' "$user_conf_dir folder doesn't exist, creating..."


printf 'Creating folder "%s".\n' "$user_conf_dir"

bebehei · 2018-07-01T13:54:47Z

contrib/dunst_xr_theme_changer.sh

+# After everything is completed, write the new config to a file
+user_xr_color_conf_content+="$(printf '%s\n' "${conf[@]}")"
+
+printf '%s\n' "$user_xr_color_conf_content" > $user_xr_color_conf


Quotes for the steamname variable? > "$user_xr_color_conf"

bebehei · 2018-07-01T14:47:46Z

~~Oh, let's put a logic test on the script:~~

~~You haven't installed dunst into /usr/bin, but you already have a working configuration file. Will the script work?~~

Edit: Covered.

lulivi · 2018-07-01T17:36:20Z

Thank you both @bebehei and @tsipinakis for your nitpicks! I think the code is cleaner now.

Any other change you would like to make to the script?

tsipinakis · 2018-07-01T17:42:16Z

contrib/dunst_xr_theme_changer.sh

+
+        if [ -f "$example_conf_dir/dunstrc.example.gz" ]; then
+            # Ubuntu <= 17.10 and Debian <= 1.2.0-2 example file:
+            example_conf="$example_conf_dir/dunstrc.example.gz"


This won't work, the file in ubuntu and debian is compressed with gzip you'll need to uncompress it to use it.

Completely forgot 😅.

bebehei · 2018-07-01T23:04:46Z

Any other change you would like to make to the script?

The code quality is close to bash perfectness. But the actual point, if the script works is completely unclear to me. I admit, I haven't executed the script and I'm too lazy to do it.

I'd like to ask some guys from #357 to test it.

@lulivi You squashed the commit. Thanks. But you also squashed the commit messages. And most of them are invalid now. e.g: "Quoted $0". There is no need to quote $0 as you already did it right in the commit ;-) It would be great, if you would remove the junk messages.

nhatzHK · 2018-07-02T03:42:06Z

I tested the script. It seems to be working fairly well, except for 2 things:

Line 88
When executed, it crashes on this line with:
./dunst_xr_theme_changer.sh: line 88: conditional binary operator expected

If I comment the whole if/else block out, it runs without error.
The generated file
There seem to be some mistakes when putting the colours in the generated file.
```
[urgency_critical]
    background = "#0d170a
#0d170a"
    foreground = "#9E7263
#9E7263"
    frame_color = "#9E7263
#9E7263"
```
The generated file has the right colours, but the string is terminated on a new line, and the colour is repeated in it.

I would suggest to put the colours in variables at the top of the config file, but dunst probably doesn't support variables, and then the user could reuse these variables in the config file. i3wm does the same thing with set.

set $var "value" # define and set the variable's value
...
...
client.urgent $var # use it to configure other stuff later on

Even though this script is going to work pretty well, I think you guys should check out how i3wm lets the user access the X ressource database in the config file.

Anyway this is awesome apart from the little things I pointed out.
👍

tadly · 2018-07-02T07:56:33Z

contrib/dunst_xr_theme_changer.sh

+###############################################################################
+#
+# This script creates a dunst user configuration file in
+# $XDG_CONFIG_HOME/dunst/ folder, changing dunst basic teeming options


teeming -> theming (I assume)
Just because I noticed it :)

lulivi · 2018-07-02T08:06:18Z

Thank you @nhatzHK for your review!

I'll check that if test later.
I think the problem with duplicating the color has to do with this line. It is one of the last changes I made and maybe didn't test it.
Thanks @tadly for noticing the misspelling!

I have a test tomorrow, so I will start with the debug after it.

EDIT: The first problem is solved when changing ~= with ==. Forgot I was comparing strings.

There is another problem. if the example file is called dunstrc.example.gz, the user file will be dunstrc.example. Trying to correct that too.

EDIT 1: I was wrong with the new problem. I just decompress /usr/share/doc/dunst/dunstrc.[example].gz to ~/.config/dunst/dunstrc

lulivi · 2018-07-02T19:07:12Z

@nhatzHK I've tried the final script, and it does not duplicate the colors for me...

~/.config/dunst/dunstrc

[urgency_normal]
    background = "#285577"
    foreground = "#ffffff"
    frame_color = "#00ff00"
    timeout = 5
    # Icon for notifications with normal urgency, uncomment to enable
    #icon = /path/to/icon

~/.config/dunst/dunstrc_xr_colors

[urgency_normal]
    background = "#2b303b"
    foreground = "#a3be8c"
    frame_color = "#a3be8c"
    timeout = 5
    # Icon for notifications with normal urgency, uncomment to enable
    #icon = /path/to/icon

Don't know why is working bad for you.

EDIT: @bebehei what do you think about the squashed commits now? I tried to keep the important ones.

bebehei · 2018-07-02T21:10:52Z

EDIT: @bebehei what do you think about the squashed commits now? I tried to keep the important ones.

I strongly recommend https://chris.beams.io/posts/git-commit/ There is no better thing. @tsipinakis, right? 😉

A good commit message could be:

Add xresources config wrapper

bebehei · 2018-07-02T21:18:55Z

contrib/dunst_xr_theme_changer.sh

+
+# Function to get resource values
+xrdb_get () {
+    output="$(xrdb -q | grep -e "$1" | cut -f2)"


The generated file has the right colours, but the string is terminated on a new line, and the colour is repeated in it.

I think here is the problem. You have to make sure, that only a single line is retrieved. Currently you grep for anything matching the string $1. It could be even the value, which matches.. Usually Xresources have *color0: #abcdef, but you also could define XTerm*color0: #123456. And your regex would match both.

As @nhatzHK has already mentioned: You should use another method to query the X resource database, which also takes care of the precedence of specific values.

@nhatzHK Just to verify my thesis: What does xrdb -q | grep 'color0:' give as the output?

I think this is right. I do have several lines ending with colorX in my X ressource database.

$ xrdb -q | grep 'color0:' *.color0: #0d170a ## notice the leading dot *color0: #0d170a

The thing is: should I just get the first line?

e.g.: If you have

XTerm*color0: #123456 *.color0: #0d170a *color0: #0d170a

then with head -n1 will get the XTerm color:

$ xrdb -q | grep "color0:" | head -n1 | cut -f 2 #123456

So maybe it is interesting to place colors at the begining or at the end. Other option is to write the exact xresource name:

$ xrdb -q | grep "*.color0:" | head -n1 | cut -f 2 #0d170a

But maybe this option will get the same problem that we have before, something like

XTerm*.color0: #123456 *.color0: #0d170a

What do you think?

How about we throw more regex at it?

^(?:\*?\.?[a-zA-Z0-9]+):[[:space:]]?(.*)$

or

^(?:\*?\.?color0):[[:space:]]?(.*)$

to catch wildcards and

^(?:Dunst\*?\.?[a-zA-Z0-9]+):[[:space:]]?(.*)$

or

^(?:Dunst\*?\.?color0):[[:space:]]?(.*)$

for dunst specifically.

Hey guys, if I read the docs right, the key should be the appres program. No regex involved.

Hum... Can't figure out how to make it work :(

This should be as simple as replacing xrdb -q with appres Dunt and then using | head -n1 as you suggested. Some testing shows that it always puts the more specific match at the top.

Thanks Nikos! I was trying something like Dunst.color0 and wasn't working. The script is fixed now.

lulivi · 2018-07-03T11:43:44Z

I strongly recommend https://chris.beams.io/posts/git-commit/ There is no better thing. @tsipinakis, right? 😉
A good commit message could be:
Add xresources config wrapper

Oh sorry, I understood you wanted me to keep relevant commits. So maybe I should follow that commit structure?

Add xresources config wrapper

Created a wrapper script to customize theme variables (colors,
fonts, etc) according with your X resources file (~/.Xresources).

As dunst is not a X exclusive notification daemon, this script
could be a workarround to solve this.

Related with: #357

What do you think? Or should I just keep it in oneline as you suggested?

tsipinakis

What do you think? Or should I just keep it in oneline as you suggested?

This looks perfect, commit messages are generally the individual developers choice and that guide should be read as a general guideline, not a rule.

The only other nitpick I'd mention is maybe have theme_attr_dict at the top, under the config variables, so that everything "user editable" is in one place and one doesn't have to dig through the script to find them.

Other than that unless @bebehei has any other comments I'd say this is ready to be merged - awesome work!

lulivi · 2018-07-10T08:39:33Z

The only other nitpick I'd mention is maybe have theme_attr_dict at the top, under the config variables, so that everything "user editable" is in one place and one doesn't have to dig through the script to find them.

That's so true. I'm on it!

Edit: Also, I think I'm defining every non changing variable as readonly.

bebehei · 2018-07-10T08:52:10Z

The changes and the script looks good.

I'm putting myself in the role of a user, who just wants to use this wrapper and has no clue about the usage of it:

How do I seed the X resource database to pass values to dunst?
How do I run this wrapper?

IMO, these questions should get answered in the header. I think that'll make the difference between "there is something usable but nobody uses it" and "there is something good and everyone uses it".

lulivi · 2018-07-10T09:08:29Z

All right.

Maybe I could do a more extended header. Something like:

###############################################################################
##
## Usage
##
##     ./dunst_xr_theme_changer.sh
##
##     If it doesn't execute, give execute permissions to de file with 
##     chmod blabla...
##     And then run ./dunst_xr_theme_changer.sh blab...
##
## Description
##
##     This script blablabla...
##
## Theming
##
##     To theme colors and fonts, you have to change the variables in 
##     theme_attr_dict blabla...
##     Variables are obtained from Xresources, which are defined in 
##     ~/.Xresources this way blablabla...
##
###############################################################################

lulivi · 2018-07-11T16:50:55Z

Hey guys, I've made some improvements in the header (you can check the extended text and tell me what do you think) and created 2 functions to output :

extended help - when -h or --help are the script options.
usage help - when the command line option is wrong.

EDIT: Maybe I can simplify the script by removing the help and usage functions and leaving the extended header without -h or --help script options.

bebehei · 2018-07-11T22:55:59Z

I play the nerd card first: What command line prompt do you have?

If you add them, I don't have any objections and I'm glad to add this to dunst!

The only question, which now came to my mind: How will this script come the user's machine. Is this a maintainer's job to put that file manually somewhere in the package or is this our job?

lulivi · 2018-07-11T23:31:30Z

I play the nerd card first: What command line prompt do you have?

The prompt is a modified version of the bash_git_prompt for fish check it out here.

How will this script come the user's machine. Is this a maintainer's job to put that file manually somewhere in the package or is this our job?

Hummm... 🤔 Thats a really good question. This script is clearly an external part of dunst. It is not as integrated as dunsk_espeak.sh. I would suggest maybe to show it as a wrapper script you can use to make a connection between Xresources and dunst. So ai think is our job as developers/users to grab the script and run it. Maybe it can be delivered as the default dunstrc file, and only grabbed if needed. Sorry for my english, sometimes its pretty bad.

tsipinakis · 2018-07-12T07:15:08Z

How will this script come the user's machine. Is this a maintainer's job to put that file manually somewhere in the package or is this our job?

I can only speak for debian since that's what I'm familiar with: contrib directories and similar utility scripts are usually put along with documentation in /usr/share/doc/<package name>

bebehei · 2018-07-12T07:58:36Z

@tsipinakis Thanks. I guess my question is answered positively!

@lulivi please add the docs, you've pasted and - if @tsipinakis has no further wishes - I'm glad to merge it!

tsipinakis · 2018-07-13T14:02:16Z

contrib/dunst_xr_theme_changer.sh

+##     scape " characters for proper functioning of dunst config reader (for
+##     example "#ffeegg").
+##
+##     You can define X_res variables in $HOME/.Xresources file. For in depht


depht typo

Created a wrapper script to customize theme variables (colors, fonts, etc) according with your X resources file (~/.Xresources). As dunst is not a X exclusive notification daemon, this script could be a workarround to solve this. Related with: dunst-project#357

tsipinakis · 2018-07-15T09:03:06Z

Thanks again for the work put into this @lulivi.

tsipinakis requested changes Jun 28, 2018

View reviewed changes

lulivi commented Jun 28, 2018

View reviewed changes

lulivi force-pushed the master branch from 28d8df7 to d919aa5 Compare June 28, 2018 17:31

bebehei reviewed Jul 1, 2018

View reviewed changes

lulivi force-pushed the master branch from a3b25c4 to 883d7c1 Compare July 1, 2018 17:32

tsipinakis reviewed Jul 1, 2018

View reviewed changes

lulivi force-pushed the master branch from 883d7c1 to 24aa046 Compare July 1, 2018 19:24

bebehei mentioned this pull request Jul 1, 2018

Xresource colors #357

Closed

tadly reviewed Jul 2, 2018

View reviewed changes

lulivi force-pushed the master branch from 24aa046 to 6aca781 Compare July 2, 2018 18:59

bebehei reviewed Jul 2, 2018

View reviewed changes

lulivi force-pushed the master branch 2 times, most recently from d08615d to 52bd9c0 Compare July 9, 2018 17:14

tsipinakis previously approved these changes Jul 10, 2018

View reviewed changes

lulivi dismissed tsipinakis’s stale review via 07ca494 July 10, 2018 08:50

lulivi force-pushed the master branch from 52bd9c0 to 07ca494 Compare July 10, 2018 08:50

lulivi force-pushed the master branch from 07ca494 to 92b8e9e Compare July 12, 2018 13:39

tsipinakis reviewed Jul 13, 2018

View reviewed changes

lulivi force-pushed the master branch from 92b8e9e to cbfe8c0 Compare July 14, 2018 22:14

tsipinakis merged commit 0e1b5a1 into dunst-project:master Jul 15, 2018

Distribution	Directory	file
Archlinux	`/usr/share/dunst`	`dunstrc`
Debian < 1.3.0-2 / Ubuntu < 18.04	`/usr/share/doc/dunst/`	`dunstrc.example.gz`
Debian >= 1.3.0-2 / Ubuntu >= 18.04	`/usr/share/doc/dunst/`	`dunstrc.gz`

Added dunst xresources theme wrapper #525

Added dunst xresources theme wrapper #525

Conversation

lulivi commented Jun 28, 2018

bebehei commented Jun 28, 2018

lulivi commented Jun 28, 2018

lulivi commented Jun 28, 2018 • edited Loading

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

lulivi Jun 28, 2018 • edited Loading

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

lulivi left a comment

Choose a reason for hiding this comment

bebehei 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

Choose a reason for hiding this comment

bebehei commented Jul 1, 2018 • edited Loading

lulivi commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bebehei commented Jul 1, 2018

nhatzHK commented Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

lulivi commented Jul 2, 2018 • edited Loading

lulivi commented Jul 2, 2018 • edited Loading

bebehei commented Jul 2, 2018

Choose a reason for hiding this comment

nhatzHK Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsipinakis Jul 3, 2018 • edited Loading

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

lulivi commented Jul 3, 2018

tsipinakis left a comment • edited Loading

Choose a reason for hiding this comment

lulivi commented Jul 10, 2018 • edited Loading

bebehei commented Jul 10, 2018

lulivi commented Jul 10, 2018

lulivi commented Jul 11, 2018 • edited Loading

bebehei commented Jul 11, 2018

lulivi commented Jul 11, 2018

tsipinakis commented Jul 12, 2018 • edited Loading

bebehei commented Jul 12, 2018

Choose a reason for hiding this comment

tsipinakis commented Jul 15, 2018

lulivi commented Jun 28, 2018 •

edited

Loading

lulivi Jun 28, 2018 •

edited

Loading

bebehei commented Jul 1, 2018 •

edited

Loading

nhatzHK commented Jul 2, 2018 •

edited

Loading

lulivi commented Jul 2, 2018 •

edited

Loading

lulivi commented Jul 2, 2018 •

edited

Loading

nhatzHK Jul 2, 2018 •

edited

Loading

tsipinakis Jul 3, 2018 •

edited

Loading

tsipinakis left a comment •

edited

Loading

lulivi commented Jul 10, 2018 •

edited

Loading

lulivi commented Jul 11, 2018 •

edited

Loading

tsipinakis commented Jul 12, 2018 •

edited

Loading