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

notify webxdc events, replies and reactions to own messages even if chat is muted #3456

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

adbenitez
Copy link
Member

@adbenitez adbenitez commented Nov 29, 2024

even if group is muted:

  • notify quote-reply to own messages
  • notify reactions to own messages
  • notify webxdc notifications

what is still unsolved:

if webxdc sends notification while the chat is open, there is no notification, if there is no info message there is no hint that there was a notification at all, and if there is info message that doesn't warranties that the notification was the same

@adbenitez adbenitez requested review from r10s and Hocuri November 29, 2024 19:16
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@r10s
Copy link
Member

r10s commented Dec 2, 2024

remove unnecessary thread

just wanted to comment on that :)

Copy link

github-actions bot commented Dec 2, 2024

To test the changes in this pull request, install this apk:
📦 app-preview.apk

final String name = JsonUtils.optString(info, "name");
String shortLine = name.isEmpty()? text : (name + ": " + text);
maybeAddNotification(accountId, dcContext.getChat(dcMsg.getChatId()), msgId, shortLine, shortLine, false);
DcChat dcChat = dcContext.getChat(dcMsg.getChatId());
maybeAddNotification(accountId, dcChat, msgId, shortLine, shortLine, false, dcChat.isMultiUser());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is that "mentioning" supposed to work?

if i understand the code correctly, that would result in any webxdc-notification to be treated as "mention" and bypass group-muting

Copy link
Member Author

@adbenitez adbenitez Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was the initial idea, but that was before "*" came in, oth, notifying all even if group is muted is still nice, and is similar to what happen in a muted group when someone uses the @all mention

and I can already see some similar use-cases like in an "announcements" app where the group is muted due to flood but when an important announcement is needed then someone can use the webxdc "*" to notify all of the event

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that for webxdcs, isMention should either always be false or wasDirectlyNotified (i.e. not only with *). Because I think it would be quite annoying to get notifications XY added an event to the calendar all the time even though you muted the group, and look like a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oth the event in the calendar could be important, ex. in work context, while the rest of the chat flood is kept away by the muted chat

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw we simply can't know if it was a direct mention or an "*" mention from current event, so "only notify direct mentions not @all" is simply not possible

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what we could do to mitigate potential annoyances is to have a setting in notifications to disable notification of "mentions" and webxdc in muted chats, like I did for ArcaneChat:

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it still seems to be the "right" way not to notify here to me, we can't know until the feature is actually used and we get some real-life experience, (plus, I do see the advantages @adbenitez mentioned), so, I would also be fine with merging as-is

to move forward.

(Björn, 2024)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2024? he is moving forward since the 80s

Copy link
Member

@r10s r10s Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I think it would be quite annoying to get notifications XY added an event to the calendar all the time even though you muted the group, and look like a bug.

agree, we should not do that.

in general, not sure to introduce these "mentions" last minute that the user cannot get rid of.

an option might be nice, but we should have that on all UI then, this is nothing for just now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, not sure to introduce these "mentions" last minute that the user cannot get rid of.

the webxdc notifications in muted groups was intended since the very beginning or that was hpk told me, I am fine with not doing it, then notifications for webxdc will be lost since there is no way to read them later unlike with messages

@r10s
Copy link
Member

r10s commented Dec 2, 2024

as adding "mentions" last-minute without an option to really mute a chat are questionable. and, to do better, might also need help of core (which is also nothing helping getting releases out), i again, suggest to split the PR (in general, it is better to do PR that are doing one thing)

  • let's merge the is-foreground-thingie in
  • postpone mentions

@adbenitez
Copy link
Member Author

created: #3460

@adbenitez
Copy link
Member Author

to do better, might also need help of core

I am not sure it is better, notify ALL is also considered a mention in other platforms, and would break the workflow of the Calendar example I described above

@adbenitez adbenitez force-pushed the adb/improve-notifications branch from 05a5e43 to 58523a2 Compare December 2, 2024 17:12
@adbenitez adbenitez changed the title improve notifications handling notify webxdc events, replies and reactions to own messages even if chat is muted Dec 2, 2024
@adbenitez
Copy link
Member Author

I updated this PR, now this one is about the extra notifications in "mentions" situations in muted chats

@adbenitez adbenitez self-assigned this Dec 2, 2024
@r10s
Copy link
Member

r10s commented Dec 2, 2024

I am not sure it is better, notify ALL is also considered a mention in other platforms, and would break the workflow of the Calendar example I described above

well, from that point of view every message could be regarded mention 🤷‍♂️ :)

question is, if a webxdc notify to "*" is more like Hello or @all Hello - so, a normal message that should be notified as any other message or a explicit mention

@adbenitez
Copy link
Member Author

adbenitez commented Dec 2, 2024

well, from that point of view every message could be regarded mention 🤷‍♂️ :)

question is, if a webxdc notify to "*" is more like Hello or @all Hello - so, a normal message that should be notified as any other message or a explicit mention

the property is called NOTIFY, so yes, it is more like @all msg, it is not like any normal chat message, which we might add the possibility to send from webxdc as already said here and there

@r10s
Copy link
Member

r10s commented Dec 2, 2024

at a first glance, the option is disabled by default, or?
as from another discussion ...
> if the chat is muted user will lose webxdc notifications without way to realize
... so that would still be true by default.
EDIT: tested, it is enabled by default

for *, if we regard that as a "mention" as well, indeed, no help from core would be needed, that's a little plus :)
otoh, well, indeed it is called NOTIFY but not MENTION - so allowing an webxdc to be "stronger" than a normal user-typed message is nothing we did up to now, maybe okay, just saying it is not that clear. i would also be interstee in what @Hocuri says - and @ALL just to test :)

Copy link

github-actions bot commented Dec 2, 2024

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@r10s r10s added notify wait-for-merge-window potentially dangerous or otherwise unfitting PR waiting for better merge timing labels Dec 3, 2024
Copy link
Member

@r10s r10s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor comments, i am fine with that in general, having a switch we can also experiment more :)

@@ -211,6 +213,14 @@ protected void finalize() throws Throwable {
public native boolean isSendingLocationsToChat(int chat_id);
public DcProvider getProviderFromEmailWithDns (String email) { long cptr = getProviderFromEmailWithDnsCPtr(email); return cptr!=0 ? new DcProvider(cptr) : null; }

public boolean isMentionsEnabled() {
return !"0".equals(getConfig(CONFIG_MENTION_NOTIF_ENABLED));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while working, it is a bit hard to understand, every string by "0" means "enabled" then, so also "" (which is the default). getConfigInt() would not work as well, although used for setting.

suggestion: call the thing CONFIG_NOTIIFY_MENTIONS_IF_MUTED / ui.notify_mentions_if_muted, then the default of "" or 0 matches and we can use get/setConfigInt(), and that is also in sync with the other options

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: call the thing CONFIG_NOTIIFY_MENTIONS_IF_MUTED / ui.notify_mentions_if_muted, then the default of "" or 0 matches and we can use get/setConfigInt(), and that is also in sync with the other options

renaming would be ok, but I don't see how it changes the problem, "" (unset) still needs to mean "1" (enabled) since it is the default, you want to "ui.notify_mentions_if_muted" to be on by default 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it 👍 and I think I improved the code to be easier to understand now

Copy link
Member

@r10s r10s Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I don't see how it changes the problem

sorry, my fault 🤦‍♂️

i meant to inverse the meaning of the var, so CONFIG_MUTE_MENTIONS_IF_MUTED; that way our default matches the default core will return and we do not need to deal with that.

in fact, we had the default-passing in old cores, but got rid of that as it turned out to be tricky and error-prone in detail one cannot even think of atm :) tbh, i would prefer to not introduce a getConfig(key, default) again, at least not for a comparable side-quested thing

Copy link
Member

@r10s r10s Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also think, it just does not work. for unset keys, getConfig() does not return NULL or throws an error, but returns a default value, this is an empty string unless something else is defined in core

the try/catch is only to catch Integer-parsing exceptions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it again

src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@adbenitez adbenitez force-pushed the adb/improve-notifications branch from db7f641 to af91cf7 Compare December 4, 2024 11:59
Copy link

github-actions bot commented Dec 4, 2024

To test the changes in this pull request, install this apk:
📦 app-preview.apk

Copy link
Member

@r10s r10s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, with the option i think it is fine to get it in, even in 1.50.x

@r10s r10s removed the wait-for-merge-window potentially dangerous or otherwise unfitting PR waiting for better merge timing label Dec 4, 2024
Copy link

github-actions bot commented Dec 4, 2024

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@adbenitez adbenitez merged commit 75296e1 into main Dec 6, 2024
2 checks passed
@adbenitez adbenitez deleted the adb/improve-notifications branch December 6, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants