Criteria #43

vilhalmer · 2018-06-11T00:50:53Z

At long last, criteria are here! The syntax is as discussed on #3, and is fairly flexible:

font=SF Mono 9
background-color=#ffffffff
text-color=#000000ff
margin=40,30

[hidden]
font=monospace 11

[urgency=normal app-name="Test\ App"\\\" actionable=true]
font=monospace 11
text-color=#ff0000ff
background-color=#00000000
margin=10

[!actionable]
margin=0
height=500
background-color=#00ff00ff
markup=0

Note that booleans can be specified either as normal, or as bare words (with leading ! to negate). Quoting, escaping, and escaping within quotes works, the literal name of the app in the above example is Test App\". The backslash within the quotes is unnecessary but doesn't hurt anything. Leaving quotes unmatched or backslashes trailing will raise errors. (I bashed on it a lot, but more testing of the parsing is much appreciated!)

Style options within the global section are stored in an empty criteria (which therefore matches everything). It is always the first in the list, and always initialized with the default style options before parsing the configuration.

Things that can currently be overridden:

colors
margin.{top,bottom} (I have no idea why you would want this.)
padding
border_size
font
format
default_timeout
ignore_timeout
markup

Things that don't work yet:

width (Currently forced to the surface width by render.)
height (I think this is just being completely ignored at the moment?)
margin.{left,right} (Need to rework how the surface size is calculated.)
actions (You can set it and it's stored, but turning them off doesn't disable them. See also other caveats below.)

Other caveats:

I don't know if markup and actions are going to end up being useful. Their enabled status gets sent as part of the XDG bus setup, which means they can only really be either on or off globally.
- Disabling markup "works" and you see the raw HTML, but since I don't think anyone would ever want this, it might be better to just make it global again. (That way if it's disabled, apps just send plaintext instead.)
- Disabling actions seems slightly more useful. We would want to advertise it as enabled globally if any criteria has it enabled, and then just ignore interactions with notifications where it has been disabled.
The [hidden] section is now fully style-able. However, I'm not happy with the current implementation. I'm going to continue working on a way to make it less of a special case.
Because styles are generated for each notification at creation, reloading the config will not update the style of existing notifications. It will, however, update the fake notification that shows the number hidden. I have a few ideas about fixing this, but I'm saving it for later.

Closes #3.

- Style options also mark the style's spec - hidden section is currently broken

This means that if the global criteria disables a feature, later critiera cannot reenable it. I'll deal with this later.

This definitely can't stay around either, but good enough for now.

whoops

Also add appropriate error messages

I want to refactor this eventually, but I don't know exactly how yet.

ascent12 · 2018-06-11T01:24:16Z

criteria.c

+		*value = '\0';
+		++value;
+	}
+	else {


Style: else on same line as }.
This also happens in many other places.

emersion · 2018-06-11T22:00:02Z

config.c

 void finish_style(struct mako_style *style) {
 	free(style->font);
 	free(style->format);
 }

+// Update `target` with the values specified in `style`. If a failure occurs,
+// `target` will remain unchanged.
+bool apply_style(struct mako_style *style, struct mako_style *target) {


Generally in C we prefer to have the struct on which the function operates always as the first argument. It seems the comment "target will remain unchanged" is outdated.

Also to make it more obvious that an argument isn't mutated, you can make it const.

emersion · 2018-06-11T22:01:45Z

criteria.c

+	return true;
+}
+
+bool parse_boolean(const char *string, bool *out) {


This could probably be reused in config.c too

I was planning on moving this and the existing stuff into a parse.c utility-type file in another PR, which would include making config.c use it. config.c is getting a little bit out of control.

Yeah, nice idea. So let's keep it that way for now.

criteria.c

+			case '"':
+				state = MAKO_PARSE_STATE_NORMAL;
+				break;
+			case ' ':


emersion · 2018-06-11T22:07:26Z

criteria.c

+	// string, so that's a safe length to use for the buffer.
+	int token_max_length = strlen(string) + 1;
+	char token[token_max_length];
+	memset(token, 0, token_max_length);


Instead of calling memset, you can initialize token with {0} and all fields will be set to zero.

You can't use initialization syntax with a variable-length array. :(

Oh, I didn't know that. RIP

emersion · 2018-06-11T22:07:45Z

criteria.c

+	int token_max_length = strlen(string) + 1;
+	char token[token_max_length];
+	memset(token, 0, token_max_length);
+	int token_location = 0;


This can be a size_t

emersion · 2018-06-11T22:11:43Z

criteria.c

+// Iterate through `criteria_list`, applying the style from each matching
+// criteria to `notif`. Returns the number of criteria that matched, or -1 if
+// a failure occurs.
+int apply_each_criteria(struct wl_list *criteria_list,


This can return a ssize_t

emersion

Excellent work. I wish I could read PRs like this everyday :)

Apart from these - mostly style and nitpick - issues, I'll test this patch out and it'll probably be ready to merge!

vilhalmer · 2018-06-12T01:20:36Z

Noting this here mostly to hold myself to it: I'm working on updating the manpage to go with this. I can either tack that on to this PR or post another one soon.

Also make the source style const

emersion · 2018-06-12T21:39:39Z

Noting this here mostly to hold myself to it: I'm working on updating the manpage to go with this. I can either tack that on to this PR or post another one soon.

Oh, right, I think this should be done in this PR, so that the manpage is not out-of-date.

emersion · 2018-06-12T21:49:11Z

Disabling markup "works" and you see the raw HTML, but since I don't think anyone would ever want this, it might be better to just make it global again.

We could strip HTML tags for instance. But yeah, it's not ideal.

Because styles are generated for each notification at creation, reloading the config will not update the style of existing notifications.

Agreed, this can be filled as a follow-up issue for later.

emersion · 2018-06-13T07:37:36Z

mako.1.scd

+- _urgency_ (one of "low", "normal", "high")
+- _category_ (string)
+- _desktop-entry_ (string)
+- _actionable_ (boolean)


This is missing "hidden"

Good catch.

emersion

Tried it, LGTM! :D

emersion · 2018-06-13T21:17:42Z

Thanks, nice work!

vilhalmer added 30 commits June 8, 2018 19:50

Add criteria

1fe0e3b

First pass at criteria string parser

ab93887

Add missing mako_style_spec.ignore_timeout

7182f0e

create_criteria takes a config instead of state

5a59247

apply_criteria_field consumes the token

2f8c982

criteria are stored in a list on mako_config

723593d

Split apply_style_option from apply_config_option

1df70a5

- Style options also mark the style's spec - hidden section is currently broken

Clean up criteria when destroying config

a3a93b4

Apply style from matched criteria to notifications

6834e42

hidden_format -> hidden_style

028c6f6

Use notification's style in render

585479a

Use global criteria when sending capabilities

53e7bb1

This means that if the global criteria disables a feature, later critiera cannot reenable it. I'll deal with this later.

Use global criteria to set surface size

abfa95c

This definitely can't stay around either, but good enough for now.

Only try to look at the section if it exists

5f2e541

Only try config options if it wasn't a valid style

5a0fa33

Actually init the default style

5ea01cd

whoops

Apply the final criteria field after loop exits

b04b73e

Properly calculate inner margin again

97a7d71

Print error for invalid criteria

e6e0765

Don't make a criteria for hidden

29fc3c6

Fix reporting of criteria parsing success

bbe53be

Also add appropriate error messages

Finish up error message TODOs in criteria

22878fc

hidden TODO is already done

db75066

Saving this for a future PR

f71b620

This happens automatically, though could be nicer

d676406

Missed converting padding

8a13aad

Fix up hidden_style correctly

8486d19

I want to refactor this eventually, but I don't know exactly how yet.

Plug a few leaks

36ef70c

Fix double-free in hidden_style when reloading

6119476

Don't try to parse empty criteria field

fa7f9f8

vilhalmer mentioned this pull request Jun 11, 2018

[WIP] Per-notification style #34

Closed

22 tasks

ascent12 reviewed Jun 11, 2018

View reviewed changes

emersion reviewed Jun 11, 2018

View reviewed changes

criteria.c

case '"':

state = MAKO_PARSE_STATE_NORMAL;

break;

case ' ':

This comment was marked as resolved.

Sign in to view

emersion reviewed Jun 11, 2018

View reviewed changes

vilhalmer added 5 commits June 11, 2018 21:28

Squish up all those elses

ae22b95

Combine unnecessary case

0840c93

token_location is a size_t

fa26e3b

apply_each_criteria returns ssize_t

172cb7d

Flip order of arguments to apply_style

93264c8

Also make the source style const

Update man page

9c7fe47

emersion reviewed Jun 13, 2018

View reviewed changes

emersion approved these changes Jun 13, 2018

View reviewed changes

Add hidden back to man page

9d89ba2

emersion merged commit 4409c6a into emersion:master Jun 13, 2018

vilhalmer deleted the criteria branch June 13, 2018 21:22

emersion mentioned this pull request Jun 14, 2018

Reloading the config doesn't update current notifications style #45

Closed

This was referenced Jun 14, 2018

Width and height cannot be overridden by criteria #46

Closed

Disabling actions with criteria has no effect #47

Closed

Horizontal margins cannot be overridden in criteria #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Criteria #43

Criteria #43

vilhalmer commented Jun 11, 2018 •

edited

Loading

ascent12 Jun 11, 2018

emersion Jun 11, 2018

emersion Jun 11, 2018

vilhalmer Jun 12, 2018

emersion Jun 12, 2018

This comment was marked as resolved.

emersion Jun 11, 2018

vilhalmer Jun 12, 2018

emersion Jun 12, 2018

emersion Jun 11, 2018

emersion Jun 11, 2018

emersion left a comment

vilhalmer commented Jun 12, 2018

emersion commented Jun 12, 2018

emersion commented Jun 12, 2018

emersion Jun 13, 2018

vilhalmer Jun 13, 2018

emersion left a comment

emersion commented Jun 13, 2018

Criteria #43

Criteria #43

Conversation

vilhalmer commented Jun 11, 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

This comment was marked as resolved.

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

emersion left a comment

Choose a reason for hiding this comment

vilhalmer commented Jun 12, 2018

emersion commented Jun 12, 2018

emersion commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersion left a comment

Choose a reason for hiding this comment

emersion commented Jun 13, 2018

vilhalmer commented Jun 11, 2018 •

edited

Loading