Put the theme switching warning dialog back

VERIFIED FIXED

Status

Core Graveyard
Skinability
P2
normal
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: johng, Assigned: Blake Ross)

Tracking

({dataloss})

Trunk
x86
Other
dataloss
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-][pdtp2][FIX IN HAND][rtm++])

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
When you apply a new theme, you get an scary dialog that we will no longer need
after some bugs are fixed (marked as dependencies, and all are marked nsbeta3+).
 Please remove this dialog box.
(Reporter)

Comment 1

18 years ago
skin triage team:
nsbeta3+
added dependency bugs, but we can go ahead and implement this bug
Depends on: 43350, 44437, 47345
Keywords: nsbeta3
Whiteboard: [nsbeta3+]
(Assignee)

Comment 2

18 years ago
.
Assignee: ben → BlakeR1234
(Assignee)

Comment 3

18 years ago
fixed
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 4

18 years ago
This should absolutely not be fixed until the dependancies are fixed! This 
warning dialog is required as long as switching skins are causing data loss!

Blake, please back out your checkin. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

18 years ago
I don't understand why not having it there anymore is a problem.  It was only 
added for PR2 so the public would know of the dangers of skin switching.  Since 
all the remaining skin switching bugs are nsbeta3+, there is no need for this 
dialog to exist in PR3.  If for whatever reason the bugs aren't fixed for PR3, 
we can always readd it... The addition of the dialog was solely for PR2.

Comment 6

18 years ago
This was added to PR2 since the data loss bugs couldn't be fixed in time. There 
is no reason to remove it until those bugs are gone, and that could be a long 
time in the future. Just beacuse they're marked nsbeta3+ doesn't mean they will 
be fixed to PR3.

Also read the original bug report. It also says that this dialog box isn't 
necessary "after some bugs are fixed". These bugs aren't fixed yet. 
(Assignee)

Comment 7

18 years ago
Note the second comment: "skin triage team:
nsbeta3+
added dependency bugs, but we can go ahead and implement this bug"

They will be fixed for beta3 (nsbeta3+), or they likely won't be fixed at all.  
Little is expected to be done between beta3 and rtm.

Whatever, this isn't worth arguing about. I don't mind putting it back.  
John..what do you want to do?

Comment 8

18 years ago
Oh, I didn't see that, but then again, I think that it's wrong to risk data loss 
without notice for all users of nightly build and even milestones released 
before the other bugs are fixed. 
Looking at the dependant bugs, none seem to be close to a fix and knowing how 
many bugs slipped past PR2, I wouldn't bet that they all be fixed for PR3.
(Reporter)

Comment 9

18 years ago
I don't care strongly whether you do it now or later, but I lean to doing it now
to call attention to fixing those other bugs.

We do fully intend to fix those other bugs by PR3 - theme switching is too
important not to, and we don't want people getting used to the idea of a warning
dialog that we will pull out anyway.  Let's make people suffer now to avoid pain
later.

However, pulling it our now will cause a little flame (deserved) for me, blake,
ben, and the other engineers who haven't fixed those bugs yet.

Personally, I wouldn't mind a little flame if it gets those other bugs fixed
sooner, but I can't speak for everyone else.  Blake, your call.
(Assignee)

Comment 10

18 years ago
I agree.  Those bugs must be fixed for beta3, or not at all.  We can't push 
them out any longer.

So, this dialog is gone now.  I can handle the flames :) Marking fixed. Thanks 
John.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

18 years ago
vrfy fixed with a build just pulled on win98
Status: RESOLVED → VERIFIED

Comment 12

18 years ago
It is no longer the case that the bugs this depended on is nsbeta3+. For 
instance, the "compose/mail window content is gone after switching skin" bug is 
now nsbeta3-. That means that the warning dialog might be needed again. 
Reopening for reconsideration. Removing nsbeta3+ from status whiteboard.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Whiteboard: [nsbeta3+]

Updated

18 years ago
Summary: Remove the skin switch warning dialog → Put the skin switch warning dialog back (revert the previous fix for this bug)

Comment 13

18 years ago
Reassigning this bug to Peter Trudelle.  Peter, after our conversation about
this the other day, where did we end up?  I thought we were going to be putting
up a dialog that asked the user to close windows that might be susceptible to
data loss before proceeding with skin switching.  If that was indeed the case,
could you close this bug once that dlog is in?

Marking nsbeta3+ so that this gets cleared up. Setting priority to P2
Assignee: blakeross → trudelle
Status: REOPENED → NEW
Priority: P3 → P2
Whiteboard: [nsbeta3+][pdtp2]
(Assignee)

Comment 14

18 years ago
Peter: I can do this if you want.  But if you reassign this to one of your 
engineers, remind them that the dialog needs to be thrown when switching via 
View > Apply Theme as well (there goes one-click switching)

Comment 15

18 years ago
As we're not going to get to Bugs 43350 and 44437, I agree that this is
definitely a nsbeta3+.  I'd like to propose the following text for the dialogue.
 Vera, please check this out and make any changes you think are necessary.  I'm
not really sure of the best way to communicate this:

"If you currently have additional Netscape 6 components such as mail, composer,
or instant messenger open, please close them before changing themes.  After
switching themes, press reload to return to the web page you were viewing prior
to the theme change."  OR...

"Please close all other open Netscape 6 windows before switching themes.  After
switching themes, press reload to return to the web page you were viewing prior
to the theme change."

ccing verah and myself

Comment 16

18 years ago
Blake is The Man! thank you.  ->blakeross
Suggest using stronger wording, to make the effect very clear, and using the
same verb already used in the UI:
"Applying a theme will cause you to lose any unsaved work in open windows.
Please ensure that any open Mail, Composer, Instant Messenger or Address Book
windows are closed before you continue. After applying the theme, press reload
to return to any web pages you were viewing."

I would make the cancel button the default, to ensure they intend to apply the
theme. I think the last sentence may be omitted, since it is obvious to anyone
who knows how to reload a page, yet worthless to most who don't, since the
reload button has no title in the default skin. Also, the dialog is quite wordy,
even in English. We'll also need to add the ellipsis to the menu command.

Assignee: trudelle → blakeross

Comment 17

18 years ago
If the simple fix for bug 44437 is checked in (Simon says he has it), then we
won't need the "reload page" part of this dialogue.

ccing simon

Comment 18

18 years ago
Not sure about trudelle's proposed wording. We don't have to say anything about
losing unsaved work. Just say that we can't switch skins with editor/email/aim
compose windows open.

Making rtm nomination on this bug, since it seems like the one we really have to
fix (I thought it was bug 43350, but Todd steered me straight)
Keywords: rtm
(Assignee)

Comment 19

18 years ago
ahh--i somehow missed notification of this bug being reassigned to me.  I'll do 
this as soon as I hear back from hyatt re: bug 44437 (if his fix even covers 
the composer nuking problem, we won't need this dialog)
Status: NEW → ASSIGNED

Comment 20

18 years ago
So after discussion with ben and hyatt, it seems this is the focus of all
"loose data when skin switching" problems. We must show the warning dialog
to tell the user to close all windows with editors. We should run through all 
windows and identify any Composer, Message Composer (text and HTML), and IM 
windows that are open and *do not* let the user switch skins if any are found.
This is the simplest solution, so please reconsider this for rtm.
Keywords: dataloss
(Assignee)

Comment 21

18 years ago
What do you mean reconsider for rtm? It's already nsbeta3+.  Do you mean ++?

Anyways, please file that `run through the windows and don't let them switch if 
any are open' request as a separate bug.  Note that I strongly disagree with 
that approach; if we're giving the user a warning, we should assume they 
understand the consequences of switching and are voluntarily going through with 
it anyways.  We should certainly not warn people and then refuse to do the 
switch anyways even if they say OK.

cc'ing trudelle in case he has any comments

Comment 22

18 years ago
Well, I think just putting the original dialog back (as the summary says) is the
simplest solution, and was found acceptable at the PDT triage, but I agree that
prevention would be far better. Unless the dialog will truly prevent switching
whenever windows are open where data loss is possible, then I think we need to
be very clear about what will happen if they continue to switch with those
windows open.
(Assignee)

Comment 23

18 years ago
Yes, I'm starting to agree with Peter's thoughts more.  Beth and I were talking 
about this, and will come up with the solution tomorrow.

Comment 24

18 years ago
I think that blocking a skin switch (with a dialog explanation) would be an
excellent (and acceptable) solution.  I *think* that the PDT discussion today
supported that as a resolution to preclude dataloss.
For PR3, we may have to release note this, but for RTM, a "sorry, you can't
change themes with non-browser windows open" (well worded of course) would be fine.

Comment 25

18 years ago
> Well, I think just putting the original dialog back (as the summary says) is 
> the simplest solution, and was found acceptable at the PDT triage

I've never seen that dialog, but is it a warning that data loss is about to 
happen, or is it blocking the data loss by refusing to switch? MichaelL and I 
would both prefer to block the skin switch when data loss would result.

Although it pains me a little bit, I don't think we need to hold nsbeta3 for 
this. Marking nsbeta3-
Whiteboard: [nsbeta3+][pdtp2] → [nsbeta3-][pdtp2]

Comment 26

18 years ago
I'd greatly prefer the block too, short of a real fix, but it wasn't part of the
dialog we had in beta1, nor the agreement that MichaelL cites above, IIRC. I was
surprised that leaving the data loss possible was deemed okay, but maybe I was
hallucinating. Perhaps the PDT triage during Claudius' visit didn't intend to
approve the minimal solution?

Comment 27

18 years ago
I remember PDT triage during Claudius' visit as deciding to block switching when
data loss would result. MichaelL apparently remembers it that way too. Pending
feasibility of walking the windows to see whether switching was safe or not.
(Assignee)

Comment 28

18 years ago
Could we at least readd the warning (no risk) for beta3, and then worry about 
the block switching for rtm?  We had the warning in PR2, and people might think 
the problem's been fixed if we don't have it in PR3...

Comment 29

18 years ago
According to Phil's email of 9/15:

"We came up with these options, in decreasing order of preference:
1. Refuse to switch skin if windows are open where data would be lost (lossy:
mail compose, AIM compose, composer)
2. Automatically close any windows where data would be lost, invoking any "save
changes?" alert the window would use itself
3. Warning dialog: about to lose data in these windows. Continue?"

So having just the dialog (and allowing data loss) was deemed acceptable.
Subsequent discussion on the thread was that a hack checking for the specific
windows was possible using just XUL/JS. Blake, do you have the dialog ready?

Comment 30

18 years ago
Just my 2cents - it would be nice if this dialog also had a "do not warn me 
again" checkbox. So users could opt out of the warning dialog....

Comment 31

18 years ago
Could someone let me know, again, what the current wording of the dialog is? 
Sorry, I've never seen the actual dialog.
(Assignee)

Comment 32

18 years ago
OK, I have a fix for the dialog aspect of this, but need to know the proper 
wording for the warning.  Vera?  Keep in mind that if we mention Instant 
Messenger we'll have to do a little bit more work to hide it from Mozilla 
builds.  The wording in PR2 was:

Switching skins is a prototype feature and will result in all changes in an 
open editor and mail compose windows to be lost, as well as the current webpage 
in the browser. If you have any mail or editor windows open it is recommended 
that you click cancel, close these windows and then switch skins.

Changes that have to be made here include: removal of the `prototype feature', 
skins -> themes, and if hyatt gets his fix in for bug 43350, removal of the 
mention of the current webpage disappearing problem.

Andrew: that would be bad if people accidentally checked "don't show me this 
again", or if they intentionally chose to hide the warning but then later on 
forgot about it.  A better place for that option, if we were to have one, would 
be somewhere in the pref window itself (Themes pane, I presume).

Peter, cmanske posted a patch to bug 53795 to walk through and close all HTML 
composer windows. I'll try it out soon on my build.
Summary: Put the skin switch warning dialog back (revert the previous fix for this bug) → Put the theme switching warning dialog back
Whiteboard: [nsbeta3-][pdtp2] → [nsbeta3-][pdtp2][FIX IN HAND]

Comment 33

18 years ago
Here's my (rapidly composed) text:

"Before you switch themes, close all Mail, Composer, Instant Messenger or 
Address Book windows. Otherwise, you may lose any unsaved work.  After switching 
themes, click Reload to return to the web page you were viewing prior to the 
theme change."

Someone (Peter) mentioned that lots 'o people won't know what "Reload" is -- how 
true! Fortunately, it has a tooltip, so perhaps a few people will hit it by 
accident and figure it out.

Comment 34

18 years ago
Text looks good to me.  Just need to know whether or not bug 44437 gets fixed so
we know whether or not the "reload webpage text" is necessary.  Although I guess
it's harmless (if not elegant) to have in even if it is fixed...

Comment 35

18 years ago
We're using the term 'apply theme' elsewhere, and should be consistent.  Also,
isn't it true that they *will* lose any unsaved work in those windows?  If so, I
would want to make that clear, rather than sounding like it is only a possibility.

Comment 36

18 years ago
Okay, so:

"Before you switch themes, close all Mail, Composer, Instant Messenger or 
Address Book windows. Otherwise, you will lose any unsaved work.  After 
switching themes, click Reload to return to the web page you were viewing prior 
to the theme change."

I prefer to use "switch themes," because that's what the user is doing. "Apply 
themes" just doesn't make much sense. I'd change the menu item if I could.

Comment 37

18 years ago
It's not just the menu item, it is in the dialog too, and presumably in doc,
help, etc.  Isn't consistency more important than personal preference here?  If
you feel strongly, we should consider changing it everywhere, someday.

Comment 38

18 years ago
I think the terminology should fit the scenario, in this case Vera is
instructing the user as to an action that is about to be performed, which if
read in that context it is correct. I would think if you were to use the term
apply -- you would need to state: 'Before you continue to apply the theme...'
Doesn't themes need to be theme? You aren't applying more than one at a time --
right?

Comment 39

18 years ago
The user context is that they have just selected 'Apply Theme> XXX' from the
menu, or clicked 'Apply XXX' in prefs.  Nowhere else is it called 'switch'.

Comment 40

18 years ago
Fine, go with Trudelle's wording. I don't have any more time to spend on this.

Comment 41

18 years ago
"Before you apply a new theme, close all Mail, Composer, Instant Messenger or 
Address Book windows. Otherwise, you will lose any unsaved work.  After 
applying a new theme, click Reload to return to the web page you were viewing 
prior to the theme change."

Comment 42

18 years ago
ccing Jaime - this is a UI change.  What do we need to do to get it approved by
L10n?  It should be easily localizable - it is similar to the text we had for
skin switching in PR2.

Comment 43

18 years ago
Adding msanz, danielmc, mcarlson and laurasl to cc: list

From what Todd tells me, this dialog was in PR2. However, it might not be in our 
current translations.

Michele - Can you check this one out?

Todd - This [nsbeta3-] and does not have an [rtm++]. Hence, right now, this 
should not be checked-in!!! Until the PDT approves it. I suggest this waits for 
RTM . . . it is not a critical crashers-type bug.

Comment 44

18 years ago
Adding jenm to cc: list.
(Assignee)

Comment 45

18 years ago
Everyone: just a dialog is not an acceptable solution.  If the user switches 
themes with composer windows open, not only will he lose whatever was in those 
editors, but he'll no longer be able to type in them -- thus rendering all 
those windows useless.

Comment 46

18 years ago
So, what's the patch? Who's reviewed? Who's tested?
(Assignee)

Comment 47

18 years ago
The current patch is just to show the dialog.  I haven't sent it around for 
reviews yet because we're going to need a better solution here (and one that I 
might not be implementing).  

Comment 48

18 years ago
The text will go in even if we do implement another solution to 43350 - i.e.
actually prevent the user from switching without closing these other windows,
right?  If this is true, can we just go ahead and put the dialogue in as a first
step?

Updated

18 years ago
No longer depends on: 43350
(Assignee)

Comment 49

17 years ago
Created attachment 16052 [details] [diff] [review]
patch to readd dialog
(Assignee)

Comment 50

17 years ago
attached a patch just to readd the dialog.  still includes the old text because 
the new text hasn't been finalized yet.  A better solution is still needed, 
though.

Comment 51

17 years ago
Please make all UI changes on or before 10/13. We can't take a change after that 
point for localization.

thanks

Comment 52

17 years ago
Correction - Please make all PDT & L10N APPROVED UI changes on or before 10/13. 

Any UI changes that are conceived, developed, engineered, landed, patched, 
fixed, etc. need to be APPROVED by L10N engineering, since we have passed the UI 
freeze date.

Comment 53

17 years ago
I think the text is final:
"Before you apply a new theme, close all Mail, Composer, Instant Messenger or 
Address Book windows. Otherwise, you will lose any unsaved work.  After 
applying a new theme, click Reload to return to the web page you were viewing 
prior to the theme change."

Any objections?

Comment 54

17 years ago
Good for me...

Comment 55

17 years ago
Wait, if hyatt's fix for 44437 is going to be checked in, then we don't need the
part about reloading the web page.  Adding hyatt to cc.

Comment 56

17 years ago
Looks like the fix for 44437 has been checked into the tree, so I presume it
will be checked in to the commercial branch as well...hyatt?
(Assignee)

Comment 57

17 years ago
Right, but also (as I keep saying), we're going to have to do more here, 
because open composer windows are useless after switching themes.  So either 
the text needs to either say something to the effect of "Otherwise, you will 
lose any unsaved work and open Composer and Mail Compose windows won't work 
properly", making us seem like idiots, or we need to decide on a better fix 
here.

Comment 58

17 years ago
Todd - Please make sure you get L10N approval for this change.

Comment 59

17 years ago
Okay, let's do this:

"Before you apply a new theme, close all Mail, Composer, Instant Messenger, or 
Address Book windows. Otherwise, you'll lose any unsaved work and you'll be 
unable to work in the windows that remained open. After applying a new theme, 
click Reload to return to the web page you were viewing prior to the theme 
change."

Or, if Reload is unnecessary:

"Before you apply a new theme, close all Mail, Composer, Instant Messenger, or 
Address Book windows. Otherwise, you'll lose any unsaved work and you'll be 
unable to work in the windows that remained open."

Comment 60

17 years ago
umm..this bug needs to get a rtm need info soon, or else we're going to have
dataloss, AND
no warning about it either.

Comment 61

17 years ago
Process for non-NS employees is to get patch attached,have reviewers put r=/a=
in the bug, then put [rtm+] in status whiteboard to get it onto PDT radar.  Once
they double-plus it, you can check in to branch.

Comment 62

17 years ago
Montse/Michele - Are you OK with this change?

Comment 63

17 years ago
Yes, data loss is bad. Please fix it asap (no later than next week)
(Assignee)

Comment 64

17 years ago
Created attachment 16595 [details] [diff] [review]
patch with new wording
(Assignee)

Comment 65

17 years ago
attached a patch using the new wording.  In the wording, I changed "or" 
to "and" and "remained" to "remain".

will pass around for review/approval...
Keywords: approval, patch, review

Comment 66

17 years ago
Wording is fine with me...

Comment 67

17 years ago
Is this warning message going to work from the view menu when switching skins?
I get no such warning at the moment and I found that all content in ChatZilla
was lost.  had to close it and restart chatzilla to get it working right again.

Jake

Comment 68

17 years ago
The words look good to me. w=verah (wordsmith/review=verah) (that's a joke.)

Thanks for tidying up the wording, Blake.
a=ben
(Assignee)

Comment 70

17 years ago
reviewed (brendan) and approved (ben) fix in hand.  PDT, can this patch still 
be taken? It prevents dataloss.
Whiteboard: [nsbeta3-][pdtp2][FIX IN HAND] → [nsbeta3-][pdtp2][FIX IN HAND][rtm+]

Comment 71

17 years ago
rtm++
Whiteboard: [nsbeta3-][pdtp2][FIX IN HAND][rtm+] → [nsbeta3-][pdtp2][FIX IN HAND][rtm++]
(Assignee)

Comment 72

17 years ago
Fix checked in to the branch.

* Reopen if you want this in the trunk (we'll have to change the wording...but 
I don't think this needs to be in the trunk.)

* I know the warning really shouldn't have a question mark icon, but to change 
that would be to not use Confirm, which would mean this would need another 
r=/sr=, and I don't want to do that this point.

* Filed bug 56279 to have a pref to turn off this warning, something that I'd 
like to see in 6.01 (or whatever the next point release is).
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED

Comment 73

17 years ago
pmac: please verify on branch builds.  Blake - if ok with you, pls change qa 
contact to pmac.
(Assignee)

Updated

17 years ago
QA Contact: blakeross → pmac

Comment 74

17 years ago
marking verified on all platforms (2000_10_14_08_MN6).
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.