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 759812 - Remember what room was selected last time when Polari starts again
Remember what room was selected last time when Polari starts again
Status: RESOLVED FIXED
Product: polari
Classification: Applications
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Polari maintainers
Polari maintainers
Depends on:
Blocks:
 
 
Reported: 2015-12-23 16:49 UTC by Bastian Ilsø
Modified: 2016-03-02 22:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Store the last active (selected) room (4.78 KB, patch)
2016-02-27 19:36 UTC, Rares Visalom
none Details | Review
Store the last active (selected) room (4.21 KB, patch)
2016-03-01 23:32 UTC, Rares Visalom
none Details | Review
Store the last active (selected) room (4.17 KB, patch)
2016-03-02 20:09 UTC, Rares Visalom
committed Details | Review

Description Bastian Ilsø 2015-12-23 16:49:17 UTC
When Polari closes we don't do much to remember what room was being viewed at that point. It could be nice if we show that room again when Polari is started next time.
Comment 1 Rares Visalom 2016-02-27 19:36:33 UTC
Created attachment 322550 [details] [review]
Store the last active (selected) room

The last active (selected) room is stored as a setting in order
for it to be set as active the next time the user
launches polari.
Comment 2 Florian Müllner 2016-02-28 13:43:41 UTC
Review of attachment 322550 [details] [review]:

::: data/org.gnome.Polari.gschema.xml
@@ +17,3 @@
       <description>Window maximized state</description>
     </key>
+    <key type="a{sv}" name="last-active-room">

We should probably try to pick a name that is more consistent with 'saved-channel-list'. 'last-selected-channel', or maybe even 'selected-channel'?

@@ +20,3 @@
+      <default>{}</default>
+      <summary>Last Active Room</summary>
+      <description>Last Active (Selected) Room</description>

Both summary and description should use sentence capitalization, not header capitalization

::: src/chatroomManager.js
@@ +230,3 @@
+            lastActiveRoom[prop] = lastActiveRoom[prop].deep_unpack();
+
+        this._restoreChannel(lastActiveRoom);

This will need a check for the case where lastActiveRoom is unset.

@@ +448,3 @@
         this.emit('room-added', room);
 
+        let lastActiveRoom = this._settings.get_value('last-active-room').deep_unpack();

Left-over? It doesn't look like you do anything with this ...

@@ +467,3 @@
     },
 
+    _onDelete: function() {

Our convention uses underscore to mark properties/methods as private. If you add a method that is called from another module like here, it should be public. It's also not a good idea to name a method that's called normally as if it was a callback for a signal.

With all that said:
The last selected room is not window state (like maximization state or size), so it's not really important that we save it when the window is closed - we just want to save it on exit, so I'd prefer to keep the method private and use a different signal to handle this, instead of calling it from MainWindow. GApplication::shutdown probably works (I don't think set_value()/reset() requires a main loop, but I may be wrong), if not you can use Gjs_Application::prepare-shutdown.

@@ +482,3 @@
             return;
 
+        if (room && room.type == Tp.HandleType.ROOM) {

Style nit: no braces
Comment 3 Rares Visalom 2016-03-01 23:32:06 UTC
Created attachment 322816 [details] [review]
Store the last active (selected) room

The last active (selected) room is stored as a setting in order
for it to be set as active the next time the user
launches polari.
Comment 4 Florian Müllner 2016-03-02 00:03:08 UTC
Review of attachment 322816 [details] [review]:

::: src/chatroomManager.js
@@ +229,3 @@
         }));
+
+        let lastActiveRoom = this._settings.get_value('last-selected-channel').deep_unpack();

lastActiveRoom = 'last-selected-channel' is a bit odd - selectedChannel maybe? This would also remove the oddity that 'lastActiveRoom' is a serialized channel, while 'this._lastActiveRoom' is a pointer to a PolariRoom.

@@ +231,3 @@
+        let lastActiveRoom = this._settings.get_value('last-selected-channel').deep_unpack();
+
+        if(lastActiveRoom != {} && lastActiveRoom.account && lastActiveRoom.channel){

The first condition is always true.

Nits: space after if and before brace

@@ +233,3 @@
+        if(lastActiveRoom != {} && lastActiveRoom.account && lastActiveRoom.channel){
+            for (let prop in lastActiveRoom)
+             lastActiveRoom[prop] = lastActiveRoom[prop].deep_unpack();

Wrong indentation. Also fine to do unconditionally and only guard the call to _restoreChannel():

let lastActiveRoom = this._settings.get_value(...);

for (let prop in lastActiveRoom)
    lastActiveRoom[prop] = lastActiveRoom[prop].deep_unpack();

if (lastActiveRoom.account && lastActiveRoom.channel)
    this._restoreChannel(lastActiveRoom);

@@ +473,3 @@
+    _onPrepareShutdown: function() {
+        if (this._lastActiveRoom) {
+            let tempLastActiveRoom = { account: GLib.Variant.new('s', this._lastActiveRoom.account.get_object_path()),

Not a big fan of "temp" variable names - serializedChannel or selectedChannel or so would be better IMHO
Comment 5 Rares Visalom 2016-03-02 20:09:04 UTC
Created attachment 322916 [details] [review]
Store the last active (selected) room

The last active (selected) room is stored as a setting in order
for it to be set as active the next time the user
launches polari.
Comment 6 Florian Müllner 2016-03-02 22:23:15 UTC
Review of attachment 322916 [details] [review]:

Looks good, thanks! I'll fix the remaining style nits and push ...

::: src/chatroomManager.js
@@ +473,3 @@
+        if (this._lastActiveRoom) {
+            let serializedChannel = { account: GLib.Variant.new('s', this._lastActiveRoom.account.get_object_path()),
+                                       channel: GLib.Variant.new('s', this._lastActiveRoom.channel_name) };

'channel' should align with 'account'

@@ +477,3 @@
+            this._settings.set_value('last-selected-channel', GLib.Variant.new('a{sv}', serializedChannel));
+        }
+        else

Coding style:
 - brace and else go on the same line
 - for if-else clauses, either both blocks
   use braces or both blocks don't
Comment 7 Florian Müllner 2016-03-02 22:24:59 UTC
Attachment 322916 [details] pushed as 6df5f3d - Store the last active (selected) room