Systray improvements #818
Systray improvements #818
Conversation
Tamaranch
force-pushed
the
systray-improvements
branch
from
05be192
to
261f1de
Compare
There was a problem hiding this comment.
There are a few things that could be better here and there but the main question I have for now is whether it's good to trigger on notifications instead of highlights. Given that the loudest ("intrusive") mode focuses the application, a series of notifications from messages in a 1:1 chat may be quite disruptive if they come, say, every minute or even more frequently. Perhaps it's worth handling notifications and highlights differently in that mode specifically. Also, as I wrote below, you seem to mis-accumulate the number of notifications across the application if that's what you tried to do.
| @@ -844,6 +844,8 @@ void MainWindow::addConnection(Connection* c, const QString& deviceName) | |||
| }); | |||
| connect(c, &Connection::loginError, this, | |||
| [this, c](const QString& msg) { loginError(c, msg); }); | |||
| for (auto* r: c->allRooms()) | |||
| systemTrayIcon->newRoom(r); | |||
There was a problem hiding this comment.
At this point allRooms() will always return an empty container because the first sync hasn't occurred yet; and during syncs, the tray is updated about new room objects via the newRoom signal - the respective connect() call is right below here.
There was a problem hiding this comment.
That's not what I'm seeing: this change does get me into systemTrayIcon::newRoom() at startup (so the container doesn't seem empty), and that's necessary so that I'm notified of unread messages from the previous session (I start Quaternion in hidden mode --hide-mainwindow).
In fact, if I don't add this initialization, I never see a pass in systemTrayIcon::newRoom() for rooms that are already present at startup, so no connection is made to their notification signals.
There was a problem hiding this comment.
Hm, interesting, maybe I misunderstand something in the way things work now...
| if (mode == "none") | ||
| int nNotifs = 0; | ||
|
|
||
| if (qApp->activeWindow() != nullptr || room->notificationCount() == 0) |
There was a problem hiding this comment.
Highlights and notifications are not exactly the same thing in Matrix. It's a bit complex but basically highlights are supposed to be "louder" than notifications. E.g. a new message in a 1:1 chat normally causes a notification but not a highlight - for a highlight to be triggered, one has to be mentioned. I'm not quite sure if it's best to elevate notifications to system tray popups; but you might have a point.
There was a problem hiding this comment.
Oh ok, I see. I seem to have a somewhat peculiar and minimalist use of the software, actually: maintaining a single IRC connection via Matrix on a few fairly low-traffic channels, where I'm almost never mentioned, hence my impression that the notifications weren't working.
So it may be that the current state of the notification system suits the majority of users, and that this PR is not very relevant. I will continue to use this patch for me, as it is suitable for my case, but maybe it is better to just close this PR?
There was a problem hiding this comment.
You might want to login to Matrix from another client (Element Web, e.g.) once, and tune notifications for those rooms (or even for the whole account) so that all notifications are "loud" (the rightmost setting in respective lines). As for this PR, I think at least some pieces of it are still worthwhile - I really like the icon change, in particular - so perhaps I'll cherry-pick those.
Tamaranch
force-pushed
the
systray-improvements
branch
from
261f1de
to
62368f8
Compare
Tamaranch
force-pushed
the
systray-improvements
branch
from
62368f8
to
4eddfc6
Compare
Tamaranch
force-pushed
the
systray-improvements
branch
from
4eddfc6
to
aaea727
Compare
The highlight count never seems to change, and the corresponding signal never gets emitted. On the other hand, the notification count seems to correspond well to the number of unread messages, and the correct signal to connect to in order to get it seems to be Room::unreadStatsChanged().
It is natural not to disturb the user in this case, since his attention is already focused on the application. In addition, it allows to filter notifications when the number of unread messages decreases.
This cannot be done without leaving some indication to the user that a notification has been issued in the past, which may anyway appear as a lack even when notifications are issued with every new message. This is achieved by changing the icon semi-permanently (until the next activation of the main window), and by silently updating the tooltip when new notifications are issued with the total number of unread messages. This icon change is included in the minimal notification mode, as it is very non-intrusive.
Tamaranch
force-pushed
the
systray-improvements
branch
from
aaea727
to
9beac0c
Compare
The first two commits are fixes for what I think are bugs.
The third commit is a minimal change to make the notification system usable in practice (from my point of view), once the two previous fixes are applied.
The fourth commit is an improvement proposal. I have been using the whole thing for a few days now locally.
I'm new to Qt, and my memories of C++ are old (I'm from the C/GTK side 🙂), so I hope I didn't do too much stupid things or reinvent the wheel.