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: