81415 - History doesn't remember settings

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Junk_HbJ]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9+) Gecko/20010516
BuildID:    2001051620

If you select "Navigator - History" under Preferences and enter new values for
"Pages in history expire after ... days" or "Session history size", these
changes are not saved if you click "OK".

Reproducible: Always
Steps to Reproduce:
1. Open preferences
2. Select "Navigator - History"
3. Enter new values for expiration time of pages or the size of local history
4. Click OK
5. Check if values were saved.

Actual Results:  Changes not saved

Expected Results:  Changes saved

There's no error message on the Win32-console (got via "-console" startup
parameter) or in the Javascript console.

Comment 1

[sairuh (rarely reading bugmail)]

claudius, could you vrfy whether this is a history or prefs problem? thx!

Comment 2

[Chris McAfee]

confirming, w2k current build.

Comment 3

[Radha on family leave (not reading bugmail)]

this is a pref problem. 

Comment 4

[sairuh (rarely reading bugmail)]

->mcafee or alecf?

Comment 5

[doctor__j]

*** Bug 81800 has been marked as a duplicate of this bug. ***

Comment 6

[Aleksey Nogin]

I see the same thing on Linux, build id 2001051815 running on RedHat 7.1:
If I change the history preferences, the next time I open the preferences
window, it shows the default prefs again. I do have
user_pref("browser.link_expiration", 720);
line in my prefs.js, but links still expire in 9 days.

This causes a loss of history data => dataloss keyword. 

Comment 7

[Chris McAfee]

session history int field also doesn't remember things,
possible hidden xul error?  linux -> all

Note, setting values in prefs.js works fine.

Comment 8

[Aleksey Nogin]

> Note, setting values in prefs.js works fine.
>
I am pretty sure it's not the case? I have
user_pref("browser.link_expiration", 720);
in my prefs.js and still - the way I noticed this bug is when I realized that
all my old history have suddenly expired!

Comment 9

[Chris McAfee]

this bug is about getting the right pref to the history library,
e.g. the problem with the pref UI not remembering.
If the right values are going to the history library
and it's _still_ not working right, that's another bug
and should be assigned to the history component.

Comment 10

[Paul Chen]

nav triage team:

marking nsbeta1+, p2, and mozilla0.9.2

Comment 11

[Alec Flett]

argh, part of the problem is that we're saving the session history prefs in a
really wacky way... I have a fix for the session history issues in my tree (I
noticed this the other day), attaching a patch

Comment 12

[Alec Flett]

ok the other problem is that we're defining both _elementIDs and GetFields(),
which is just nutty. We should only define one, as far as I know, and
GetFields() is just wrong.

Comment 13

[Alec Flett]

Attachment: fix up session history, and probably global history too

Comment 14

[Alec Flett]

ok, I _think_ this fixes it...this is untested though (still waiting on my build)

Comment 15

[Chris McAfee]

over to alecf!

Comment 16

[R.K.Aa.]

*** Bug 82730 has been marked as a duplicate of this bug. ***

Comment 17

[Alec Flett]

*** Bug 83473 has been marked as a duplicate of this bug. ***

Comment 18

[Viswanath Ramachandran]

nav triage: alec, this is not strictly an rtm stopper hence we're moving it out. 
It would still be a nice fix to have. 

Comment 19

[R.K.Aa.]

*** Bug 84576 has been marked as a duplicate of this bug. ***

Comment 20

[Alec Flett]

*** Bug 85245 has been marked as a duplicate of this bug. ***

Comment 21

[R.K.Aa.]

*** Bug 85760 has been marked as a duplicate of this bug. ***

Comment 22

[Blake Ross]

Alec: lots of people complained about this.  Can you test it out (and I'll sr)?

Comment 23

[Alec Flett]

ok seems to fix it but my biggest fear is that this will cause us to leak
session history.. need to talk to radha about how to solve that..

Comment 24

[Blake Ross]

Okay.  We can also just fix this on the FE side if necessary (I looked into this
earlier before finding this patch and found what I think is the problem).

Comment 25

[R.K.Aa.]

*** Bug 86465 has been marked as a duplicate of this bug. ***

Comment 26

[Blake Ross]

Attachment: fix from FE side

Comment 27

[Blake Ross]

Okay, this odd looking patch fixes it from the FE side.  I say we get this in 
tonight before tree closure and then worry about the other patch afterwards.

The problem was that GetFields() has a special purpose in nsWidgetStateManager; 
it's used when the panels are switched and when the OK button is pressed to save 
state.  That's not what we want in this case.  The patch renames it.  If the 
panel is shown, we need to get the current value in the textbox.

That stuff in the if (maxSize < 0) ensures that the UI properly reflects 0 
instead of what it previously did (just bailing early without updating the 
pref).  If the panel is currently shown, nsWidgetStateManager/nsPrefWindow go 
through the elementIDs in the panel to save each pref, so we need to update the 
value in the textbox itself.  If it's not shown, it uses the pageData that it 
collected upon switching panels, so we have to update the value in the array.

Comment 28

[Blake Ross]

Oops, realized while typing that up that this patch has a slight problem: now it 
updates the UI to say 0, but not the backend.

Easy enough to fix, but now I'm wondering if we want to just keep the old value 
if they enter an illegal one.  Which is more correct, assuming 0 if they enter 
negative/non-numeric, or keeping the old one?

All our troubles disappear once bug 66163 is fixed.

Comment 29

[Blake Ross]

Attachment: (assuming 0...)

Comment 30

[Alec Flett]

sr=alecf
one 'o these days I'm going to fix this the right way :)

Comment 31

[Blake Ross]

Crap.  I forgot to get this in for .9.2.  Reassigning to me to try to get this 
into the branch, then I'll give it back to you alec.

This actually is the right fix (until bug 66163 is fixed) for what we're trying 
to do -- access and validate a value in one pref panel from another panel.

Comment 32

[Blake Ross]

Hrm, I think I remember something from when I was testing this earlier.  I seem 
to recall that the value entered was always one off from the value used by 
session history, e.g. enter 1 and 0 are saved, enter 2 and 1 are saved.

Reminder to myself to investigate where such an off-by-one error is taking 
place...

Comment 33

[Blake Ross]

Er, to clarify (and spam everyone) again: we're saving the pref correctly; the 
off-by-one error is somewhere in the backend.

Comment 34

[Jason Kersey]

r=kerz

Comment 35

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the frozen 0.9.2.
(on behalf of drivers)

Comment 36

[Blake Ross]

--> alec for backend fix

Comment 37

[Jussi-Pekka Mantere]

Was this ever checked into the trunk?

Comment 38

[Blake Ross]

Yes.

Comment 39

[Hong Kwon]

based on comments, and my testing on Win2k, this is fixed.  claudius can you verify?

marking RESOLVED FIXED

Comment 40

[Claudius Gayle]

VERIFIED Fixed with 2001091003 builds.

side note. Blake, I do not think it is correct to arbitrarily reset invalid
entries to '0'. If i'm trying to extend my history and I accidentally type a
letter instead of a number I'm going to be pretty ticked when the history
expires the next day (or immediately?).

Also, the textfield does not update (in this case reset to '0') when you have an
invalid entry and panels are switched.

I guess that'll be two new bugs...