Right now there is a seperate function for sequencing through Next buttons vs sequencing through Previous buttons, even thought they exactly parallel each other but in reverse order. Need to fold them into a single function that takes a parameter for Next or Prev. Also, there is almost certainly a less confusing approach which could be devised for dealing with different branches through the dialogs as, for example, in the case of Custom showing a couple different dialogs from the other install types.
Created attachment 92763 [details] [diff] [review] Patch 1 This is a maintenance patch. The functionality should not change at all. The gist of what I've done: I replaced DlgSequenceNext() and DlgSequencePrevious with a single function DlgSequence() which takes a parameter to tell it which direction to go. The first thing DlgSequence() does is look at what swWizardState it is in and, depending upon whether it has recieved GO_NEXT or GO_BACK it sets the swWizardState for the next dialog which should be displayed. The body of the function then handles the dialog for that state. The advantages to this are twofold: 1) The code for displaying the dialogs is no longer duplicated in two functions and 2) the states which are identified in the main switch statement are the actual states not the last state. For maintenance purposes, I found it very confusing to have to think in terms of "where was I" rather than "where am I". One other change I made: eventually I think it would be good to figure out a way to eliminate ever changing dwWizardState outside of DlgSequence(). I think this could be possible and it would be useful to have all the decision making about which dialogs to show completely encapsulated in one place. But that should take a little more discussion and can be another bug if others agree that it would be useful. In the meantime, I added two additional parameters which can be passed into DlgSequence(), GO_HERE_NEXT and GO_HERE_BACK. In the case that you need to jump out of sequence (the only case where I discovered this to be true so far is when there is a button on a dialog which is supposed to take you to another dialog, besides Next and Previous) you can point dwWizardState to the specific dialog you want to target instead of having to figure out which state preceeds the one you're shooting for. DlgSequence() takes you directly to that dialog and then proceed NEXT or BACK if that dialog is not displayed.
I like this change. GO_HERE_NEXT and GO_HERE_BACK are a little confusing. GO_HERE_THEN_NEXT sounds more like what you described. But you pass those values into DlgSequence and promptly change them. + dwWizardState = DLG_ADVANCED_SETTINGS; + DlgSequence(GO_HERE_NEXT); And the code itself documents the notion you're trying to convey with the additional flag. I think you can take them out. I can understand the thinking behind GO_BACK and GO_NEXT, because the installer uses back and next buttons for navigation. There's a direct relationship between the ui and code. But in the code, it feels more consistent to use next and prev together. They are natural opposites. I'd probably rename GO_BACK to PREV_DLG and GO_NEXT to NEXT_DLG
Created attachment 92824 [details] [diff] [review] Patch 2 I hate to admit it but has some really good points. This patch uses his naming scheme which is way better than mine. He also got me to thinking about he relationship between the buttons in the UI and the DlgSequence() and I realize that what we are really telling DlgSequence() is which button was clicked, either Next, Previous, or some other button. If DlgSequence() knows what button was clicked and what state it is in it should have all the info it needs in order to know what state to go to next. No other function should ever need to change state. So... I got rid of the goofy GO_HERE_NEXT and GO_HERE_BACK parameters and replaced them with OTHER_DLG1, which can be passed into DlgSequence() if a button which effects sequence other than Next or Previous is clicked. Of course, if we come up with a dialog that has two such buttons we'll need to add a OTHER_DLG2, but right now there is only one dialog effected (Additional Options) and that has only one button (for Advanced Settings). So this patch is MUCH cleaner, thanks to Dave's feedback. Have at it again, then.
Attachment #92763 - Attachment is obsolete: true
Comment on attachment 92824 [details] [diff] [review] Patch 2 r=dprice
Attachment #92824 - Flags: review+
please make sure the patch is tested with some and all of the dialogs set to not be shown. Also test to make sure that when in certain dialogs, clicking on "Back" then going back to that dialog will cause it remember the original settings of when you first entered that dialog. Meaning that the "Back" button acts like a cancel button (not the same as exiting the installer).
I have tested all the dialog sequences I could think of, with both the mozilla and netscape config.ini's. Also turned on dialogs that aren't currently used by either (I think that only applies to one dialog, actually.) and tested that way. I will take a closer look whether dialogs are remembering setting correctly. I *think* that should be okay--didn't notice any problems--but wasn't specifically checking for that.
Comment on attachment 92824 [details] [diff] [review] Patch 2 I would have preferred a switch on dwWizardState with direction inside each case rather than branching on the direction first. Future dialog addition would be more like adding to a linked list and fixing up the next and prev pointers than maintaining a list of next pointers and a separate list of prev pointers. But that's just me... sr=dveditz
Attachment #92824 - Flags: superreview+
Oops. Just stumbled across this. This was checked in back on 8/6, but it looks like I failed to close the bug.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
verified code fix
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.