Merge lp:~donadigo/wingpanel-indicator-notifications/memory-improvements into lp:~wingpanel-devs/wingpanel-indicator-notifications/wingpanel-indicator-notifications

[Adam Bieńkowski]

- Memory improvements
- Removed unneded code logic
- Code cleanup
- Simplified code
- Fixed: session would not load on startup
- Added: DesktopID key to session to be able to recognize sender

This branch aims to lower the memory use, remove unneded logic and simplify the code for easier reading. It also fixes that the previous session wouldn't load because it was cleared before it actually loaded.

The most changes were made to NotificationsList class where we now don't store all entries, only app entries which already contain every information we need.

Improvements were also made to the Session class where we only store one key file object and use it for both write / read operations. We now also save DesktopID key which will help the indicator recognize the sender from previous session.

The branch does not introduce any new features. However it needs extensive testing & code review before it can get merged.

[lp:~donadigo/wingpanel-indicator-notifications/memory-improvements]

107. By Adam Bieńkowski on 2016-07-20

Init app_entry with null

108. By Adam Bieńkowski on 2016-07-20

Use foreach function

109. By Adam Bieńkowski on 2016-07-21

Rename add_app_entry to add_entry_internal; bring back the resort function for AppEntry

[Felipe Escoto (philip.scott)]

Still working as intended :) good job!

review: Approve

=== modified file 'src/Indicator.vala'
--- src/Indicator.vala	2016-07-18 15:57:23 +0000
+++ src/Indicator.vala	2016-07-21 12:53:02 +0000
@@ -117,7 +117,6 @@
             main_box.pack_end (clear_all_btn, false, false, 0);
             main_box.show_all ();
 
-            nlist.clear_all ();
             restore_previous_session ();
 
             dynamic_icon.set_main_icon_name (get_display_icon_name ());
@@ -155,7 +154,7 @@
 
         if (app_settings == null || app_settings.get_boolean (REMEMBER_KEY)) {
             var entry = new NotificationEntry (notification);
-            nlist.add_item (entry);
+            nlist.add_entry (entry);
         }
 
         dynamic_icon.set_main_icon_name (get_display_icon_name ());        
@@ -174,18 +173,15 @@
 
     private void restore_previous_session () {
         var previous_session = Session.get_instance ().get_session_notifications ();
-        if (previous_session.length () > 0) {
-            previous_session.@foreach ((notification) => {
-                var entry = new NotificationEntry (notification);
-                nlist.add_item (entry);                     
-            });
-        }        
+        previous_session.foreach ((notification) => {
+            nlist.add_entry (new NotificationEntry (notification));
+        });
     }
 
     private string get_display_icon_name () {
         if (NotifySettings.get_instance ().do_not_disturb) {
             return "notification-disabled-symbolic";
-        } else if (nlist != null && nlist.get_items_length () > 0) {
+        } else if (nlist != null && nlist.get_entries_length () > 0) {
             return "notification-new-symbolic";
         }
         

=== modified file 'src/Services/Notification.vala'
--- src/Services/Notification.vala	2016-07-18 15:42:58 +0000
+++ src/Services/Notification.vala	2016-07-21 12:53:02 +0000
@@ -21,7 +21,6 @@
     public bool data_session;
 
     public string app_name;
-    public string display_name;
     public string summary;
     public string message_body;
     public string app_icon;
@@ -63,7 +62,6 @@
         data_session = false;
 
         app_name = get_string (body, Column.APP_NAME);
-        display_name = app_name;
         app_icon = get_string (body, Column.APP_ICON);
         summary = get_string (body, Column.SUMMARY);
         message_body = get_string (body, Column.BODY);
@@ -102,11 +100,10 @@
 
     public Notification.from_data (uint32 _id, string _app_name, string _app_icon,
                                 string _summary, string _message_body,
-                                string[] _actions, int64 _unix_time, string _sender) {
+                                string[] _actions, string _desktop_id, int64 _unix_time, string _sender) {
         data_session = true;
 
         app_name = _app_name;
-        display_name = app_name;
         app_icon = _app_icon;
         summary = _summary;
         message_body = _message_body;
@@ -115,15 +112,14 @@
         id = _id;
         sender = _sender;
 
-        app_info = Utils.get_appinfo_from_app_name (app_name);
-
-        setup_pid ();
-
         actions = _actions;
         unix_time = _unix_time;
         timestamp = new DateTime.from_unix_local (unix_time);
 
-        desktop_id = "";
+        desktop_id = _desktop_id;
+        app_info = new DesktopAppInfo (desktop_id);
+
+        setup_pid ();
 
         Timeout.add_seconds_full (Priority.DEFAULT, 60, source_func);
     }
@@ -132,6 +128,29 @@
         return app_info != null;
     }
 
+    public bool run_default_action () {
+        if (DEFAULT_ACTION in actions && NotificationMonitor.get_instance ().notifications_iface != null) {
+            NotificationMonitor.get_instance ().notifications_iface.action_invoked (DEFAULT_ACTION, id);
+            return true;
+        }
+
+        return false;
+    }
+
+    public Wnck.Window? get_app_window () {
+        Wnck.Window? window = null;
+        if (pid_accuired) {
+            Wnck.Screen.get_default ().get_windows ().foreach ((_window) => {
+                if (_window.get_pid () == pid && window == null) {
+                    window = _window;
+                    return;
+                }
+            });     
+        }
+
+        return window;        
+    }
+
     private void setup_pid () {
         pid_accuired = try_get_pid ();
         NotifySettings.get_instance ().changed[NotifySettings.DO_NOT_DISTURB_KEY].connect (() => {
@@ -158,15 +177,6 @@
         return true;
     }
 
-    public bool run_default_action () {
-        if (DEFAULT_ACTION in actions && NotificationMonitor.get_instance ().notifications_iface != null) {
-            NotificationMonitor.get_instance ().notifications_iface.action_invoked (DEFAULT_ACTION, id);
-            return true;
-        }
-
-        return false;
-    }
-
     private string get_string (Variant tuple, int column) {
         var child = tuple.get_child_value (column);
         return child.dup_string ();

=== modified file 'src/Services/Session.vala'
--- src/Services/Session.vala	2016-07-17 17:49:54 +0000
+++ src/Services/Session.vala	2016-07-21 12:53:02 +0000
@@ -25,14 +25,14 @@
     private const string SESSION_FILE_NAME = ".notifications.session";
     private static Session? instance = null;
 
-    private static File? session_file = null;
-    private static string full_path;
+    private File session_file = null;
     
     private const string APP_NAME_KEY = "AppName";
     private const string APP_ICON_KEY = "AppIcon";
     private const string SUMMARY_KEY = "Summary";
     private const string BODY_KEY = "Body";
     private const string ACTIONS_KEY = "Actions";
+    private const string DESKTOP_ID_KEY = "DesktopID";
     private const string UNIX_TIME_KEY = "UnixTime";
     private const string SENDER_KEY = "Sender";
 
@@ -47,29 +47,29 @@
     }
 
     private Session () {
-        full_path = Path.build_filename (Environment.get_user_cache_dir (), SESSION_FILE_NAME);
-        session_file = File.new_for_path (full_path);
-        if (!session_file.query_exists ())
+        session_file = File.new_for_path (Path.build_filename (Environment.get_user_cache_dir (), SESSION_FILE_NAME));
+        if (!session_file.query_exists ()) {
             create_session_file ();
+        }
 
         key = new KeyFile ();
-        key.set_list_separator (',');
+        key.set_list_separator (';');
     }
 
     public List<Notification> get_session_notifications () {
         var list = new List<Notification> ();
-        var _key = new KeyFile ();
         try {
-            _key.load_from_file (full_path, KeyFileFlags.NONE);
-            foreach (unowned string group in _key.get_groups ()) {
+            key.load_from_file (session_file.get_path (), KeyFileFlags.NONE);
+            foreach (unowned string group in key.get_groups ()) {
                 var notification = new Notification.from_data ((uint32)int.parse (group),
-                                                            _key.get_string (group, APP_NAME_KEY),
-                                                            _key.get_string (group, APP_ICON_KEY),
-                                                            _key.get_string (group, SUMMARY_KEY),
-                                                            _key.get_string (group, BODY_KEY),
-                                                            _key.get_string_list (group, ACTIONS_KEY),
-                                                            _key.get_int64 (group, UNIX_TIME_KEY),
-                                                            _key.get_string (group, SENDER_KEY));
+                                                            key.get_string (group, APP_NAME_KEY),
+                                                            key.get_string (group, APP_ICON_KEY),
+                                                            key.get_string (group, SUMMARY_KEY),
+                                                            key.get_string (group, BODY_KEY),
+                                                            key.get_string_list (group, ACTIONS_KEY),
+                                                            key.get_string (group, DESKTOP_ID_KEY),
+                                                            key.get_int64 (group, UNIX_TIME_KEY),
+                                                            key.get_string (group, SENDER_KEY));
                 list.append (notification);
             }
         } catch (KeyFileError e) {
@@ -88,6 +88,7 @@
         key.set_string (id, SUMMARY_KEY, notification.summary);
         key.set_string (id, BODY_KEY, notification.message_body);
         key.set_string_list (id, ACTIONS_KEY, notification.actions);
+        key.set_string (id, DESKTOP_ID_KEY, notification.desktop_id);
         key.set_int64 (id, UNIX_TIME_KEY, notification.unix_time);
         key.set_string (id, SENDER_KEY, notification.sender);
 
@@ -106,7 +107,6 @@
 
     public void clear () {
         try {
-            key = new KeyFile ();
             FileUtils.set_contents (session_file.get_path (), "");
         } catch (FileError e) {
             warning (e.message);
@@ -122,9 +122,6 @@
     }
 
     private void write_contents () {
-        if (session_file == null)
-            create_session_file ();
-
         try {
             FileUtils.set_contents (session_file.get_path (), key.to_data ());
         } catch (FileError e) {

=== modified file 'src/Widgets/AppEntry.vala'
--- src/Widgets/AppEntry.vala	2016-07-17 18:46:38 +0000
+++ src/Widgets/AppEntry.vala	2016-07-21 12:53:02 +0000
@@ -16,49 +16,32 @@
  */
 
 public class AppEntry : Gtk.ListBoxRow {
-    public string desktop_id;
-    public Gtk.Button clear_btn_entry;
-    public AppInfo? app_info = null;
-    public Wnck.Window? app_window;
-
-    public signal void destroy_entry ();
-
-    private List<NotificationEntry> app_notifications;
-    private string display_name;
-
-    public AppEntry (NotificationEntry entry, Wnck.Window? _app_window) {
+    public signal void clear ();
+
+    public AppInfo app_info;
+    public List<NotificationEntry> app_notifications;
+
+    public AppEntry (NotificationEntry entry) {
         margin_bottom = 3;
         margin_top = 3;
         margin_start = 12;
         margin_end = 6;
 
+        app_notifications = new List<NotificationEntry> ();
+        add_notification_entry (entry);
+
         var notification = entry.notification;
-        desktop_id = notification.desktop_id;
-        app_window = _app_window;
         app_info = notification.app_info;
 
-        app_notifications = new List<NotificationEntry> ();
-        add_notification_entry (entry);
-
         var hbox = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 12);
 
-        /* Capitalize the first letter */
-        char[] utf8 = notification.display_name.to_utf8 ();
-        utf8[0] = utf8[0].toupper ();
-
-        if (app_info != null) {
-            display_name = app_info.get_name ();
-        } else {
-            display_name = string.join ("", utf8);
-        }
-
-        var label = new Gtk.Label (display_name);
+        var label = new Gtk.Label (app_info.get_name ());
         label.get_style_context ().add_class ("h3");
 
-        clear_btn_entry = new Gtk.Button.from_icon_name ("edit-clear-symbolic", Gtk.IconSize.SMALL_TOOLBAR);
+        var clear_btn_entry = new Gtk.Button.from_icon_name ("edit-clear-symbolic", Gtk.IconSize.SMALL_TOOLBAR);
         clear_btn_entry.get_style_context ().add_class ("flat");
         clear_btn_entry.clicked.connect (() => {
-            destroy_entry ();
+            clear ();
         });
 
         string icon = "";
@@ -74,33 +57,32 @@
         hbox.pack_start (label, false, false, 0);
         hbox.pack_end (clear_btn_entry, false, false, 0);
 
-        connect_entry (entry);
-
         add (hbox);
         show_all ();
     }
 
-    private void connect_entry (NotificationEntry entry) {
-        entry.clear.connect (() => {
-            if (entry != null) {
-                remove_notification_entry (entry);
-            }
-        });
-    }
+    public Wnck.Window? get_app_window () {
+        if (app_notifications.length () == 0) {
+            return null;
+        }
 
-    public unowned List<NotificationEntry> get_notifications () {
-        return app_notifications;
+        var entry = app_notifications.first ().data;
+        return entry.notification.get_app_window ();
     }
 
     public void add_notification_entry (NotificationEntry entry) {
         app_notifications.prepend (entry);
-        connect_entry (entry);
+        entry.clear.connect (remove_notification_entry);
     }
 
-    public void remove_notification_entry (NotificationEntry entry) {
+    public async void remove_notification_entry (NotificationEntry entry) {
         app_notifications.remove (entry);
+        entry.active = false;
+        entry.destroy ();
+
+        Session.get_instance ().remove_notification (entry.notification);
         if (app_notifications.length () == 0) {
-            destroy_entry ();
+            clear ();
         }
     }
 }

=== modified file 'src/Widgets/NotificationEntry.vala'
--- src/Widgets/NotificationEntry.vala	2016-05-27 15:10:53 +0000
+++ src/Widgets/NotificationEntry.vala	2016-07-21 12:53:02 +0000
@@ -54,14 +54,7 @@
         });
 
         hexpand = true;
-        add_widgets ();
-
-        if (notification.data_session) {
-            notification.time_changed (notification.timestamp.difference (new DateTime.now_local ()));
-        }
-    }
-
-    private void add_widgets () {
+        
         var grid = new Gtk.Grid ();
         grid.margin_start = 40;
         grid.margin_end = 6;
@@ -92,6 +85,10 @@
 
         add (grid);
         show_all ();
+        
+        if (notification.data_session) {
+            notification.time_changed (notification.timestamp.difference (new DateTime.now_local ()));
+        }
     }
 
     /**

=== modified file 'src/Widgets/NotificationsList.vala'
--- src/Widgets/NotificationsList.vala	2016-07-17 18:46:38 +0000
+++ src/Widgets/NotificationsList.vala	2016-07-21 12:53:02 +0000
@@ -18,13 +18,11 @@
 public class NotificationsList : Gtk.ListBox {
     public signal void switch_stack (bool show_list);
     public signal void close_popover ();
+
     private List<AppEntry> app_entries;
-    private List<NotificationEntry> items;
     private HashTable<string, int> table;
     private int counter = 0;
 
-    private static Wnck.Screen screen;
-
     public NotificationsList () {
         margin_top = 2;
 
@@ -32,57 +30,41 @@
         selection_mode = Gtk.SelectionMode.NONE;
         row_activated.connect (on_row_activated);
 
-        items = new List<NotificationEntry> ();
         app_entries = new List<AppEntry> ();
         table = new HashTable<string, int> (str_hash, str_equal);
 
-        screen = Wnck.Screen.get_default ();
-
         vexpand = true;
         show_all ();
+
+        monitor_active_window ();
     }
 
-    public void add_item (NotificationEntry entry) {
-        var app_entry = add_app_entry (entry);
+    public void add_entry (NotificationEntry entry) {
+        var app_entry = add_entry_internal (entry);
+        if (app_entry == null) {
+            return;
+        }
 
-        items.prepend (entry);
         switch_stack (true);
 
-        app_entry.add_notification_entry (entry);
-        resort_from_app_entry (app_entry);
-
-        entry.clear.connect (() => {
-            destroy_notification_entry (entry);
-        });
-
-        app_entry.destroy_entry.connect (() => {
-            destroy_app_entry (app_entry);
-        });
-
-        counter = counter + 2;
+        app_entry.clear.connect (clear_app_entry);
+
+        counter += 2;
 
         Session.get_instance ().add_notification (entry.notification);
-        entry.show_all ();
 
         update_separators ();
         show_all ();
     }
 
 
-    public uint get_items_length () {
-        return items.length ();
+    public uint get_entries_length () {
+        return app_entries.length ();
     }
 
     public void clear_all () {
-        items.@foreach ((item) => {
-            items.remove (item);
-            remove (item);
-            item.active = false;
-        });
-
-        app_entries.@foreach ((entry) => {
-            app_entries.remove (entry);
-            remove (entry);
+        app_entries.foreach ((app_entry) => {
+            app_entry.clear ();
         });
 
         counter = 0;
@@ -93,6 +75,17 @@
         show_all ();
     }
 
+    private void resort_app_entry (AppEntry app_entry) {
+        if (get_row_at_index (0) != app_entry) {
+            remove (app_entry);
+            prepend (app_entry);
+            app_entry.app_notifications.foreach ((notification_entry) => {
+                remove (notification_entry);
+                insert (notification_entry, 1);
+            });
+        }
+    }
+
     private void update_separators () {
         if (get_children ().length () > 0) {
             foreach (var child in get_children ()) {
@@ -112,27 +105,24 @@
         show_all ();
     }
 
-    private AppEntry add_app_entry (NotificationEntry entry) {
-        AppEntry app_entry;
+    private AppEntry? add_entry_internal (NotificationEntry entry) {
+        AppEntry? app_entry = null;
         bool add = !(entry.notification.desktop_id in construct_desktop_id_list ());
         if (add) {
-            var window = get_window_from_entry (entry);
-            app_entry = new AppEntry (entry, window);
-
-            screen.active_window_changed.connect (() => {
-                if (screen.get_active_window () == app_entry.app_window) {
-                    app_entry.clear_btn_entry.clicked ();
-                }
-            });
+            app_entry = new AppEntry (entry);
 
             app_entries.append (app_entry);
             prepend (app_entry);
             insert (entry, 1);
-            table.insert (app_entry.desktop_id, 0);
+            table.insert (app_entry.app_info.get_id (), 0);
         } else {
             app_entry = get_app_entry_from_desktop_id (entry.notification.desktop_id);
+
             if (app_entry != null) {
-                int insert_pos = table.@get (app_entry.desktop_id);
+                resort_app_entry (app_entry);
+                app_entry.add_notification_entry (entry);
+
+                int insert_pos = table.get (app_entry.app_info.get_id ());
                 insert (entry, insert_pos + 1);                
             }
         }
@@ -140,118 +130,94 @@
         return app_entry;
     }
 
-    private Wnck.Window? get_window_from_entry (NotificationEntry entry) {
-        Wnck.Window? window = null;
-        screen.get_windows ().@foreach ((_window) => {
-            if (_window.get_pid () == entry.notification.pid) {
-                window = _window;
-                return;
-            }
+    private void monitor_active_window () {
+        var screen = Wnck.Screen.get_default ();
+        screen.active_window_changed.connect (() => {
+            app_entries.foreach ((app_entry) => {
+                if (screen.get_active_window () == app_entry.get_app_window ()) {
+                    app_entry.clear ();
+                }
+            });
         });
-
-        return window;
-    }
-
-    private async void destroy_notification_entry (NotificationEntry entry) {
-        entry.destroy ();
-        items.remove (entry);
-        entry.active = false;
-
-        Session.get_instance ().remove_notification (entry.notification);
-        if (items.length () == 0) {
-            clear_all ();
-        }
-    }
-
-    private void destroy_app_entry (AppEntry app_entry) {
+    }
+
+    private void clear_app_entry (AppEntry app_entry) {
         app_entries.remove (app_entry);
 
-        app_entry.get_notifications ().@foreach ((notification_entry) => {
-            notification_entry.destroy ();
-            items.remove (notification_entry);
-        });
-
-        Idle.add (() => {
-            if (app_entry != null) {
-                app_entry.destroy ();
-            }
-
-            return false;
-        });
-
-        if (items.length () == 0) {
+        app_entry.app_notifications.foreach ((notification_entry) => {
+            app_entry.remove_notification_entry.begin (notification_entry);
+        });
+
+        app_entry.destroy ();
+        update_separators ();
+
+        if (get_entries_length () == 0) {
             clear_all ();
         }
-
-        update_separators ();
-    }
-
-    private void resort_from_app_entry (AppEntry app_entry) {
-        if (get_row_at_index (0) != app_entry) {
-            remove (app_entry);
-            prepend (app_entry);
-            int counter = 1;
-            app_entry.get_notifications ().@foreach ((notification_entry) => {
-                remove (notification_entry);
-                add_item (notification_entry);
-                counter++;
-            });
-        }
     }
 
     private AppEntry? get_app_entry_from_desktop_id (string desktop_id) {
-        AppEntry? entry = null;
-        app_entries.@foreach ((_entry) => {
-            if (_entry.desktop_id == desktop_id) {
-                entry = _entry;
-                return;
+        AppEntry? app_entry = null;
+        app_entries.foreach ((_app_entry) => {
+            if (_app_entry.app_info.get_id () == desktop_id && app_entry == null) {
+                app_entry = _app_entry;
             }
         });
 
-        return entry;
+        return app_entry;
     }
 
     private string[] construct_desktop_id_list () {
         string[] desktop_id_list = {};
-        app_entries.@foreach ((entry) => {
-            desktop_id_list += entry.desktop_id;
+        app_entries.foreach ((app_entry) => {
+            desktop_id_list += app_entry.app_info.get_id ();
         });
 
         return desktop_id_list;
     }
 
     private void on_row_activated (Gtk.ListBoxRow row) {
-        if (row.get_path ().get_object_type () == typeof (AppEntry)) {
-            if (((AppEntry) row).app_window == null) {
-                var window = get_window_from_entry (((AppEntry) row).get_notifications ().nth_data (0));
-                if (window != null) {
-                    ((AppEntry) row).app_window = window;
-                }
-            }
-
-            if (((AppEntry) row).app_window != null) {
-                ((AppEntry) row).app_window.unminimize (Gtk.get_current_event_time ());
-                ((AppEntry) row).clear_btn_entry.clicked ();
-                close_popover ();
-            } else if (((AppEntry) row).app_info != null) {
-                try {
-                    ((AppEntry) row).app_info.launch (null, null);
-                } catch (Error e) {
-                    error ("%s\n", e.message);
-                }
-
-                ((AppEntry) row).clear_btn_entry.clicked ();
-                close_popover ();
-            }
+        bool close = true;
+
+        if (row is AppEntry) {
+            var app_entry = (AppEntry)row;
+            close = focus_notification_app (app_entry.get_app_window (),
+                                            app_entry.app_info);
+
+            app_entry.clear ();
+        } else if (row is NotificationEntry) {
+            var notification_entry = (NotificationEntry)row;
+
+            if (!notification_entry.notification.run_default_action ()) {
+                close = focus_notification_app (notification_entry.notification.get_app_window (),
+                                                notification_entry.notification.app_info);
+            }
+
+            notification_entry.clear ();
         } else {
-            if (((NotificationEntry) row).notification.run_default_action ()) {
-                ((NotificationEntry) row).active = false;
-                close_popover ();
-            }
+            close = false;
+        }
 
-            ((NotificationEntry) row).clear ();
+        if (close) {
+            close_popover ();
         }
 
         update_separators ();
     }
+
+    private bool focus_notification_app (Wnck.Window? app_window, AppInfo? app_info) {
+        if (app_window != null) {
+            app_window.unminimize (Gtk.get_current_event_time ());
+            return true;
+        } else if (app_info != null) {
+            try {
+                app_info.launch (null, null);
+                return true;
+            } catch (Error e) {
+                warning ("%s\n", e.message);
+            }            
+        }
+
+        return false;
+    }
 }