From 872ffe64126bddc6dc45221129da0a8c67d62df5 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 21:01:08 +0100 Subject: [PATCH 01/20] performance: Ditch unneeded entry_has_name() We already performed an entry.get_name() and stored that. So lateron we do not need to query if it has a username. Either it has or it is None and we know that already. This is called once per entry in the group to be displayed and this call took 5 seconds on my laptop for a simple db with 1000 entries. --- passwordsafe/entry_row.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 713bfee99..985caecb5 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -56,7 +56,7 @@ class EntryRow(Gtk.ListBoxRow): icon_name: str = passwordsafe.icon.get_icon_name(self.icon) entry_icon.set_from_icon_name(icon_name, 20) # Title/Name - if self.database_manager.has_entry_name(self.entry_uuid) and self.label: + if self.label: entry_name_label.set_text(self.label) else: entry_name_label.set_markup("" + _("Title not specified") + "") -- GitLab From 8e8380c39914061686d7a9ab1d1ca753c6bb352c Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 21:16:13 +0100 Subject: [PATCH 02/20] make get_entry_color() take an Entry instead of just a UUID This way we can hand it the Entry that we have anyway and we save one lookup with find_entries which is expensive. This function is called once per entry in a group that we display and took 5 seconds for a list of 1000 entries on my laptop. --- passwordsafe/color_widget.py | 3 +-- passwordsafe/database_manager.py | 32 +++++++++++++++++++++++++++++--- passwordsafe/entry_row.py | 2 +- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/passwordsafe/color_widget.py b/passwordsafe/color_widget.py index 343846649..51b1f5cab 100644 --- a/passwordsafe/color_widget.py +++ b/passwordsafe/color_widget.py @@ -82,8 +82,7 @@ class ColorEntryRow(Gtk.ListBoxRow): # pylint: disable=too-few-public-methods self._scrolled_page: ScrolledPage = scrolled_page self._entry_uuid: UUID = entry_uuid - self._selected_color: str = self._db_manager.get_entry_color_from_entry_uuid( - entry_uuid) + self._selected_color: str = self._db_manager.get_entry_color(entry_uuid) for color in Color: active: bool = (self._selected_color == color.value) diff --git a/passwordsafe/database_manager.py b/passwordsafe/database_manager.py index a8f550254..7ab3abc8d 100644 --- a/passwordsafe/database_manager.py +++ b/passwordsafe/database_manager.py @@ -182,6 +182,8 @@ class DatabaseManager(GObject.GObject): def get_entry_name(self, data: Union[Entry, UUID]) -> str: """Get entry name from an uuid or an entry + Passing in an Entry is more performant than passing in a UUID + as we avoid having to look up the entry. :param data: UUID or Entry :returns: entry name or an empty string if it does not exist :rtype: str @@ -201,6 +203,8 @@ class DatabaseManager(GObject.GObject): def get_entry_username(self, data: Union[Entry, UUID]) -> str: """Get an entry username from an entry or an uuid + Passing in an Entry is more performant than passing in a UUID + as we avoid having to look up the entry. :param data: entry or uuid :returns: entry username or an empty string if it does not exist :rtype: str @@ -220,6 +224,8 @@ class DatabaseManager(GObject.GObject): def get_entry_password(self, data: Union[Entry, UUID]) -> str: """Get an entry password from an entry or an uuid + Passing in an Entry is more performant than passing in a UUID + as we avoid having to look up the entry. :param data: entry or uuid :returns: entry password or an empty string if it does not exist :rtype: str @@ -239,6 +245,8 @@ class DatabaseManager(GObject.GObject): def get_entry_url(self, data: Union[Entry, UUID]) -> str: """Get an entry url from an entry or an uuid + Passing in an Entry is more performant than passing in a UUID + as we avoid having to look up the entry. :param data: UUID or Entry :returns: entry url or an empty string if it does not exist :rtype: str @@ -255,9 +263,27 @@ class DatabaseManager(GObject.GObject): return entry.url or "" - # Return the belonging color for an entry uuid - def get_entry_color_from_entry_uuid(self, uuid): - entry = self.db.find_entries(uuid=uuid, first=True) + def get_entry_color(self, data: Union[Entry, UUID]) -> Union[str, Color]: + """Get an entry color from an entry or an uuid + + Passing in an Entry is more performant than passing in a UUID + as we avoid having to look up the entry. + :param data: UUID or Entry + :returns: entry color as str or Color.NONE.value + :rtype: str + """ + if isinstance(data, UUID): + entry: Entry = self.db.find_entries(uuid=data, first=True) + if not entry: + logging.warning( + "Trying to look up a non-existing UUID %s, this should " + "never happen", + data, + ) + return Color.NONE.value + else: + entry = data + if entry.get_custom_property("color_prop_LcljUMJZ9X") is None: return Color.NONE.value else: diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 985caecb5..a08542307 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -35,7 +35,7 @@ class EntryRow(Gtk.ListBoxRow): self.icon: Optional[int] = dbm.get_icon(entry) self.label = dbm.get_entry_name(entry) self.password = dbm.get_entry_password(entry) - self.color = dbm.get_entry_color_from_entry_uuid(self.entry_uuid) + self.color = dbm.get_entry_color(entry) self.assemble_entry_row() -- GitLab From 8acc43a3996d64f552e28a57bcf38e784170b1da Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 21:35:05 +0100 Subject: [PATCH 03/20] Do not use has_entry_username more than necessary When assembling the listbox items for our Entries, we were repeatedly calling has_entry_username() and get_entry_username() with the uuid of an entry which causes expensive lookups. Just look up the username once and be done with it. --- passwordsafe/entry_row.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index a08542307..5b40ea1f1 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -4,6 +4,7 @@ from gettext import gettext as _ from uuid import UUID from typing import Optional from gi.repository import Gtk + import passwordsafe.config_manager import passwordsafe.icon from passwordsafe.color_widget import Color @@ -36,6 +37,11 @@ class EntryRow(Gtk.ListBoxRow): self.label = dbm.get_entry_name(entry) self.password = dbm.get_entry_password(entry) self.color = dbm.get_entry_color(entry) + self.username: Optional[str] = dbm.get_entry_username(entry) + if self.username and self.username.startswith("{REF:U"): + # Loopup reference and put in the "real" username + uuid = UUID(self.unlocked_database.reference_to_hex_uuid(self.username)) + self.username = self.database_manager.get_entry_username(uuid) self.assemble_entry_row() @@ -62,16 +68,8 @@ class EntryRow(Gtk.ListBoxRow): entry_name_label.set_markup("" + _("Title not specified") + "") # Subtitle - subtitle = self.database_manager.get_entry_username(self.entry_uuid) - if (self.database_manager.has_entry_username(self.entry_uuid) and subtitle): - username = self.database_manager.get_entry_username(self.entry_uuid) - if username.startswith("{REF:U"): - uuid = UUID( - self.unlocked_database.reference_to_hex_uuid(username)) - username = self.database_manager.get_entry_username(uuid) - entry_subtitle_label.set_text(username) - else: - entry_subtitle_label.set_text(username) + if self.username: + entry_subtitle_label.set_text(self.username) else: entry_subtitle_label.set_markup("" + _("No username specified") + "") -- GitLab From 6980c2acdb7a844e2aa554ccbc81224893273653 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 21:47:44 +0100 Subject: [PATCH 04/20] entry_page: remove superfluous has_entry_name and get_entry_name It turned out that these calls are more expensive that we thought, so refactor the code so that we need to call that only once. --- passwordsafe/entry_page.py | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/passwordsafe/entry_page.py b/passwordsafe/entry_page.py index bdfaaaa97..c7c4205be 100644 --- a/passwordsafe/entry_page.py +++ b/passwordsafe/entry_page.py @@ -67,33 +67,23 @@ class EntryPage: builder.add_from_resource("/org/gnome/PasswordSafe/entry_page.ui") - entry_uuid = self.unlocked_database.current_element.uuid + cur_entry = self.unlocked_database.current_element + entry_uuid = cur_entry.uuid scrolled_page = self.unlocked_database.get_current_page() - - if self.unlocked_database.database_manager.has_entry_name(entry_uuid) is True or add_all is True: + entry_name = self.unlocked_database.database_manager.get_entry_name(cur_entry) + if entry_name or add_all: if scrolled_page.name_property_row is NotImplemented: + # Create the name_property_row scrolled_page.name_property_row = builder.get_object("name_property_row") scrolled_page.name_property_value_entry = builder.get_object("name_property_value_entry") scrolled_page.name_property_value_entry.set_buffer(HistoryEntryBuffer([])) - value = self.unlocked_database.database_manager.get_entry_name(entry_uuid) - if self.unlocked_database.database_manager.has_entry_name(entry_uuid) is True: - scrolled_page.name_property_value_entry.set_text(value) - else: - scrolled_page.name_property_value_entry.set_text("") - - scrolled_page.name_property_value_entry.connect("changed", self.on_property_value_entry_changed, "name") - properties_list_box.add(scrolled_page.name_property_row) - scrolled_page.name_property_value_entry.grab_focus() - elif scrolled_page.name_property_row: - value = self.unlocked_database.database_manager.get_entry_name( - entry_uuid) - if self.unlocked_database.database_manager.has_entry_name(entry_uuid) is True: - scrolled_page.name_property_value_entry.set_text(value) - else: - scrolled_page.name_property_value_entry.set_text("") - scrolled_page.name_property_value_entry.connect("changed", self.on_property_value_entry_changed, "name") - properties_list_box.add(scrolled_page.name_property_row) + scrolled_page.name_property_value_entry.set_text(entry_name) + scrolled_page.name_property_value_entry.connect( + "changed", self.on_property_value_entry_changed, "name" + ) + properties_list_box.add(scrolled_page.name_property_row) + scrolled_page.name_property_value_entry.grab_focus() if self.unlocked_database.database_manager.has_entry_username(entry_uuid) is True or add_all is True: if scrolled_page.username_property_row is NotImplemented: -- GitLab From f0130566c817d85432f6874f6d1671282f4fdaa9 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 22:31:27 +0100 Subject: [PATCH 05/20] entry_row: Remove unused functions and attributes In our big.kdbx with 1000 entries the construction of 1000 EntryRows takes 27 seconds on my laptop (4.512sec in __init__ and 22 secs in assemble_entry_row invoked by __init__). Remove those pesky unused clas attributed (NotImplemented) that are created during __init__ anyway, so we do not need these as class attribute placeholders. Also remove the 2 totally unused functions set_changed and get_changed. --- passwordsafe/entry_row.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 5b40ea1f1..fe66c6fc3 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -11,13 +11,6 @@ from passwordsafe.color_widget import Color class EntryRow(Gtk.ListBoxRow): - unlocked_database = NotImplemented - database_manager = NotImplemented - entry_uuid = NotImplemented - icon = NotImplemented - label = NotImplemented - password = NotImplemented - changed = False selection_checkbox = NotImplemented checkbox_box = NotImplemented color = NotImplemented @@ -112,12 +105,6 @@ class EntryRow(Gtk.ListBoxRow): def get_type(self): return self.type - def set_changed(self, boolean): - self.changed = boolean - - def get_changed(self): - return self.changed - def on_selection_checkbox_toggled(self, _widget): if self.selection_checkbox.get_active() is True: if self not in self.unlocked_database.selection_ui.entries_selected: -- GitLab From 60a56e10b4d1392946ccc4829bbc58c69849bc2c Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 22:48:28 +0100 Subject: [PATCH 06/20] entry_row: Don't use expensive functions when we can simply access an entry. attribute we don't need the overhead of the helper functions. --- passwordsafe/entry_row.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index fe66c6fc3..7137a04ee 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -27,11 +27,11 @@ class EntryRow(Gtk.ListBoxRow): self.entry_uuid = entry.uuid self.icon: Optional[int] = dbm.get_icon(entry) - self.label = dbm.get_entry_name(entry) - self.password = dbm.get_entry_password(entry) + self.label: str = entry.title or "" + self.password = entry.password # type: Optional[str] self.color = dbm.get_entry_color(entry) - self.username: Optional[str] = dbm.get_entry_username(entry) - if self.username and self.username.startswith("{REF:U"): + self.username: str = entry.username or "" + if self.username.startswith("{REF:U"): # Loopup reference and put in the "real" username uuid = UUID(self.unlocked_database.reference_to_hex_uuid(self.username)) self.username = self.database_manager.get_entry_username(uuid) -- GitLab From 75a88ad63fbe3f41db6b39ce77caf19770b7405c Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 22:50:42 +0100 Subject: [PATCH 07/20] entry_row: remove unused update_password() entryrow.update_password() was never called (and is not useful), so we do not need to query and store the password for every gui label. --- passwordsafe/entry_row.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 7137a04ee..7e287b58d 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -28,7 +28,6 @@ class EntryRow(Gtk.ListBoxRow): self.entry_uuid = entry.uuid self.icon: Optional[int] = dbm.get_icon(entry) self.label: str = entry.title or "" - self.password = entry.password # type: Optional[str] self.color = dbm.get_entry_color(entry) self.username: str = entry.username or "" if self.username.startswith("{REF:U"): @@ -98,10 +97,6 @@ class EntryRow(Gtk.ListBoxRow): def set_label(self, label): self.label = label - def update_password(self): - self.password = self.database_manager.get_entry_password( - self.entry_uuid) - def get_type(self): return self.type -- GitLab From cff518050d3cb016df9c5de47d8eb118b5b40644 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 23:07:59 +0100 Subject: [PATCH 08/20] entry_row: get Gtk.Builder() only once as class attribute, and not per instance initialization. Need to adapt all code to use self.builder instead of builder. --- passwordsafe/entry_row.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 7e287b58d..64965a306 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -11,6 +11,8 @@ from passwordsafe.color_widget import Color class EntryRow(Gtk.ListBoxRow): + builder = Gtk.Builder() + selection_checkbox = NotImplemented checkbox_box = NotImplemented color = NotImplemented @@ -38,17 +40,15 @@ class EntryRow(Gtk.ListBoxRow): self.assemble_entry_row() def assemble_entry_row(self): - builder = Gtk.Builder() - builder.add_from_resource( - "/org/gnome/PasswordSafe/unlocked_database.ui") - entry_event_box = builder.get_object("entry_event_box") + self.builder.add_from_resource("/org/gnome/PasswordSafe/unlocked_database.ui") + entry_event_box = self.builder.get_object("entry_event_box") entry_event_box.connect("button-press-event", self.unlocked_database.on_entry_row_button_pressed) - entry_icon = builder.get_object("entry_icon") - entry_name_label = builder.get_object("entry_name_label") - entry_subtitle_label = builder.get_object("entry_subtitle_label") - entry_copy_button = builder.get_object("entry_copy_button") - entry_color_button = builder.get_object("entry_color_button") + entry_icon = self.builder.get_object("entry_icon") + entry_name_label = self.builder.get_object("entry_name_label") + entry_subtitle_label = self.builder.get_object("entry_subtitle_label") + entry_copy_button = self.builder.get_object("entry_copy_button") + entry_color_button = self.builder.get_object("entry_color_button") # Icon icon_name: str = passwordsafe.icon.get_icon_name(self.icon) @@ -80,8 +80,8 @@ class EntryRow(Gtk.ListBoxRow): self.show_all() # Selection Mode Checkboxes - self.checkbox_box = builder.get_object("entry_checkbox_box") - self.selection_checkbox = builder.get_object("selection_checkbox_entry") + self.checkbox_box = self.builder.get_object("entry_checkbox_box") + self.selection_checkbox = self.builder.get_object("selection_checkbox_entry") self.selection_checkbox.connect("toggled", self.on_selection_checkbox_toggled) if self.unlocked_database.selection_ui.selection_mode_active is True: self.checkbox_box.show_all() -- GitLab From 80867c5913e30d9dd489b8b603ef9c0bdd18d197 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 23:27:34 +0100 Subject: [PATCH 09/20] entry|group_row: Don't wrap selection checkboxes in GtkBoxes Constructing these rows is performance critical and a large part of the time is spent in fetching and constructing those gtk widgets. So simplifying those rows is a performance critical path. Don't wrap and store the selection checkboxes in yet another GtkBox that we explicitely hide when we are not in search mode. Ditch that additional layer and hide the checkboxes directly. Actually we hide them in the glade ui now and only need to explicitely show them when we construct the EntryRow in selection mode. This shaves off a bit of time loading the 1000 entry big.kdbx and has no other side effects (so far :-)). --- data/unlocked_database.ui | 64 +++++++++++------------------------- passwordsafe/entry_row.py | 8 ++--- passwordsafe/group_row.py | 8 ++--- passwordsafe/selection_ui.py | 8 ++--- 4 files changed, 28 insertions(+), 60 deletions(-) diff --git a/data/unlocked_database.ui b/data/unlocked_database.ui index 93341558f..b1ba2b586 100644 --- a/data/unlocked_database.ui +++ b/data/unlocked_database.ui @@ -278,31 +278,17 @@ True False - - True - False - 10 - vertical - - - True - True - False - center - center - True - True - - - False - True - 0 - - + + True + False + center + center + True + True False - True + False 0 @@ -450,30 +436,17 @@ True False - - True - False - 10 - - - True - True - False - center - center - True - True - - - False - True - 0 - - + + True + False + center + center + True + True False - True + False 0 @@ -598,6 +571,9 @@ + + 1 + @@ -1195,7 +1171,7 @@ 2 - + True False diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 64965a306..0c8189419 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -14,7 +14,6 @@ class EntryRow(Gtk.ListBoxRow): builder = Gtk.Builder() selection_checkbox = NotImplemented - checkbox_box = NotImplemented color = NotImplemented type = "EntryRow" @@ -77,16 +76,13 @@ class EntryRow(Gtk.ListBoxRow): image_style.add_class("DarkIcon") self.add(entry_event_box) - self.show_all() + self.show() # Selection Mode Checkboxes - self.checkbox_box = self.builder.get_object("entry_checkbox_box") self.selection_checkbox = self.builder.get_object("selection_checkbox_entry") self.selection_checkbox.connect("toggled", self.on_selection_checkbox_toggled) if self.unlocked_database.selection_ui.selection_mode_active is True: - self.checkbox_box.show_all() - else: - self.checkbox_box.hide() + self.selection_checkbox.show() def get_uuid(self): return self.entry_uuid diff --git a/passwordsafe/group_row.py b/passwordsafe/group_row.py index 9e8854e79..54dcc1ee5 100644 --- a/passwordsafe/group_row.py +++ b/passwordsafe/group_row.py @@ -8,7 +8,6 @@ class GroupRow(Gtk.ListBoxRow): group_uuid = NotImplemented label = NotImplemented selection_checkbox = NotImplemented - checkbox_box = NotImplemented edit_button = NotImplemented type = "GroupRow" @@ -38,16 +37,13 @@ class GroupRow(Gtk.ListBoxRow): group_name_label.set_markup("" + _("No group title specified") + "") self.add(group_event_box) - self.show_all() + self.show() # Selection Mode Checkboxes - self.checkbox_box = builder.get_object("group_checkbox_box") self.selection_checkbox = builder.get_object("selection_checkbox_group") self.selection_checkbox.connect("toggled", self.on_selection_checkbox_toggled) if self.unlocked_database.selection_ui.selection_mode_active is True: - self.checkbox_box.show_all() - else: - self.checkbox_box.hide() + self.selection_checkbox.show() # Edit Button self.edit_button = builder.get_object("group_edit_button") diff --git a/passwordsafe/selection_ui.py b/passwordsafe/selection_ui.py index cce88e794..d930493b1 100644 --- a/passwordsafe/selection_ui.py +++ b/passwordsafe/selection_ui.py @@ -82,8 +82,8 @@ class SelectionUI: list_box = stack_page.get_children()[0].get_children()[0].get_children()[0].get_children()[0] for row in list_box: row.show() - if hasattr(row, "checkbox_box") is True: - row.checkbox_box.hide() + if hasattr(row, "selection_checkbox"): + row.selection_checkbox.hide() row.selection_checkbox.set_active(False) if hasattr(row, "edit_button") is True: row.edit_button.show_all() @@ -127,8 +127,8 @@ class SelectionUI: if stack_page.check_is_edit_page() is False: list_box = stack_page.get_children()[0].get_children()[0].get_children()[0].get_children()[0] for row in list_box: - if hasattr(row, "checkbox_box") is True: - row.checkbox_box.show() + if hasattr(row, "selection_checkbox"): + row.selection_checkbox.show() if hasattr(row, "edit_button") is True: row.edit_button.hide() -- GitLab From b9af03ca7da9d3cd1e3e1e89aec35349a6852d1e Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 12 Nov 2020 23:56:56 +0100 Subject: [PATCH 10/20] entry_row: Only fudge with icon classes if color is set Give all entry_row icons a DarkIcon class by default in the glade ui and only remove it and add class "BrightIcon" if a color has been set. This saves us from dynamically fudging with most widgets after their construction. It does not save loads of time, but a little. --- data/unlocked_database.ui | 1 + passwordsafe/entry_row.py | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data/unlocked_database.ui b/data/unlocked_database.ui index b1ba2b586..9ff3ded63 100644 --- a/data/unlocked_database.ui +++ b/data/unlocked_database.ui @@ -316,6 +316,7 @@ diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 0c8189419..3f533de82 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -71,9 +71,8 @@ class EntryRow(Gtk.ListBoxRow): image = entry_color_button.get_children()[0] image_style = image.get_style_context() if self.color != Color.NONE.value: + image_style.remove_class("DarkIcon") image_style.add_class("BrightIcon") - else: - image_style.add_class("DarkIcon") self.add(entry_event_box) self.show() -- GitLab From 57d6a533c390198dd1d8ae5ee71572a33dfbbe7b Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 00:09:42 +0100 Subject: [PATCH 11/20] entry_row: remove unused class attributes .targets was completely unused and .color is populated during __init__ so there is no need to keep a class attribute as fallback. --- passwordsafe/entry_row.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 3f533de82..f7a7c8db6 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -14,11 +14,8 @@ class EntryRow(Gtk.ListBoxRow): builder = Gtk.Builder() selection_checkbox = NotImplemented - color = NotImplemented type = "EntryRow" - targets = NotImplemented - def __init__(self, unlocked_database, dbm, entry): Gtk.ListBoxRow.__init__(self) self.set_name("EntryRow") -- GitLab From 4420fdc6d8672998b3ee29c758b82cc30c567d7b Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 00:14:35 +0100 Subject: [PATCH 12/20] EntryRow: Don't pass in DatabaseManager as parameter We pass in UnlockedDatabase which contains the DatabaseManager that we need so there is no need to pass it as a separate parameter. Using the opportunity to annotate and rename the unwieldy database_manager to db_manager within that class. --- passwordsafe/entry_row.py | 22 ++++++++++++++-------- passwordsafe/search.py | 7 ++++++- passwordsafe/unlocked_database.py | 2 +- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index f7a7c8db6..1aa82b5eb 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-3.0-only from __future__ import annotations +import typing from gettext import gettext as _ from uuid import UUID from typing import Optional @@ -9,29 +10,32 @@ import passwordsafe.config_manager import passwordsafe.icon from passwordsafe.color_widget import Color +if typing.TYPE_CHECKING: + from pykeepass.entry import Entry + from passwordsafe.unlocked_database import UnlockedDatabase + class EntryRow(Gtk.ListBoxRow): builder = Gtk.Builder() - selection_checkbox = NotImplemented type = "EntryRow" - def __init__(self, unlocked_database, dbm, entry): + def __init__(self, database: UnlockedDatabase, entry: Entry) -> None: Gtk.ListBoxRow.__init__(self) self.set_name("EntryRow") - self.unlocked_database = unlocked_database - self.database_manager = dbm + self.unlocked_database = database + self.db_manager = database.database_manager self.entry_uuid = entry.uuid - self.icon: Optional[int] = dbm.get_icon(entry) + self.icon: Optional[int] = self.db_manager.get_icon(entry) self.label: str = entry.title or "" - self.color = dbm.get_entry_color(entry) + self.color = self.db_manager.get_entry_color(entry) self.username: str = entry.username or "" if self.username.startswith("{REF:U"): # Loopup reference and put in the "real" username uuid = UUID(self.unlocked_database.reference_to_hex_uuid(self.username)) - self.username = self.database_manager.get_entry_username(uuid) + self.username = self.db_manager.get_entry_username(uuid) self.assemble_entry_row() @@ -115,7 +119,9 @@ class EntryRow(Gtk.ListBoxRow): # self.unlocked_database.selection_ui.cut_mode is True def on_entry_copy_button_clicked(self, _button): - self.unlocked_database.send_to_clipboard(self.database_manager.get_entry_password(self.entry_uuid)) + self.unlocked_database.send_to_clipboard( + self.db_manager.get_entry_password(self.entry_uuid) + ) def update_color(self, color): self.color = color diff --git a/passwordsafe/search.py b/passwordsafe/search.py index 46df8c1f7..86cc87cc5 100644 --- a/passwordsafe/search.py +++ b/passwordsafe/search.py @@ -188,7 +188,12 @@ class Search: search_height += entry_row_height if skip is False: - row = EntryRow(self.unlocked_database, self.unlocked_database.database_manager, self.unlocked_database.database_manager.get_entry_object_from_uuid(uuid)) + row = EntryRow( + self.unlocked_database, + self.unlocked_database.database_manager.get_entry_object_from_uuid( + uuid + ), + ) self.search_list_box.add(row) self.cached_rows.append(row) else: diff --git a/passwordsafe/unlocked_database.py b/passwordsafe/unlocked_database.py index 1d9d1c845..76c1c3756 100644 --- a/passwordsafe/unlocked_database.py +++ b/passwordsafe/unlocked_database.py @@ -440,7 +440,7 @@ class UnlockedDatabase(GObject.GObject): def entry_instance_creation(self, list_box, sorted_list, entries, overlay): for entry in entries: - entry_row = EntryRow(self, self.database_manager, entry) + entry_row = EntryRow(self, entry) sorted_list.append(entry_row) if self.list_box_sorting == "A-Z": -- GitLab From 430966371fcc3df074cf921e50ea97cd2c6a1dc8 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 00:41:19 +0100 Subject: [PATCH 13/20] unlocked_database: ditch get_entries_in_folder(self.current_element.uuid) get_entries_in_folder(self.current_element.uuid) was passing in a UUID, looking up the group, and returning the group's.entries. This happens everytime we create a new stack, ie display a group, so it is worth optimizing. As this is THE ONLY consumer of this function, ditch it and simply use group.entries directly. --- passwordsafe/database_manager.py | 5 ----- passwordsafe/unlocked_database.py | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/passwordsafe/database_manager.py b/passwordsafe/database_manager.py index 7ab3abc8d..c23022db7 100644 --- a/passwordsafe/database_manager.py +++ b/passwordsafe/database_manager.py @@ -645,11 +645,6 @@ class DatabaseManager(GObject.GObject): folder = self.get_group(uuid) return folder.subgroups - def get_entries_in_folder(self, uuid): - """Return list of all entries in a group""" - parent_group = self.get_group(uuid) - return parent_group.entries - # Return the root group of the database instance def get_root_group(self): return self.db.root_group diff --git a/passwordsafe/unlocked_database.py b/passwordsafe/unlocked_database.py index 76c1c3756..e40e6c66c 100644 --- a/passwordsafe/unlocked_database.py +++ b/passwordsafe/unlocked_database.py @@ -420,7 +420,7 @@ class UnlockedDatabase(GObject.GObject): self.insert_entries_into_listbox(list_box, overlay) def insert_entries_into_listbox(self, list_box, overlay): - entries = self.database_manager.get_entries_in_folder(self.current_element.uuid) + entries = self.current_element.entries sorted_list = [] GLib.idle_add(self.entry_instance_creation, list_box, sorted_list, entries, overlay) -- GitLab From ee64c5261efd0d4e384ad9f70e28fb1b39a945f7 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 00:58:41 +0100 Subject: [PATCH 14/20] unlocked_database: Ditch get_groups_in_folder() When creating a new stack, ie displaying a group we would do a get_groups_in_folder(current_element.uuid) which would look up the current group, ie current_element and return its subgroups. As this was THE ONLY consumer of that function, drop it and directly do current_group.subgroups. Also there is no need to treat the root group any different from other groups, so we can drop the special treatment too. As that is THE ONLY consumer of get_groups_in_root(), drop it too. --- passwordsafe/database_manager.py | 8 -------- passwordsafe/unlocked_database.py | 6 +----- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/passwordsafe/database_manager.py b/passwordsafe/database_manager.py index c23022db7..87e92444f 100644 --- a/passwordsafe/database_manager.py +++ b/passwordsafe/database_manager.py @@ -637,14 +637,6 @@ class DatabaseManager(GObject.GObject): # Read Database # - def get_groups_in_root(self): - return self.db.root_group.subgroups - - def get_groups_in_folder(self, uuid): - """Return list of all subgroups in a group""" - folder = self.get_group(uuid) - return folder.subgroups - # Return the root group of the database instance def get_root_group(self): return self.db.root_group diff --git a/passwordsafe/unlocked_database.py b/passwordsafe/unlocked_database.py index e40e6c66c..cd87657df 100644 --- a/passwordsafe/unlocked_database.py +++ b/passwordsafe/unlocked_database.py @@ -410,11 +410,7 @@ class UnlockedDatabase(GObject.GObject): add_loading_indicator_thread = threading.Thread(target=self.add_loading_indicator_thread, args=(list_box, overlay)) add_loading_indicator_thread.start() - if self.current_element.is_root_group: - groups = self.database_manager.get_groups_in_root() - else: - groups = self.database_manager.get_groups_in_folder(self.current_element.uuid) - + groups = self.current_element.subgroups GLib.idle_add(self.group_instance_creation, list_box, sorted_list, groups) self.insert_entries_into_listbox(list_box, overlay) -- GitLab From 664d9f7aae870209e10179276194c1d8c63bd487 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 01:43:26 +0100 Subject: [PATCH 15/20] Put EntryRow's glade data in separate .ui file, makes creating EntryRow MUCH faster It turns out the big culprit is that we add_resources using the HUGE unlocked_database.ui for each label. It contains lots of stuff which we do not want to create a 1000 times. Just putting the entry_row stuff in its own dedicated resource files and adding that a 1000 times is MUCH faster. Also do NOT compress the .ui file in our gresources file, it small and used often, so uncompressing this thing cost us more than it brings. (we should examine if the same holds true for the other .ui files there) This commit makes it go from around 25 seconds to <3.5 seconds for creating 1000 entries in the gui. --- data/entry_row.ui | 152 ++++++++++++++++++++++++++++++++ data/passwordsafe.gresource.xml | 1 + data/unlocked_database.ui | 147 ------------------------------ passwordsafe/entry_row.py | 2 +- 4 files changed, 154 insertions(+), 148 deletions(-) create mode 100644 data/entry_row.ui diff --git a/data/entry_row.ui b/data/entry_row.ui new file mode 100644 index 000000000..cc659471c --- /dev/null +++ b/data/entry_row.ui @@ -0,0 +1,152 @@ + + + + + + 1 + True + False + baseline + + + 60 + True + False + + + True + False + center + center + True + True + + + False + False + 0 + + + + + 32 + 32 + True + False + False + False + False + center + center + 10 + 10 + none + + + True + False + 16 + application-x-executable-symbolic + + + + + + False + False + 1 + + + + + True + False + vertical + + + name_label + True + False + start + end + False + True + Title not specified + end + + + False + True + 0 + + + + + True + False + True + vertical + + + entry_subtitle_label + True + False + start + start + False + True + No username specified + end + + + False + True + 0 + + + + + False + True + 1 + + + + + False + True + 2 + + + + + ListBoxRowButton + True + True + True + end + center + 5 + 8 + + + True + False + edit-copy-symbolic + + + + + False + False + end + 3 + + + + + + diff --git a/data/passwordsafe.gresource.xml b/data/passwordsafe.gresource.xml index eaacb4cc9..52ae13ce9 100644 --- a/data/passwordsafe.gresource.xml +++ b/data/passwordsafe.gresource.xml @@ -11,6 +11,7 @@ created_database.ui database_settings_dialog.ui entry_page.ui + entry_row.ui group_page.ui unlock_database.ui unlocked_database.ui diff --git a/data/unlocked_database.ui b/data/unlocked_database.ui index 9ff3ded63..bf5a82429 100644 --- a/data/unlocked_database.ui +++ b/data/unlocked_database.ui @@ -267,153 +267,6 @@ - - 1 - True - False - baseline - - - 60 - True - False - - - True - False - center - center - True - True - - - False - False - 0 - - - - - 32 - 32 - True - False - False - False - False - center - center - 10 - 10 - none - - - True - False - 16 - application-x-executable-symbolic - - - - - - False - False - 1 - - - - - True - False - vertical - - - name_label - True - False - start - end - False - True - Title not specified - end - - - False - True - 0 - - - - - True - False - True - vertical - - - entry_subtitle_label - True - False - start - start - False - True - No username specified - end - - - False - True - 0 - - - - - False - True - 1 - - - - - False - True - 2 - - - - - ListBoxRowButton - True - True - True - end - center - 5 - 8 - - - True - False - edit-copy-symbolic - - - - - False - False - end - 3 - - - - - True False diff --git a/passwordsafe/entry_row.py b/passwordsafe/entry_row.py index 1aa82b5eb..0d6b5df88 100644 --- a/passwordsafe/entry_row.py +++ b/passwordsafe/entry_row.py @@ -40,7 +40,7 @@ class EntryRow(Gtk.ListBoxRow): self.assemble_entry_row() def assemble_entry_row(self): - self.builder.add_from_resource("/org/gnome/PasswordSafe/unlocked_database.ui") + self.builder.add_from_resource("/org/gnome/PasswordSafe/entry_row.ui") entry_event_box = self.builder.get_object("entry_event_box") entry_event_box.connect("button-press-event", self.unlocked_database.on_entry_row_button_pressed) -- GitLab From 8b048bd4c09c85d1de4eafbe60bb274d63c6065e Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 20:20:36 +0100 Subject: [PATCH 16/20] unlocked_db: destroy_scheduled_stack_page -> destroy_current_page_if_scheduled() Rename the function to make clear what it actually does. And remove the two invokations of this function right after each other. It does not make sense to call this twice in a row. --- passwordsafe/unlocked_database.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/passwordsafe/unlocked_database.py b/passwordsafe/unlocked_database.py index cd87657df..a94923998 100644 --- a/passwordsafe/unlocked_database.py +++ b/passwordsafe/unlocked_database.py @@ -218,12 +218,10 @@ class UnlockedDatabase(GObject.GObject): def show_page_of_new_directory(self, edit_group, new_entry): # First, remove stack pages which should not exist because they are scheduled for remove - self.destroy_scheduled_stack_page() + self.destroy_current_page_if_scheduled() # Creation of group edit page if edit_group is True: - self.destroy_scheduled_stack_page() - builder = Gtk.Builder() builder.add_from_resource("/org/gnome/PasswordSafe/group_page.ui") scrolled_window = ScrolledPage(True) @@ -390,7 +388,8 @@ class UnlockedDatabase(GObject.GObject): def schedule_stack_page_for_destroy(self, page_name): self.scheduled_page_destroy.append(page_name) - def destroy_scheduled_stack_page(self): + def destroy_current_page_if_scheduled(self) -> None: + """If the current_element is in self.scheduled_page_destroy, destroy it""" page_uuid = self.current_element.uuid if page_uuid in self.scheduled_page_destroy: -- GitLab From 69a4b01658adff0cee7ba8ef0c0c9b8e2f5e78a8 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 20:43:08 +0100 Subject: [PATCH 17/20] Annotate schedule_stack_page_for_destroy() While I was trying to understand the code, and the type of parameters passed in, I annotated this function (and clarified a variable name to make clear we are passing an UUID in). --- passwordsafe/pathbar.py | 15 ++++++++++----- passwordsafe/unlocked_database.py | 4 ++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/passwordsafe/pathbar.py b/passwordsafe/pathbar.py index 8914d24d6..63448d885 100644 --- a/passwordsafe/pathbar.py +++ b/passwordsafe/pathbar.py @@ -216,16 +216,16 @@ class Pathbar(Gtk.HBox): if self.database_manager.check_is_root_group(update_group) is True: update_group = self.database_manager.get_root_group() - page_name = update_group.uuid - self.unlocked_database.schedule_stack_page_for_destroy(page_name) + page_uuid = update_group.uuid + self.unlocked_database.schedule_stack_page_for_destroy(page_uuid) self.page_update_queried() else: if self.check_is_edit_page_from_group() is False: return - edit_page = self.unlocked_database.current_element - self.unlocked_database.schedule_stack_page_for_destroy(edit_page.uuid) + edited_uuid = self.unlocked_database.current_element.uuid + self.unlocked_database.schedule_stack_page_for_destroy(edited_uuid) def page_update_queried(self): """Marks the curent page as not dirty""" @@ -233,7 +233,12 @@ class Pathbar(Gtk.HBox): page.is_dirty = False def check_values_of_edit_page(self, parent_group: Group) -> bool: - """Check all values of the group/entry - if all are blank we delete the entry/group and return true""" + """Check all values of the current group/entry which we finished editing + + If all are blank we delete the entry/group and return True. + It also schedules the parent page for destruction if the + Entry/Group has been deleted. + """ current_elt = self.unlocked_database.current_element notes = self.database_manager.get_notes(current_elt) icon = self.database_manager.get_icon(current_elt) diff --git a/passwordsafe/unlocked_database.py b/passwordsafe/unlocked_database.py index a94923998..9a6df62af 100644 --- a/passwordsafe/unlocked_database.py +++ b/passwordsafe/unlocked_database.py @@ -385,8 +385,8 @@ class UnlockedDatabase(GObject.GObject): """ return self._stack.get_children() - def schedule_stack_page_for_destroy(self, page_name): - self.scheduled_page_destroy.append(page_name) + def schedule_stack_page_for_destroy(self, page_uuid: UUID) -> None: + self.scheduled_page_destroy.append(page_uuid) def destroy_current_page_if_scheduled(self) -> None: """If the current_element is in self.scheduled_page_destroy, destroy it""" -- GitLab From 374f72c41ca9ed97c2ed87577ff116a17b301301 Mon Sep 17 00:00:00 2001 From: Maximiliano Sandoval R Date: Fri, 13 Nov 2020 20:23:25 +0100 Subject: [PATCH 18/20] entry_row.ui: Remove useless packing properties This makes the file loaded 1000 times 1/5 smaller. Also packing is deprecated in gtk4. --- data/entry_row.ui | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/data/entry_row.ui b/data/entry_row.ui index cc659471c..f27e7e1e0 100644 --- a/data/entry_row.ui +++ b/data/entry_row.ui @@ -21,11 +21,6 @@ True True - - False - False - 0 - @@ -54,11 +49,6 @@ - - False - False - 1 - @@ -77,11 +67,6 @@ Title not specified end - - False - True - 0 - @@ -101,25 +86,10 @@ No username specified end - - False - True - 0 - - - False - True - 1 - - - False - True - 2 - @@ -140,10 +110,7 @@ - False - False end - 3 -- GitLab From b03a9225426629e8d8256208cbddfab531df9d22 Mon Sep 17 00:00:00 2001 From: Maximiliano Sandoval R Date: Fri, 13 Nov 2020 20:39:07 +0100 Subject: [PATCH 19/20] group/entry_row.ui: Give checkboxes margin also remove useless width_request --- data/entry_row.ui | 2 +- data/unlocked_database.ui | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/data/entry_row.ui b/data/entry_row.ui index f27e7e1e0..0462dfa1f 100644 --- a/data/entry_row.ui +++ b/data/entry_row.ui @@ -3,7 +3,6 @@ - 1 True False baseline @@ -20,6 +19,7 @@ center True True + 8 diff --git a/data/unlocked_database.ui b/data/unlocked_database.ui index bf5a82429..8bf4c4606 100644 --- a/data/unlocked_database.ui +++ b/data/unlocked_database.ui @@ -280,7 +280,6 @@ - 1 True False baseline @@ -297,6 +296,7 @@ center True True + 8 False @@ -336,7 +336,6 @@ - 1 True False True -- GitLab From 6f5c5e23bacd246887ec4100a903b596639ef0aa Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Fri, 13 Nov 2020 21:35:00 +0100 Subject: [PATCH 20/20] Add debug messages about which pages are being destroyed At least those that are scheduled for destruction or destructed. --- passwordsafe/unlocked_database.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/passwordsafe/unlocked_database.py b/passwordsafe/unlocked_database.py index 9a6df62af..dc814ec3f 100644 --- a/passwordsafe/unlocked_database.py +++ b/passwordsafe/unlocked_database.py @@ -386,13 +386,16 @@ class UnlockedDatabase(GObject.GObject): return self._stack.get_children() def schedule_stack_page_for_destroy(self, page_uuid: UUID) -> None: + """Add page to the list of pages to be destroyed""" + logging.debug("Scheduling page %s for destruction") self.scheduled_page_destroy.append(page_uuid) def destroy_current_page_if_scheduled(self) -> None: """If the current_element is in self.scheduled_page_destroy, destroy it""" page_uuid = self.current_element.uuid - + logging.debug("Test if we should destroy page %s", page_uuid) if page_uuid in self.scheduled_page_destroy: + logging.debug("Yes, destroying page %s", page_uuid) stack_page_name = self._stack.get_child_by_name(page_uuid.urn) if stack_page_name is not None: stack_page_name.destroy() -- GitLab