Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nick highlighting broken in MUCs #1578

Closed
kaffeekanne opened this issue Jul 14, 2021 · 7 comments
Closed

Nick highlighting broken in MUCs #1578

kaffeekanne opened this issue Jul 14, 2021 · 7 comments
Assignees
Labels
behaviour-change changes to current behaviour
Milestone

Comments

@kaffeekanne
Copy link
Contributor

kaffeekanne commented Jul 14, 2021

Up until recent changes the nick highlighting was pretty solid. Meaning if the nick was sourrounded by whitespaces or puntuation the highlighting was triggered. Since some commits, every occurance of the nick as substring of any word can trigger highlighting. Using short ambigious nicks leads to many false positive highlights.

Expected Behavior

A nick should only be highlighted if it is sourrounded by whitespace or punctuation (including brackets and quotation marks). Let the nick be kaffee, a message containing kaffeekanne should not trigger a highlighting.

Current Behavior

Let the nick be kaffee, a message containging the word kaffeekanne will be highlighted.

Possible Solution

Probably a regex that behaves more like the former one. Check how other Clients deal with this, Conversations and Gajim have pretty highlighting, with rare false positives.

Probably the current behaviour was intruduced by 6bc440c .

Steps to Reproduce (for bugs)

  1. Have a short or ambgious nickname
  2. Join a MUC
  3. Let that nickname be a substring of a (semi) long word
  4. Be highlighted without intent

Context

I get a lot of mentions and therefore unnecessary updates in console window and status window.

Environment

  • Give us the version and build information output generated by profanity -v
Profanity, version 0.10.0dev.master.4c8b6e9d
Copyright (C) 2012 - 2019 James Booth <boothj5web@gmail.com>.
Copyright (C) 2019 - 2021 Michael Vetter <jubalh@iodoru.org>.
License GPLv3+: GNU GPL version 3 or later <https://www.gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Build information:
XMPP library: libmesode
Desktop notification support: Disabled
OTR support: Disabled
PGP support: Enabled (libgpgme 1.15.1)
OMEMO support: Enabled
C plugins: Disabled
Python plugins: Disabled
GTK icons/clipboard: Disabled
  • Operating System/Distribution
OpenBSD 6.9
  • glib version
2.66.8
@kaffeekanne
Copy link
Contributor Author

From my MUC chat log:

public static Pattern generateNickHighlightPattern(final String nick) {
    return Pattern.compile("(?<=(^|\\s))" + Pattern.quote(nick) + "(?=\\s|$|\\p{Punct})");
}

this is what Conversations does. The nick surrounded by punctuation and white spaces. I am not aware of any flaws of this pattern matching. The way profanity did it before, was good too.

@jubalh jubalh added this to the 0.11.1 milestone Jul 17, 2021
@jubalh
Copy link
Member

jubalh commented Jul 17, 2021

I will take a look about the bug later.
Regex is already requested here: #369

@kaffeekanne
Copy link
Contributor Author

That's not exactly what i meant. I think the user will not come up with better regexes than the community. I just think Conversations has a pretty good pattern that can be taken as default in our codebase. If somebody comes up with a better regex, it should be merged in master and not stay hidden in some users local config file.

Nevertheless highlighting is broken right now in 11.0 and master.

@jubalh
Copy link
Member

jubalh commented Jul 19, 2021

That's not exactly what i meant. I think the user will not come up with better regexes than the community. I just think Conversations has a pretty good pattern that can be taken as default in our codebase. If somebody comes up with a better regex, it should be merged in master and not stay hidden in some users local config file.

The bug is about regexes. So that's exactly what you meant. Whether they are custom made from the user for no matter which word or whether we will add a default for the neck based on a regex is another topic.

Nevertheless highlighting is broken right now in 11.0 and master.

LIke I said: I will take a look about the bug later. so no need to repeat.

@jubalh jubalh self-assigned this Jul 29, 2021
@jubalh
Copy link
Member

jubalh commented Aug 20, 2021

Is it possible that this is actually only working correctly since 6bc440c ?

There is:
/notify room mention word_whole and /notify room mention word_part. What is your setting @kaffeekanne ? Probably word_part? If yes, it looks to me like it's works as expected?
I actually never looked at these settings before.

@jubalh
Copy link
Member

jubalh commented Aug 31, 2021

@kaffeekanne ping

I still think that the behaviour only works properly now. If you agree then I guess it makes sense to that we will set /notify room mention word_whole as default. I think that makes most sense.

jubalh added a commit that referenced this issue Sep 8, 2021
Set PREF_NOTIFY_MENTION_WHOLE_WORD to true.

If I'm not mistaken the _mucwin_print_mention() / get_mentions()
functions only work correctly since
6bc440c.

This changed the behaviour for users.
They got notified when their nick was `kaffee` and in the message the
string `kaffeekanne` occured.

Setting `/notify room mention word_whole` corrected this.

So my idea is that only now the mention function work correctly. And to
have a good default behaviour we should set the `word_whole` on by
default.

Regards #1578
@jubalh
Copy link
Member

jubalh commented Sep 8, 2021

Since no feedback I assumed my ideas were right and set word_whole as the default now.

@jubalh jubalh closed this as completed Sep 8, 2021
@jubalh jubalh added the behaviour-change changes to current behaviour label Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviour-change changes to current behaviour
Projects
None yet
Development

No branches or pull requests

2 participants