c2: Perform matching against "all" property values with special index `[*]` by tryone144 · Pull Request #550 · yshui/picom

tryone144 · 2020-11-29T17:56:40Z

~~Change the default~~Add special index [*] in window-rule matching to check against any value if the property in question is an array ~~instead of only checking the first value if no index is provided~~. See the discussion in #512 for why this makes more sense:

opacity-rule = [ "0:_NET_WM_STATE@[*]:32a *= '_NET_WM_STATE_HIDDEN'" ];

instead of

opacity-rule = [
  "0:_NET_WM_STATE@[0]:32a *= '_NET_WM_STATE_HIDDEN'",
  "0:_NET_WM_STATE@[1]:32a *= '_NET_WM_STATE_HIDDEN'",
  "0:_NET_WM_STATE@[2]:32a *= '_NET_WM_STATE_HIDDEN'",
  "0:_NET_WM_STATE@[3]:32a *= '_NET_WM_STATE_HIDDEN'",
  "0:_NET_WM_STATE@[4]:32a *= '_NET_WM_STATE_HIDDEN'",
];

TODO

Remove hardcoded limit of 10 values

codecov · 2020-11-29T17:58:40Z

Codecov Report

Merging #550 (5989477) into next (fb38bf0) will increase coverage by 0.13%.
The diff coverage is 35.88%.

@@            Coverage Diff             @@
##             next     #550      +/-   ##
==========================================
+ Coverage   38.21%   38.35%   +0.13%     
==========================================
  Files          46       46              
  Lines        9056     9116      +60     
==========================================
+ Hits         3461     3496      +35     
- Misses       5595     5620      +25

Impacted Files	Coverage Δ
src/x.h	`84.21% <ø> (ø)`
src/c2.c	`36.56% <30.26%> (+1.23%)`	⬆️
src/x.c	`48.75% <83.33%> (+1.01%)`	⬆️

yshui · 2020-11-29T18:23:12Z

Thanks.

yshui · 2020-11-29T18:49:11Z

Should this behavior only apply when using *= ? I think it would make intuitive sense, because *= mean matches anywhere.

tryone144 · 2020-11-29T19:06:21Z

Regarding #549 (comment)

I want to avoid the 10 properties hard limit if we can.

I initially used the stack allocated array to reduce the number of memory allocations since matching might be performed quite often.
To be fair, we already duplicate the property-value strings when performing string matching, so it might not be that big of a deal after all.

Should this behavior only apply when using *= ? I think it would make intuitive sense, because *= mean matches anywhere.

Do we want to restrict = (exact), ^= (start), %= (wildcard), and ~= (pcre) to "first item if array"? Having different behaviour depending on the match type makes this less intuitive in my opinion.

yshui · 2020-11-29T19:09:27Z

Do we want to restrict = (exact), ^= (start), %= (wildcard), and ~= (pcre) to "first item if array"? Having different behaviour depending on the match type makes this less intuitive in my opinion.

Yeah, this is a headache. Or should we have another qualifier for matching arrays? Or maybe something like array[*] = ...?

tryone144 · 2020-11-29T19:21:37Z

Yeah, this is a headache. Or should we have another qualifier for matching arrays? Or maybe something like array[*] = ...?

Thought about that, but this would mean we can't rely on index in the match-leaf being initialized to -1 when parsing the rule.

Let me think about that again. We might be able to default to 0 (i.e. first entry, current behaviour) and set to -1 for array[] / array[*] / array[@] or the like to indicate match against any. This would just introduce a slight inconsistency when printing the rules (but I don't think we do that outside of debugging).

Do we even want to change the current behaviour of defaulting to the first entry (i.e. break backwards compatibility)?

yshui · 2020-11-29T19:25:05Z

This would just introduce a slight inconsistency when printing the rules

Do you mean that array = 'pattern' will be printed as array[0] = 'pattern'? I think that would be fine, maybe even preferable.

(i.e. break backwards compatibility)?

Yeah, that's one of my worries.

tryone144 · 2020-11-29T19:45:47Z

Do you mean that array = 'pattern' will be printed as array[0] = 'pattern'? I think that would be fine, maybe even preferable.

Yes.

I want to avoid the 10 properties hard limit if we can.

I initially used the stack allocated array to reduce the number of memory allocations since matching might be performed quite often.
To be fair, we already duplicate the property-value strings when performing string matching, so it might not be that big of a deal after all.

Forgot about x_get_prop_with_offset(). We have to specify how many 32-bit words / entries we want to read. The wintype update defaults to 32 32-bit words:

picom/src/win.c

Lines 628 to 630 in fb38bf0

    
           static wintype_t wid_get_prop_wintype(session_t *ps, xcb_window_t wid) { 
        
           	winprop_t prop = 
        
           	    x_get_prop(ps, wid, ps->atoms->a_NET_WM_WINDOW_TYPE, 32L, XCB_ATOM_ATOM, 32);

yshui · 2020-11-29T19:51:14Z

The wintype update defaults to 32 32-bit words:

You are right, looks like we should use NUM_WINTYPES here

tryone144 · 2020-11-29T22:06:40Z

You are right, looks like we should use NUM_WINTYPES here

I'll change that in #549.

To get rid of a hardcoded limit, we have to provide a way to get the property length similar to wid_get_text_prop(). Maybe when the supplied length is 0? x_get_prop_with_offset() is only called in the critical section.

I think array[*] is the best syntax to specify "match against any entry".

yshui · 2020-11-29T22:13:43Z

Maybe when the supplied length is 0? x_get_prop_with_offset() is only called in the critical section.

Just add a x_get_prop_length. I don't think putting length retrieval into the same function saves us anything.

yshui · 2020-11-30T20:12:47Z

 	// Parse index
 	if ('[' == pattern[offset]) {
+		if (pleaf->predef != C2_L_PUNDEFINED) {
+			c2_error("Predefined targets can't have index.");


wm_state is going to be a predefined target, and it can have index. are you planning to remove this check later?

wm_state doesn't currently support indexing. That doesn't mean I couldn't add the support for indices. I'm not sure how useful indexing would be, though.

I think it might be good for consistency. Because wm_state is indeed an array behind the sceen.

So we would inherit the same behaviour (no index: first, index *: all) as with the custom properties? I can change and document that in #549.

Yes, I think that would be good. (Also in that case maybe we don't need to cache the value of wm_state?) Do you have a feeling on this?

To be honest, I am more concerned about users confusing ...

Good point, that plus wm_state being just a shortcut without other added values (besides caching, but see my next comment), makes me wonder if we should just ask the user to use _NET_WM_STATE@[*]:32a = "_NET_WM_STATE_HIDDEN". User won't get wrong expectations from that rule.

And for the caching aspect, I think we could add generic caching for all properties.

… wm_state being just a shortcut without other added values …

Yeah, this was just meant as a compromise to make these rules "easier to read" (see #512 (comment)). I am fine with leaving those changes out.

I think we could add generic caching for all properties.

Let me see if this can be nicely done for arbitrary atom/string properties (and atom arrays) without adding too much overhead.

Caching can be done on-demand when properties are used, eviction can be done when we get property notify.

I'll take over #549 to implement general caching instead of the wm_state target.

yshui

LGTM

tryone144 · 2020-12-14T15:31:25Z

Since we decided against the wm_state target in #549, I'm going to update the documentation (i.e. manpage) with examples for the new [*]-syntax for hidden or sticky windows and merge this.

… `[*]` When matching against custom window properties or atoms perform the matching against all available values using logical OR if the special index `[*]` is specified. If no index is specified, we fall-back to the first value. This should help when an atom has multiple values and you only want to check against any of these — e.g. hiding windows with state `hidden`: `--opacity-rule "0:_NET_WM_STATE@[*]:32a *= 'HIDDEN'"` — without having to explicitly specify each index separately or when the index is not known in advance. Updated the manpage with examples for hidden and sticky windows.

tryone144 mentioned this pull request Nov 29, 2020

Cache _NET_WM_STATE property and add rule target wm_state #549

Open

1 task

x: split out function to get property info (type, format, size in bytes)

d96abca

tryone144 force-pushed the property-array-matching branch from 0e57e5e to 838e85a Compare November 30, 2020 15:26

tryone144 requested a review from yshui November 30, 2020 15:34

yshui reviewed Nov 30, 2020

View reviewed changes

Comment thread src/x.c

yshui reviewed Nov 30, 2020

View reviewed changes

yshui approved these changes Nov 30, 2020

View reviewed changes

tryone144 mentioned this pull request Dec 8, 2020

focus-exclude WM_PROTOCOLS containing WM_TAKE_FOCUS #560

Closed

tryone144 force-pushed the property-array-matching branch from 838e85a to 5f83983 Compare December 15, 2020 17:25

tryone144 force-pushed the property-array-matching branch from 5f83983 to 5989477 Compare December 15, 2020 21:32

tryone144 changed the title ~~c2: Perform matching against "all" property values if no index is specified~~ c2: Perform matching against "all" property values with special index [*] Dec 15, 2020

tryone144 merged commit 8802057 into yshui:next Dec 15, 2020

tryone144 deleted the property-array-matching branch December 15, 2020 21:53

tryone144 mentioned this pull request Dec 15, 2020

Feature request: add predefined targets for sticky and hidden #512

Closed

MarioJim mentioned this pull request Dec 17, 2020

Immediate crash on Arch Linux-- Segmentation fault (core dumped) #570

Closed

vincentbernat mentioned this pull request Jul 22, 2021

In tabbed mode, windows underneath transparent windows do not update i3/i3#2263

Closed

Uh oh!

Conversation

tryone144 commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

codecov Bot commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yshui commented Nov 29, 2020

Uh oh!

yshui commented Nov 29, 2020

Uh oh!

tryone144 commented Nov 29, 2020

Uh oh!

yshui commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tryone144 commented Nov 29, 2020

Uh oh!

yshui commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tryone144 commented Nov 29, 2020

Uh oh!

yshui commented Nov 29, 2020

Uh oh!

tryone144 commented Nov 29, 2020

Uh oh!

yshui commented Nov 29, 2020

Uh oh!

Uh oh!

yshui Nov 30, 2020 • 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!

tryone144 Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

yshui Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

tryone144 Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

yshui Dec 1, 2020 • 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!

yshui Dec 11, 2020 • 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!

yshui Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

tryone144 Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

yshui Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

tryone144 Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

yshui left a comment

Choose a reason for hiding this comment

Uh oh!

tryone144 commented Dec 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tryone144 commented Nov 29, 2020 •

edited

Loading

codecov Bot commented Nov 29, 2020 •

edited

Loading

yshui commented Nov 29, 2020 •

edited

Loading

yshui commented Nov 29, 2020 •

edited

Loading

yshui Nov 30, 2020 •

edited

Loading

yshui Dec 1, 2020 •

edited

Loading

yshui Dec 11, 2020 •

edited

Loading