After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 769592 - Fix launching through (persistent) notifications
Fix launching through (persistent) notifications
Status: RESOLVED FIXED
Product: polari
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Polari maintainers
Polari maintainers
Depends on:
Blocks: 751575 769655
 
 
Reported: 2016-08-07 01:27 UTC by Florian Müllner
Modified: 2016-08-29 23:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
accountsMonitor: Replace ::am-prepared signal with a method (6.02 KB, patch)
2016-08-07 01:27 UTC, Florian Müllner
committed Details | Review
mainWindow: Initialize sidebar visibility (2.04 KB, patch)
2016-08-07 01:27 UTC, Florian Müllner
committed Details | Review
roomList: Exclude placeholders from show_all() (1.30 KB, patch)
2016-08-07 01:27 UTC, Florian Müllner
committed Details | Review
app: Present window if appropriate when handling join/query actions (1.67 KB, patch)
2016-08-07 01:28 UTC, Florian Müllner
committed Details | Review
app: Ensure we are prepared before handling join-room/message-user (2.10 KB, patch)
2016-08-07 01:28 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2016-08-07 01:27:37 UTC
The notifications we show on highlights persist even when polari is closed, but clicking one to launch polari is currently spectacularly broken. The attached patch set makes it work again.
Comment 1 Florian Müllner 2016-08-07 01:27:44 UTC
Created attachment 332871 [details] [review]
accountsMonitor: Replace ::am-prepared signal with a method

The signal was added in commit 5bfc81 to make sure that nothing interacts
with the account manager before its factory has been set up. However using
a signal for that purpose is problematic - it is only emitted when the
account monitor's own prepare_async() call returns, so any signal handlers
that are only connected after that call has completed will never run.

We can fix this issue without reintroducing the ambiguity of multiple
prepare_async() calls by providing our own prepare() method that runs
its callback parameter either immediately or once the account manager
is prepared.
Comment 2 Florian Müllner 2016-08-07 01:27:49 UTC
Created attachment 332872 [details] [review]
mainWindow: Initialize sidebar visibility

The code currently assumes that the window is created before accounts are
initialized, which is the case in the common case that the application is
activated immediately. However when started through an action (for example
when clicking a persistent notification), accounts may already be ready
when we call activate() ourselves, and the sidebar remains hidden (until
the user adds another account). Fix this by making sure the visibility
is properly initialized.
Comment 3 Florian Müllner 2016-08-07 01:27:55 UTC
Created attachment 332873 [details] [review]
roomList: Exclude placeholders from show_all()

For enabled accounts without rooms, we show an empty placeholder widget
so that we still get the header that allows managing the account. While
we correctly set the initial visibility, that code may run before any
parent calls show_all(), and we end up with disabled accounts in the
sidebar - we need to control the placeholders' visibility ourselves,
so exclude them from show_all().
Comment 4 Florian Müllner 2016-08-07 01:28:01 UTC
Created attachment 332874 [details] [review]
app: Present window if appropriate when handling join/query actions

When we are launched through an action, it is on us to present a window
if the action requires it. Joining a room or switching to it doesn't make
much sense if we don't display it anywhere, so ensure we have a window
and present it if appropriate.
Comment 5 Florian Müllner 2016-08-07 01:28:06 UTC
Created attachment 332875 [details] [review]
app: Ensure we are prepared before handling join-room/message-user

We are not prepared to handle the join-room/message-user actions before
the account manager is prepared, so run the handling through the account
monitor's prepare() method.
Comment 6 Florian Müllner 2016-08-07 01:43:47 UTC
This is part of a bigger patchset, so probably won't apply to master directly. To make it easy to try without hunting dependencies in bugs, I've pushed it as part of https://git.gnome.org/browse/polari/log/?h=wip%2Ffmuellner%2Fmisc-fixes
Comment 7 Bastian Ilsø 2016-08-13 11:04:22 UTC
Review of attachment 332871 [details] [review]:

looks good to me.
Comment 8 Bastian Ilsø 2016-08-13 11:14:17 UTC
Review of attachment 332872 [details] [review]:

looks good to me.
Comment 9 Bastian Ilsø 2016-08-14 08:47:19 UTC
Review of attachment 332873 [details] [review]:

looks good to me.
Comment 10 Bastian Ilsø 2016-08-14 08:50:50 UTC
Review of attachment 332874 [details] [review]:

looks good to me
Comment 11 Bastian Ilsø 2016-08-14 08:58:04 UTC
Review of attachment 332875 [details] [review]:

looks good to me.
Comment 12 Florian Müllner 2016-08-29 23:29:27 UTC
Attachment 332871 [details] pushed as eed4911 - accountsMonitor: Replace ::am-prepared signal with a method
Attachment 332872 [details] pushed as 9460f34 - mainWindow: Initialize sidebar visibility
Attachment 332873 [details] pushed as 857f388 - roomList: Exclude placeholders from show_all()
Attachment 332874 [details] pushed as 2377eb8 - app: Present window if appropriate when handling join/query actions
Attachment 332875 [details] pushed as bf5db76 - app: Ensure we are prepared before handling join-room/message-user