GNOME Bugzilla – Bug 769582
Miscellaneous cleanups
Last modified: 2016-08-29 23:27:51 UTC
I've been doing some restructuring lately to prepare for future features, and a couple of unrelated changes fell out of it as side product. Those are the patches that don't fix anything, but just make the code a bit nicer.
Created attachment 332852 [details] [review] accountsMonitor: Store accounts in a Map Accounts are serialized for actions and settings, however we currently have no good way to deserialize them, falling back to TpSimpleClientFactory.ensure_account(). Storing accounts in a Map instead of an array will allow us to provide a lookup function, besides making the accounts monitor code itself slightly cleaner.
Created attachment 332853 [details] [review] accountsMonitor: Change dupAccounts() to a getter Other modules have no business in modifying the monitor's internal account map, which is why we expose a method to get a copied list of accounts rather than the raw property. A getter function works just the same way, but reads just a tad bit nicer ...
Created attachment 332854 [details] [review] accountsMonitor: Add enabledAccounts convenience getter It is common enough to filter out disabled accounts to add a convenience getter function that takes care of the filtering.
Created attachment 332855 [details] [review] accountsMonitor: Provide lookup function We are currently using TpSimpleClientFactory.ensure_account() when we need to look up accounts, which is slightly dodgy as it bypasses the account filtering in AccountsMonitor. To address this, add a small wrapper around Map.get().
Created attachment 332856 [details] [review] accountsMonitor: Mirror ::account-enabled/disabled signals To get notified about accounts being enabled or disabled rather than added/removed, we currently use the Tp.AccountManager signals directly. That's not terrible, but having the signals on the AccountsMonitor instead is a bit more convenient, and means we only get notifications about accounts we actually care about (that is, no GTalk/Jabber or other non-IRC ones).
Created attachment 332857 [details] [review] accountsMonitor: Minor cleanup Use arrow notation to make the shutdown handling code more concise.
Created attachment 332858 [details] [review] chatroomManager: Simplify actions code a bit Save some code repetition by iterating over an array of ActionEntry-like objects to look up actions and connect to their ::activate signal.
Created attachment 332859 [details] [review] roomList: Simplify actions code a bit We have several handlers that select a specific row by index, so split out some shared code into a helper method. We can also save on some code repetition by iterating over an array of ActionEntry-like objects to look up actions and connect to their ::activate signal.
Created attachment 332860 [details] [review] roomList: Use Map to keep track of rows/placeholders With the availability of the ECMA6 Map class in gjs, it's time to phase out the old pattern of using objects as hash tables ...
Created attachment 332861 [details] [review] roomStack: Store rooms in a Map With the availability of the ECMA6 Map class in gjs, it's time to phase out the old pattern of using objects as hash tables ...
Created attachment 332862 [details] [review] app: Store pending requests in a Map With the availability of the ECMA6 Map class in gjs, it's time to phase out the old pattern of using objects as hash tables ...
Also pushed as branch: https://git.gnome.org/browse/polari/log/?h=wip/fmuellner/misc-cleanups
Review of attachment 332852 [details] [review]: looks good to me.
Review of attachment 332853 [details] [review]: looks good to me.
Review of attachment 332854 [details] [review]: looks good to me.
Created attachment 332912 [details] [review] accountsMonitor: Provide lookup function Found during a chat on IRC: The check for (!account.enabled && !account.has_been_online) is no longer correct to test for accounts that were removed - lookupAccount() will simply return null in that case.
Review of attachment 332912 [details] [review]: looks good to me.
Review of attachment 332856 [details] [review]: looks good to me.
Review of attachment 332858 [details] [review]: looks good to me.
Review of attachment 332859 [details] [review]: looks good to me.
Review of attachment 332860 [details] [review]: looks good to me.
Review of attachment 332861 [details] [review]: looks good to me.
Comment on attachment 332862 [details] [review] app: Store pending requests in a Map + if (this._pendingRequests.get(roomId)) { + this._pendingRequests.get(roomId).cancel(); Could the condition use the Map.has() method? I know it works just fine as it is, i just think that the .has() method is a bit more efficient in this case.
Review of attachment 332857 [details] [review]: looks good to me.
Review of attachment 332862 [details] [review]: looks good to me.
Attachment 332852 [details] pushed as 18b4314 - accountsMonitor: Store accounts in a Map Attachment 332853 [details] pushed as 967ac58 - accountsMonitor: Change dupAccounts() to a getter Attachment 332854 [details] pushed as b1dd010 - accountsMonitor: Add enabledAccounts convenience getter Attachment 332856 [details] pushed as fd93968 - accountsMonitor: Mirror ::account-enabled/disabled signals Attachment 332857 [details] pushed as 6754bd8 - accountsMonitor: Minor cleanup Attachment 332858 [details] pushed as 24661cb - chatroomManager: Simplify actions code a bit Attachment 332859 [details] pushed as ed181f4 - roomList: Simplify actions code a bit Attachment 332860 [details] pushed as 5f346d5 - roomList: Use Map to keep track of rows/placeholders Attachment 332861 [details] pushed as 3ab2517 - roomStack: Store rooms in a Map Attachment 332862 [details] pushed as 634e6ff - app: Store pending requests in a Map Attachment 332912 [details] pushed as 7dc1eaf - accountsMonitor: Provide lookup function