Skip to content

Conversation

@Bionicgitman
Copy link

@Bionicgitman Bionicgitman commented Mar 5, 2025

Currently Profanity sends notifications even if you are focused on the same chat window as the recipient message.
It should account for whenever profanity is the focused window or not.
See #1987 .

@Bionicgitman Bionicgitman changed the title feat/1987-DisableSelWinNotifs Disable message notifications from active chatrooms Mar 5, 2025
@Bionicgitman Bionicgitman marked this pull request as draft March 5, 2025 13:37
Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

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

Looks good!

@jubalh
Copy link
Member

jubalh commented Mar 6, 2025

Thanks for this PR!
Any reason its just a draft yet?

@Bionicgitman
Copy link
Author

I have yet to include the AFK portion, I will submit it soon

@Bionicgitman Bionicgitman marked this pull request as ready for review March 6, 2025 11:40
@sjaeckel sjaeckel force-pushed the feat/1987-DisableSelWinNotifs branch from 3e1e63c to 6f1f43e Compare March 13, 2025 08:59
Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

I just had a look at the existing implementation and found that we already have PREF_NOTIFY_ROOM_CURRENT resp. PREF_NOTIFY_CHAT_CURRENT. Did you try enabling those before making these changes?

The main difference is then the check for the idle time, which should happen inside prefs_do_room_notify() resp. prefs_do_chat_notify() and the time should maybe be configurable? (CC @jubalh) The ChatState is only for 1on1 chats, so we can't use that.

@jubalh
Copy link
Member

jubalh commented Mar 25, 2025

the time should maybe be configurable

Makes sense :)

@sjaeckel sjaeckel requested a review from jubalh April 16, 2025 10:10
Copy link

@Privat33r-dev Privat33r-dev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. In general, as it was mentioned earlier, disabling notification for the current chat room is already enabled in a form of a preference, condition for which is being checked inside of the prefs_do_chat_notify, called to calculate notify variable.

if ((current_win == FALSE) || ((current_win == TRUE) && prefs_get_boolean(PREF_NOTIFY_CHAT_CURRENT))) {

I suggest to instead make changes in the prefs_do_chat_notify and prefs_do_room_notify, using configurable preference for idling, as it was suggested earlier by @sjaeckel.

mucwin->last_msg_timestamp = g_date_time_new_now_local();

if (prefs_do_room_notify(is_current, mucwin->roomjid, mynick, message->from_jid->resourcepart, message->plain, mention, triggers != NULL)) {
if ((prefs_do_room_notify(is_current, mucwin->roomjid, mynick, message->from_jid->resourcepart, message->plain, mention, triggers != NULL) && !wins_is_current(window)) || ui_get_idle_time() > 1000) {

Choose a reason for hiding this comment

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

Same issue with conditions as explained in another comment.

@jubalh
Copy link
Member

jubalh commented Aug 22, 2025

@Bionicgitman ping

@Privat33r-dev
Copy link

Unfortunately, it still duplicates functionality of PREF_NOTIFY_CHAT_CURRENT.

@Bionicgitman
Copy link
Author

Started working on adding idle time as an option. Tell me if there are any problems with it or if it isn't complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants