92522 - select offline dialog: cancel button in Select Items for Offline usage gui does not cancel any changes you make

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Backend, defect, P2)

Description

[grylchan]

Using branch builds
20010725 NT 4.0, linux 2.2, mac 9.0.4

1)Bring up the Select Items for Offline usage window
 from either Download and Sync gui or the 'select button' in
 the preference section for Offline & Disk Space
2)Check a folder for download
3)Hit cancel
4)Bring the Select Items for Offline usage window back up

Result: The folder you checked has a checkmark in the box 
Expected: The folder to not be checked since you canceled

Mohan assuming this would be your bug, if I am wrong please
correct.

Comment 1

[Mohan Bhamidipati]

reassigning to dianesun;

Comment 2

[Diane Sun]

Attachment: Fix

Comment 3

[Diane Sun]

Request R & SR.

Comment 4

[(not reading, please use [e-mail] instead)]

sorry, I should have caught this earlier.

this approach won't work once #85088 lands.

(the fix for that bug is in #73865, which is about to land.)

I'll take this bug and work on the fix.

Comment 5

[(not reading, please use [e-mail] instead)]

accepting.  now that 85088 has landed, I can fix this.

Comment 6

[Scott MacGregor]

this work will happen during .9.5. Leaving the enterprise nomination alone.

Comment 7

[(not reading, please use [e-mail] instead)]

any strong objections to temporarily (until I fix this) removing the "cancel" 
button (since it doesn't do anything), and just having an OK button?

Comment 8

[(not reading, please use [e-mail] instead)]

note, the work around is to uncheck what you've checked, so this isn't a super-
stop-ship bug.  (but it is lame, and I'll fix it one day)

Comment 9

[grylchan]

I don't have a problem with it. defer to others on this one.

Comment 10

[Håkan Waara]

Removing the Cancel button will force the user to press OK, regardless if she
agrees to download the selected folders. Which is bad.

Please don't remove it. Only alerts, where you say "This and this happened, and
you can do absolutely nothing about it!" should have only a OK button.

Comment 11

[grylchan]

I'm guessing Seth's pt is the cancel button doesn't work anyways.
If you select a folder and make a mistake, hitting cancel
won't cancel it. The folder will still be selected. The user
must still check the folder again to uncheck it. So temporarily
don't confuse the user and eliminate the cancel button since
the user will be under impression hitting cancel will correct
any selecting/unselecting of folders (which isn't currently true).

Comment 12

[Håkan Waara]

So make Cancel work (i.e., fix the bug)?

Removing Cancel will only create a new bug, which is no better than this.

Comment 13

[jglick]

Remove Cancel and OK for now. Add only a "Close" button.  Fill a separate but to 
implement OK and Cancel and remove "Close".

Comment 14

[jglick]

Fill a separate "bug" not "but" to be implemented in the future. :-)

Comment 15

[Håkan Waara]

Why are you people suggesting all these workarounds?  Just fix the root bug once
and for all instead of wallpapering over it!

Comment 16

[Jussi-Pekka Mantere]

Triaging nsenterprise-.

Comment 17

[(not reading, please use [e-mail] instead)]

I hope to fix this the right way in 0.9.6

Comment 18

[scottputterman]

reassigning to ssu.

Comment 19

[scottputterman]

reassign to shliang.

Comment 20

[shliang]

Attachment: patch

Comment 21

[shliang]

Attachment: patch

Comment 22

[racham]

Comment on attachment 70960 [details] [diff] [review]
patch

Patch looks fine.

However, I really think that we should be taking the approach where we save all
the preferences user made in selectOkButton routine and don't do anything on
cancel exactly reflecting user's actions. I leave the choice to you.

Add comments to the current patch before you check in as to why you needed to
do take snapshot of initial state.

r=bhuvan

Comment 23

[(not reading, please use [e-mail] instead)]

Comment on attachment 70960 [details] [diff] [review]
patch

sr=sspitzer, nice work, clean solution.

Comment 24

[(not reading, please use [e-mail] instead)]

>However, I really think that we should be taking the approach where we save all
>the preferences user made in selectOkButton routine and don't do anything on
>cancel exactly reflecting user's actions. I leave the choice to you.

shuehan's approach is correct.  

here's the problem:  I switched this code to use outliner builder and the 
folder datasource.  for state (and changes to the state) to show up in the 
outliner, they have to be refected in the folder datasource.  so, I made it so 
on user actions, we modify the folder flags, which notifies the folder 
datasource, which will notify the outliner builder.

this was an easy way to re-use the existing code (folder ds, outliner template 
xul.)

it works great for OK, but how to handle cancel?

shuehan's fix is a simple, clean way to do that, given the decision I made on 
how to implement the dialog.

re-writing this dialog to do nothing on click, and then make changes on OK 
would be a lot more work. 

Comment 25

[racham]

My idea was not to do nothing on click. On click, you would collect the folder
and flag state info. On OK, you apply them. On Cancel, you ignore them.

Comment 26

[(not reading, please use [e-mail] instead)]

> My idea was not to do nothing on click. On click, you would collect the folder
> and flag state info. On OK, you apply them. On Cancel, you ignore them.

you'd want the user's actions to update the UI, right?

how would you do that, given that it's a outliner template on top of the folder 
datasource?

Comment 27

[racham]

Never mind. Yes, given the way we have implemented the UI there, we got to take
snapshots and restore.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 70960 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 29

[shliang]

fixed

Comment 30

[grylchan]

Using commercial trunk
2002032203 NT 4.0
2002032008 linux 2.2
2002032208 on Mac 9.2.2 & 10.1.3

Verified Cancel button in the Items for Offline Usage window
now does cancel any changes you made and returns you to your last
saved state.

Tried via Download/Sync now and through offline settings and no problems.
Works fine in online or offline mode and tested both themes.

Marking as verified.