Closed Bug 170522 Opened 22 years ago Closed 22 years ago

BuildRecentMenu is inefficient; should be split up

Categories

(SeaMonkey :: Composer, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.3alpha

People

(Reporter: Brade, Assigned: cmanske)

Details

(Keywords: perf)

Attachments

(1 file, 4 obsolete files)

The JS function for "BuildRecentMenu" does too much.  It should be split into
two separate functions.  The saving of the prefs should be a separate routine
since sometimes (like when a document is opened or saved with a new name or
given a new title) the menu doesn't need to be rebuilt and only the preferences
need to be updated.  BuildRecentMenu should do just that (and only that). 
Perhaps the new function could be called something like "SaveRecentPrefs" or
something clearer than being a flag for the BuildRecentMenu function.
Keywords: perf
Mine
Went over this patch with brade in #composer, we took out the StripPassword,
because the password is always stripped when going into the prefs, and we
return if the curUrl is about:blank, because we won't be changing anything.
Oh, this patch is untested, can somebody please test?
Comment on attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

r=cmanske
Thanks!
Note that this patch has to  be merged with the one on bug 169029, so the
change at block @@ -3841,8 +3841,9 @@ becomes:
@@ -326,9 +326,11 @@

     // Call EditorSetDefaultPrefsAndDoctype first so it gets the default
author before initing toolbars
     EditorSetDefaultPrefsAndDoctype();
     EditorInitToolbars();
-    BuildRecentMenu(true);	 // Build the recent files menu and save to
prefs
+    
+    // Set window title and build "Recent Files" menu
+    UpdateWindowTitle();

We don't have to save the  recent file items to prefs when a new editor has
just been created. I'll check this work in with fix to bug 169029.
Attachment #100424 - Flags: review+
Status: NEW → ASSIGNED
Whiteboard: [FIX IN HAND]need sr=
Target Milestone: --- → mozilla1.2beta
uggh! of course we have to build the RecentFiles menu:
+    // Set window title and build "Recent Files" menu
+    UpdateWindowTitle();
+    BuildRecentMenu();
Comment on attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

Actually you should enable/disable the recent files option (as part of
EditorInitFileMenu?) when you open the file menu so that you only need to call
BuildRecentMenu when opening the recent menu.
Attachment #100424 - Flags: needs-work+
Comment on attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

Actually, I'm not sure I agree with Neil but I'd have to do some testing to be
sure.  The scenario I am thinking of goes something like this:
  one file in history: foo.html
  open new page
  open recent menu (1)
  save as foo.html
  open recent menu (2)

The first time I open the recent menu, I expect it to show me one item.  The
2nd time, I'd expect the menu to be disabled.

Related might be when the user changes their preferences from showing 0 to 3
(or whatever) or from 3 to 0?
Comment on attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

OK, so brade has convinced me that enabling/disabling (as opposed to
rebuilding) the whole recent menu on open or save is enough.
Comment on attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

reviewers comments were addressed.
Attachment #100424 - Flags: needs-work+
I don't suppose you can slip in another fix I spotted:
Currently AppendRecentMenuitem adds an oncommand attribute to each recent item.
Instead, there should be a bubbling oncommand attribute set on the menupopup:
<menupopup id="menupopup_RecentFiles"
oncommand="editPage(event.target.getAttribute('value'), window, false);"/>
Comment on attachment 100424 [details] [diff] [review]
Split SaveRecentFilesPrefs from BuildRecentMenu

This all looks fine except for the savePrefFile() - I'd really prefer that you
don't save prefs every time someone opens a document...let prefs figure out
when the best time to save is)
sr=alecf without that line.
Attachment #100424 - Flags: superreview+
This was checked in with fix to bug 169029
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND]need sr=
Attached patch Resolves some possible errors (obsolete) — Splinter Review
Fixes:
1. Use GetDocumentTitle
2. Really don't save about:blank
3. Don't save data: if you're not going to show it
4. If no prefs, disable recent menu
5. Fix brade's edge case: save about:blank over sole recent url
Reopening for consideration.
Status: RESOLVED → REOPENED
Keywords: patch, review
Resolution: FIXED → ---
I fixed some of these issues in recent checkin for bug 169029.
I forgot about this patch -- I'll review it and see how to update as suggested.
Status: REOPENED → ASSIGNED
Whiteboard: [FIX IN HAND] need r=,sr=
Target Milestone: mozilla1.2beta → mozilla1.3alpha
Attachment #101767 - Flags: review?(cmanske)
Attached patch Fix v3 (obsolete) — Splinter Review
This patch updates for bitrot and puts the enabling in the "onPopupShowing" 
method of the Files menu. This is better since after a user opens a 2nd file in

a different window, the first window's "Recent Pages" will be enabled
correctly.
The actual menu is constructed from prefs *only* when menu is opened.
Saving the recently-opened filenames and titles to prefs is done only when
absolutely necessary.
I also removed the "cmd_buildRecentMenu" command since it is no longer used
(it used to be used by editorShell.)
Attachment #100424 - Attachment is obsolete: true
Attachment #101767 - Attachment is obsolete: true
Attachment #107965 - Flags: superreview?(alecf)
Attachment #107965 - Flags: review?(neil)
Comment on attachment 107965 [details] [diff] [review]
Fix v3

>-function BuildRecentMenu()
>+function BuildRecentPagesMenu()
What's this doing here? You haven't renamed it in editorOverlay.

>+  if (historyCount && !IsUrlAboutBlank(curUrl))
>+  {
>+    titleArray.push(GetDocumentTitle());
>+    urlArray.push(curUrl);
>+  }
I was trying to avoid saving data urls in history...

>+  // Enable recent pages submenu if there are any history entries in prefs
>+  var historyUrl = GetUnicharPref("editor.history_url_0");
>+
>+  // See if there's more if current file is only entry in history list
>+  if (historyUrl && historyUrl == docUrl)
>+    historyUrl = GetUnicharPref("editor.history_url_1");
>+  SetElementEnabledById("menu_RecentFiles", historyUrl != "");

Not quite accurate - you don't check the historyCount, or for data: urls.
Attachment #107965 - Flags: review?(neil) → review-
Re: Neil's comments:
(From update of attachment 107965 [details] [diff] [review])
>-function BuildRecentMenu()
>+function BuildRecentPagesMenu()
What's this doing here? You haven't renamed it in editorOverlay.

cmanske: Oops! It's changed in my tree. Forgot to update patch.

>+  if (historyCount && !IsUrlAboutBlank(curUrl))
>+  {
>+    titleArray.push(GetDocumentTitle());
>+    urlArray.push(curUrl);
>+  }
I was trying to avoid saving data urls in history...
cmanske: I'll fix that

>+  // Enable recent pages submenu if there are any history entries in prefs
>+  var historyUrl = GetUnicharPref("editor.history_url_0");
>+
>+  // See if there's more if current file is only entry in history list
>+  if (historyUrl && historyUrl == docUrl)
>+    historyUrl = GetUnicharPref("editor.history_url_1");
>+  SetElementEnabledById("menu_RecentFiles", historyUrl != "");

Not quite accurate - you don't check the historyCount, or for data: urls.
cmanske: Good point. It would be a silly user who set their pref to "0", imho!
Attached patch fix v4 (obsolete) — Splinter Review
This fixes all issues noted by Neil. I kept the check for "data" urls in 
SaveRecentFilesPrefs() even though I was tempted to remove it, since if we 
check when we add the URL for the current page, we shouldn't have to check
again. But Neil pointed out that there may be data urls from previous versions.

This allows us to not check for data url in the enable code in method 
InitFileMenu, to make that more efficient.
Attachment #107965 - Attachment is obsolete: true
Attached patch fix v5Splinter Review
Same as v4, but includes change to editorOverlay.xul for
"BuildRecentPagesMenu()"
Attachment #108116 - Attachment is obsolete: true
Comment on attachment 108123 [details] [diff] [review]
fix v5

You have a long line in the try { historyCount thing.  Otherwise, it seems to
address all the issues.  r=akkana
Attachment #108123 - Flags: review+
Attachment #107965 - Flags: superreview?(alecf)
Attachment #107965 - Flags: review-
Attachment #100424 - Flags: superreview+
Attachment #100424 - Flags: review+
Attachment #108123 - Flags: superreview?(alecf)
Comment on attachment 108123 [details] [diff] [review]
fix v5

>+  var historyCount;
Nit: Should be = 10 by default as in other places

>+  try { historyCount = gPrefs.getIntPref("editor.history.url_maximum"); } catch(e) {}
Nit: separate lines for try and catch

>+      historyUrl = GetUnicharPref("editor.history_url_1");
Possible check for historyCount > 1?

>+            onpopupshowing="BuildRecentPagesMenu()">
Nit: Should add ;
Ok, issues noted by Neil in last comment fixed.
Whiteboard: [FIX IN HAND] need r=,sr= → [FIX IN HAND] need sr=
Comment on attachment 108123 [details] [diff] [review]
fix v5

sr=alecf
Attachment #108123 - Flags: superreview?(alecf) → superreview+
Attachment #108123 - Flags: approval1.3a?
Comment on attachment 108123 [details] [diff] [review]
fix v5

a=asa for checkin to 1.3a
Attachment #108123 - Flags: approval1.3a? → approval1.3a+
checked into 1.3a trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND] need sr=
Attachment #101767 - Flags: review?(cmanske)
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: