More maintainable code for sequencing through dialogs



17 years ago
14 years ago


(Reporter: Curt Patrick (gone), Assigned: Curt Patrick (gone))


Windows NT

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

31.01 KB, patch
dprice (gone)
: review+
: superreview+
Details | Diff | Splinter Review


17 years ago
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.

Comment 1

16 years ago
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.

Comment 2

16 years ago
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


Comment 3

16 years ago
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,
Attachment #92763 - Attachment is obsolete: true

Comment 4

16 years ago
Comment on attachment 92824 [details] [diff] [review]
Patch 2

Attachment #92824 - Flags: review+

Comment 5

16 years ago
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).

Comment 6

16 years ago
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...

Attachment #92824 - Flags: superreview+


16 years ago
QA Contact: bugzilla → gbush

Comment 8

16 years ago
Oops.  Just stumbled across this.  This was checked in back on 8/6, but it looks
like I failed to close the bug.
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 9

16 years ago
verified code fix
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.