Implement the current spec for event match conditions #6457
Conversation
This fixes that people randomly get pinged on every reply to a user names @roomba:server.tld. fixes element-hq#2541 Signed-off-by: Nicolas Werner <[email protected]>
|
(Still need to install java to be able to lint and the android SDK to be able to test this) |
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
Signed-off-by: Nicolas Werner <[email protected]>
|
Actually, thinking about it, not sure if I have time to set up a complete Android dev env soon, some feedback would be nice and maybe it can be merged without me manually testing it, if all the tests and lints pass? |
Signed-off-by: Nicolas Werner <[email protected]>
|
I think the sonarqube stuff failing is not my fault, is it? |
| regex.containsMatchIn(value) | ||
| } else { | ||
| val regex = Regex(pattern.simpleGlobToRegExp(), setOf(RegexOption.DOT_MATCHES_ALL, RegexOption.IGNORE_CASE)) |
There was a problem hiding this comment.
Shouldn't you keep the existing optimisation to remove * prefix/suffix? as per #5008
There was a problem hiding this comment.
So in theory we could, but in this branch we are not matching the content.body field, but other fields of the event, which usually don't contain a long text. If someone explicitly writes a rule to match *a* on the sender field, we could in theory match 255 characters. It would also need to be extended to not do a containsMatchIn, since if there was no * at the start of the string, we need to match the start of the string.
The important part here is in the following line, which does matches instead of containsMatchIn. This would need to change to:
if (pattern.startsWith("*") && pattern.endsWith("*")) {
val regex = Regex(pattern.removePrefix("*").removeSuffix("*").simpleGlobToRegExp(), RegexOption.DOT_MATCHES_ALL)
regex.containsMatchIn(value)
} else if (pattern.startsWith("*")) {
// match if value endsWith pattern
} else if (pattern.endsWith("*")) {
// match if value startsWith pattern
} else {
// match if complete value matches pattern
}
The last 3 branches could probably be collapsed just into one, but considering that fields apart from content.body are usually short, patterns usually never start AND end with a * and this would mostly affect the user themselves if they find a way to write such a rule that actually is slow to evaluate, I would say it is not worth it in this case.
Now on the other hand in the half that matches on content.body, we need to check for word boundaries at the start and end for most matches. Now I don't think this matters if we have a glob on both sides, so I think keeping the optimization can actually be useful there. But for context, the old optimization was because Element Android did *@room* to match the body, which this PR explicitly removes, since that matches the wrong stuff. So this only optimizes some badly written user rules and tbh probably should be implemented upstream in the regex engine... Anyway, I'll add the optimization back to the content.body branch.
There was a problem hiding this comment.
Added the optimization back in the body branch, not in the other branch since I don't really see a need for it, but if you think I should, I can do that as well.
| } | ||
| val regex = Regex(modPattern, RegexOption.DOT_MATCHES_ALL) | ||
| if (key == "content.body") { | ||
| val regex = Regex("(\\W|^)" + pattern.simpleGlobToRegExp() + "(\\W|$)", setOf(RegexOption.DOT_MATCHES_ALL, RegexOption.IGNORE_CASE)) |
There was a problem hiding this comment.
How different from caseInsensitiveFind is this? can it reuse it too?
There was a problem hiding this comment.
caseInsesitiveFind doesn't evaluate globs (since it escapes the string, from what I can tell), otherwise it is the same.
Signed-off-by: Nicolas Werner <[email protected]>
|
Jo |
|
Thank you! <3 |
This fixes that people randomly get pinged on every reply to a user
names @roomba:server.tld.
fixes #2541
Signed-off-by: Nicolas Werner [email protected]
Type of change
Content
Implements the spec from matrix-org/matrix-spec-proposals#3690 so that replying to users, whose name starts with @room, shouldn't ping you anymore.
Motivation and context
I complained about this issue a few years ago and it still isn't fixed and now exactly what I predicted happened: A user named @roomba joined my rooms and this is causing quite a few complaints from Element Android users. Since not replying to people is a hard thing to remember, I thought I would try fixing it instead!
Screenshots / GIFs
Tests
I actually didn't, since I don't have any Android device. I'll see if I can test it in the emulator on a different device later. But if you want to test it, just join #gentoo:matrix.org and see if you have thousands of room mentions, where someone replied to roomba.
Tested devices
Checklist