Closed Bug 809066 Opened 12 years ago Closed 6 years ago

In the folder picker, generate the Recent menu only if opened.

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 66.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 7 obsolete files)

part 1 - cleanup v6 [checked in: comment 22]
(deleted), patch
aceman
: review+
Details | Diff | Splinter Review
809066-2.patch real change v1.1
(deleted), patch
aceman
: review+
Details | Diff | Splinter Review
The main folder picker menu is optimized that it is only built when being shown, unless it is a child of a menulist. It then iterates over all accounts/folders to build itself. Similarly, if the Recent item is to be created, it must iterate over all accounts/folders again. But this is done when the whole menu is built. This can be optimized further, to build the Recent submenu only when it is really to be shown (some users may choose to always skip this item). There are some further optimizations and cleanups possible in the code. There should not be any changes in appearance.
Status: NEW → ASSIGNED
Depends on: 808974
Attached patch patch (obsolete) (deleted) — — Splinter Review
To be applied on top of patch from bug 808974.
Attachment #678827 - Flags: review?(neil)
Blocks: 546722
(In reply to aceman from comment #0) > There are some further optimizations and cleanups possible in the code. And it's not clear which of the changes are just "optimizations and cleanups" and which are the real changes that this bug is about. Please can you split the patch into two?
Attached patch part 1 - cleanup (obsolete) (deleted) — — Splinter Review
Sure, here is the first part.
Attachment #678827 - Attachment is obsolete: true
Attachment #678827 - Flags: review?(neil)
Attachment #680219 - Flags: review?(neil)
Neil, what about replacing the recursive function checkRecentSubFolders() with just iterating over the accountmanager.allFolders array? That may even be faster.
Sounds like a plan. (I see nobody ever followed up on bug 305340 comment 17 to make ListDescendants use an nsIMutableArray though...)
How hard is that?
No idea, I only noticed it because I looked up the history of allFolders.
Attachment #680219 - Attachment is obsolete: true
Attachment #680219 - Flags: review?(neil)
Attached patch part 1 - cleanup v2 (obsolete) (deleted) — — Splinter Review
Attachment #684096 - Flags: review?(neil)
Comment on attachment 684096 [details] [diff] [review] part 1 - cleanup v2 It's not cleanup any more if you're changing stuff! (Sorry for taking so long to do the review though.)
Attachment #684096 - Flags: review?(neil)
It is not changing stuff, just using mode modern constructs :) There should be no functional change in this patch. The changing patch is not yet attached. So what should I do now?
I wanted to get the code moves / variable renames / comment changes (i.e. really trivial changes) out of the way, so that they didn't conflict with the real code changes. Unfortunately I can't seem to get attachment 680219 [details] [diff] [review] to apply, it must have bitrotted :s Do you want me to find an old tree, or do you have an updated patch handy?
Attachment 684096 [details] [diff] is the new one. The old attachment 680219 [details] [diff] [review] is bitrotted because of bug 808974 landing. They both should differ only in the comment 4 plus one more fixIterator conversion.
(In reply to aceman from comment #12) > The old attachment 680219 [details] [diff] [review] is bitrotted because of bug 808974 landing. But comment 1 says to be applied on top of patch from bug 808974...
Comment on attachment 684096 [details] [diff] [review] part 1 - cleanup v2 >+ for each (let folder in this.fixIterator(this._parentFolder.subFolders, >+ Ci.nsIMsgFolder)) { >+ folders.push(folder); > } Don't we have a way to go straight from an nsIArray to a JavaScript array? In the worst case you could write folders = [folder in this.fixIterator(...)]; (Note that folders was set earlier to [] but that was only so the folders could be pushed in the loop.) > /** >- * This function will iterate through any existing sub-folders and >- * (1) check if they're recent and (2) recursively call this function >- * to iterate through any sub-sub-folders. >+ * This function will add a folder to the recentFolders array if it >+ * is among the 15 (_MAXRECENT) most recent. If we exceed 15 folders, >+ * it will pop the oldest folder, ensuring that we end up with the >+ * right number. > * > * @param aFolder the folder to check > */ Bah, you really know how to confuse hg diff don't you? > // Because we're scanning across multiple accounts, we can end up with >+ // two folders with the same name. In this case, we append the name > // of the account as well, to distingiush. >- var dupeNames = []; [Whoa, why isn't this an object, or better still, a Map?] >- return aLabel.toLocaleLowerCase() > bLabel.toLocaleLowerCase(); Eek! That's not how you sort strings, you use .localeCompare! >+ recentFolders.sort(function sort_folders_by_name(a, b){ >+ return unique_folder_name(a).toLocaleLowerCase() > >+ unique_folder_name(b).toLocaleLowerCase(); [I guess there are only 15 folders, so you're not going to be calling unique_folder_name that often. In theory though you could compare on the pretty name and only when that fails do you compare on the server name, so you don't actually have to build unique names every time.]
Attached patch part 1 - cleanup v3 (obsolete) (deleted) — — Splinter Review
Should I really use Map while it is experimental? https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Map
Attachment #684096 - Attachment is obsolete: true
Attachment #686229 - Flags: review?(neil)
Attached patch part 1 - cleanup v4 (obsolete) (deleted) — — Splinter Review
OK, so use Set.
Attachment #686229 - Attachment is obsolete: true
Attachment #686229 - Flags: review?(neil)
Attachment #686310 - Flags: review?(neil)
Comment on attachment 686310 [details] [diff] [review] part 1 - cleanup v4 >+ recentFolders.push({folder: aFolder, label: ""}); Nit: no need to set a dummy label here, you're going to compute all the labels eventually anyway. r=me with that fixed. >- var dupeNames = []; >+ let dupeNames = new Set(); You're right, this is a set, not a map. >+ let label = folderItem.folder.prettyName; >+ if (dupeNames.has(label)) >+ label += " - " + folderItem.folder.server.prettyName; >+ >+ folderItem.label = label; >+ } >+ >+ // Make sure the entries are sorted alphabetically. >+ recentFolders.sort(function sort_folders_by_name(a, b) { >+ return a.label.localeCompare(b.label); >+ }); This way works too. For posterity, the code I was thinking of would have looked like this: recentFolders.sort(function sort_folders_by_name(a, b) { return a.prettyName.localeCompare(b.prettyName) || a.server.prettyName.localeCompare(b.server.prettyName); });
Attachment #686310 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #17) > For posterity, the code I was thinking of would have looked like this: > recentFolders.sort(function sort_folders_by_name(a, b) { > return a.prettyName.localeCompare(b.prettyName) || > a.server.prettyName.localeCompare(b.server.prettyName); > }); Yes, but that almost duplicates the logic of constructing the label. And I want to avoid such things when possible.
Attached patch part 1 - cleanup v5 (obsolete) (deleted) — — Splinter Review
Attachment #686310 - Attachment is obsolete: true
Attachment #687505 - Flags: review+
Now, due to bug 546722 comment 7 I am not sure we actually to postpone the creation of the submenu. We would need to iterate over all the folders 3 times (once for the real folder list, once for Recent, once for Favorites) each at different times. I wonder if we shouldn't iterate only once for the folder list and collect all the needed data. Then building of the Recent and Favorites menu would not be expensive (only populate the popup).
Keywords: perf
But the cleanup part can be checked in standalone even if the second part is undecided.
Attachment #687505 - Attachment is obsolete: true
Attachment #708228 - Flags: review+
Keywords: checkin-needed
Whiteboard: [leave bug open after checkin]
Keywords: perf
Whiteboard: [leave bug open after checkin] → [leave open]
Comment on attachment 708228 [details] [diff] [review] part 1 - cleanup v6 [checked in: comment 22] https://hg.mozilla.org/comm-central/rev/45ffe0281169
Attachment #708228 - Attachment description: part 1 - cleanup v6 [ready for checkin] → part 1 - cleanup v6 [checked in: comment 22]
Depends on: 840591
(In reply to :aceman from comment #20) > Now, due to bug 546722 comment 7 I am not sure we actually to postpone the > creation of the submenu. We would need to iterate over all the folders 3 > times (once for the real folder list, once for Recent, once for Favorites) > each at different times. > > I wonder if we shouldn't iterate only once for the folder list and collect > all the needed data. Then building of the Recent and Favorites menu would > not be expensive (only populate the popup). But when the user has many accounts, collecting the Recent and Favorite folders takes a long time if we do it immediatelly. So doing it only when the user really enters the Recent or Favorite menu should be a win, or at least it is clearly visible to the user which part is slow. Building the first level of folder picker should be fast and it does not need to iterate over all folders in all accounts.
Attached patch 809066-2.patch (obsolete) (deleted) — — Splinter Review
This could do it and also merges the Recent and Favorites menus into common code. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0a8932722b5bf58e5ff545d702909442c7f5b8b7
Attachment #9034592 - Flags: review?(jorgk)
Attachment #9034592 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9034592 [details] [diff] [review] 809066-2.patch Not an expert there, sorry.
Attachment #9034592 - Flags: review?(jorgk)
Comment on attachment 9034592 [details] [diff] [review] 809066-2.patch Review of attachment 9034592 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/test/mozmill/folder-display/test-recent-menu.js @@ +48,5 @@ > right_click_on_row(0); > let popups = mc.click_menus_in_sequence(mc.e("mailContext"), > + [{id: "mailContext-moveMenu"}, > + {label: "Recent"}], true); > + let recentMenu = popups[popups.length - 1].parentNode; why this change? Seems less readable to me ::: mailnews/base/content/folderWidgets.xml @@ +199,5 @@ > <field name="_listener"> > <![CDATA[({ > _menu: this, > + _clearMenu(menu) { > + //xxx I'm not quite sure why this isn't always a function did you just move the code, or do you actually see any such problems? If you don't see it, we could remove this guard. @@ +604,5 @@ > + <method name="_buildSpecialMenu"> > + <parameter name="type"/> > + <body><![CDATA[ > + // Now create the Recent folder and its children > + var menu = document.createElement("menu"); can you make all the vars lets? @@ +646,3 @@ > > + switch (specialType) { > + case "recent": please indent cases
Attachment #9034592 - Flags: review?(mkmelin+mozilla) → review+

(In reply to Magnus Melin [:mkmelin] from comment #26)

let popups = mc.click_menus_in_sequence(mc.e("mailContext"),

  •                                      [{id: "mailContext-moveMenu"},

  •                                       {label: "Recent"}], true);
  • let recentMenu = popups[popups.length - 1].parentNode;

why this change? Seems less readable to me

Ok.

::: mailnews/base/content/folderWidgets.xml

  •    _clearMenu(menu) {

  •      //xxx I'm not quite sure why this isn't always a function

did you just move the code, or do you actually see any such problems? If you
don't see it, we could remove this guard.

I just move it and there is bug 514445 for it so we can investigate it in the future.

@@ +604,5 @@

  •  <method name="_buildSpecialMenu">

  •    <parameter name="type"/>

  •    <body><![CDATA[

  •      // Now create the Recent folder and its children

  •      var menu = document.createElement("menu");

can you make all the vars lets?

I thought 'var' and 'let' is the same in top level of a function, but no problem, I'll switch them.

Attached patch 809066-2.patch real change v1.1 (deleted) — — Splinter Review
Attachment #9034592 - Attachment is obsolete: true
Attachment #9035462 - Flags: review+
Keywords: checkin-needed
Whiteboard: [leave open]

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b7fe10c2833b
In the folder picker, generate the Recent and Favorites menus only if opened. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
Blocks: 1504141
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: