Closed Bug 230357 Opened 21 years ago Closed 21 years ago

gHistoryGrouping in history.js is not updated

Categories

(Core Graveyard :: History: Global, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: durbacher, Assigned: durbacher)

References

Details

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209

This variable reflects the state of the pref "browser.history.grouping" that
determines if history data should be shown grouped (in folders) by day, by site
or ungrouped.
It is only read in on window (or sidebar) opening and then never changed. This
results in changes of that pref being unnoticed by the history window.

Reproducible: Always

Steps to Reproduce:
1. Open history window with Ctrl-H or via "Go" menu
2. Select "View" - "Group by" - "Site" in history window 
3. Close and reopen history window
4. select two "folders"
5. right click for context menu
6. Remember that it says "Delete" and "Select all"
7. Select "View" - "Group by" - "None"
8. Close and reopen history window
9. Select "View" - "Group by" - "Site" again
10. Again, select two folders and open context menu


Actual Results:  
It now offers you many more things, e.g. bookmarking the link ("Bookmark this
Page(L)" is bug 230349) and copying the link location

Expected Results:  
It should show the same context menu as before.

As I wrote, gHistoryGrouping in history.js is never updated; it is used when the
context menu is constructed for determining which entries should show up.

Underlying problems:
1) Only the sidebar panel gets its pref observer:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/history/resources/history.js#108

2) The pref observer there only calls GroupBy() to update view:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/history/resources/history.js#346
- but it does not change gHistoryGrouping.


In an experimental patch I changed these points, but got a JS error from the
observer:
Error: Object is not defined
Source File: chrome://communicator/content/history/history.js
Line: 348
this is the catch clause of the observer:
"catch(ex) {"
and I have no idea what that error means...

However, this error is not a result of the patch, but is already built in - only
it happens rarely: you have to open the history sidebar (so the observer is
active) and then change view grouping in the history standalone window to see this.
What happens is that aPrefBranch.QueryInterface fails in the second run of the
observer (yes, it is not only run once as I had thought).
Depends on bug 227053 because that error seems to abort script execution.
Depends on: 227053
Attached patch patch (obsolete) — Splinter Review
When view grouping in the history window is changed, reflect the change in
gHistoryGrouping.
In the history sidebar this cannot be changed, it only observs the pref. So
there change gHistoryGrouping in the observer (when the pref changes).
Comment on attachment 138695 [details] [diff] [review]
patch

Ok, this patch is ready for rewiev. Asking r= from Neil and sr= from alecf.

The "Object is not defined" error appears when the history sidebar is opened
more than once because the prefs observer is added each time, but not removed
when the sidebar is closed.

This is also fixed by Neil's patch for bug 227053. So this bug here depends on
that one for _two_ reasons now...
Attachment #138695 - Flags: superreview?(alecf)
Attachment #138695 - Flags: review?(neil.parkwaycc.co.uk)
Assignee: nobody → durbacher
Comment on attachment 138695 [details] [diff] [review]
patch

sr=alecf
Attachment #138695 - Flags: superreview?(alecf) → superreview+
Comment on attachment 138695 [details] [diff] [review]
patch

>     switch(groupingType) {
>     case "none":
>         tree.setAttribute("ref", "NC:HistoryRoot");
>+        gHistoryGrouping = "none";
>         break;
>     case "site":
>         tree.setAttribute("ref", "find:datasource=history&groupby=Hostname");
>+        gHistoryGrouping = "site";
>         break;
>     case "day":
>     default:
>         tree.setAttribute("ref", "NC:HistoryByDate");
>+        gHistoryGrouping = "day";
>         break;
>     }
We only really need to know if gHistoryGrouping is "none" or not... it doesn't
matter if it's a random value that defaulted to day.

>     gPrefService.setCharPref("browser.history.grouping", groupingType);
Ah, nice, so the sidebar observer gets notified when the pref changed and calls
the grouping function, which fortunately doesn't fire another change because it
sets the pref to the same value :-)

>-      GroupBy(aPrefBranch.QueryInterface(Components.interfaces.nsIPrefBranch).getCharPref(aPrefName));
>+      gHistoryGrouping = aPrefBranch.QueryInterface(Components.interfaces.nsIPrefBranch).getCharPref(aPrefName);
>+      GroupBy(gHistoryGrouping);
But you just changed GroupBy to set gHistoryGrouping...
Attachment #138695 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch new patch (obsolete) — Splinter Review
Neil's idea: GroupBy should change the pref and global variable and only be
called by the menu when grouping is changed. The new function GroupTreeBy is
called by it and also called by init and observer and does now only update the
shown tree.

Now the pref observer does not set the pref anymore... ;-)
Attachment #138695 - Attachment is obsolete: true
Comment on attachment 138761 [details] [diff] [review]
new patch

Better?
I changed the indentation of GroupTreeBy (the former GroupBy) to what seems to
predominate in this part of the file. But if I shouldn't I'll reverse it.
Attachment #138761 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 138761 [details] [diff] [review]
new patch

>+  switch(aGroupingType) {
>+  case "none":
>+    gHistoryGrouping = "none";
>+    break;
>+  case "site":
>+    gHistoryGrouping = "site";
>+    break;
>+  case "day":
>+  default:
>+    gHistoryGrouping = "day";
>+    break;
>+  }
You should be able to set gHistoryGrouping = aGroupingType here, because this
function always gets "none", "site" or "day".

>+function GroupTreeBy(aGroupingType)
You should be able to use gHistoryGrouping here, in which case you might want
to rename this UpdateTreeGrouping or something..

>     try {
>-      GroupBy(aPrefBranch.QueryInterface(Components.interfaces.nsIPrefBranch).getCharPref(aPrefName));
>+      GroupTreeBy(aPrefBranch.QueryInterface(Components.interfaces.nsIPrefBranch).getCharPref(aPrefName));
>     }
>     catch(ex) {
>     }
I think the code here should look like it is around line 90 (including your
change) - it looks as if it should be safe to use gPrefService (but you will
test it won't you ;-)
Attachment #138761 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch next trySplinter Review
All comments fixed.
Using gPrefService seems safe from testing and looking at code: it is not set
when a history "sub-window" is opened (open history folder in new window), but
the opener and the sidebar window (that registered the pref observer) both set
it.
Thanks for your reviewing patience...
Attachment #138761 - Attachment is obsolete: true
Comment on attachment 138766 [details] [diff] [review]
next try

Neil? (see comment above)
Attachment #138766 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #138766 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 138766 [details] [diff] [review]
next try

Requesting sr= from alecf.

Sorry for bugging you twice with this! From now on I will only request sr= when
I already have r=. 
(...or when I'm confident to get it ...or when everything has to happen
quickly... ;-) )
Attachment #138766 - Flags: superreview?(alecf)
Comment on attachment 138766 [details] [diff] [review]
next try

cool, thanks..I like this patch better anyhow
Attachment #138766 - Flags: superreview?(alecf) → superreview+
Checking in xpfe/components/history/resources/history.js;
/cvsroot/mozilla/xpfe/components/history/resources/history.js,v  <--  history.js
new revision: 1.59; previous revision: 1.58
done
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: