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.

skin triage team: nsbeta3+ added dependency bugs, but we can go ahead and implement this bug

.

fixed

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.

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.

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.

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?

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.

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.

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.

vrfy fixed with a build just pulled on win98

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.

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

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)

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

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.

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

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)

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)

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.

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

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.

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.

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.

> 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-

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?

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.

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...

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?

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....

Could someone let me know, again, what the current wording of the dialog is? Sorry, I've never seen the actual dialog.

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.

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.

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...

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.

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.

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.

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?

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'.

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

"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."

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.

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.

Adding jenm to cc: list.

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.

So, what's the patch? Who's reviewed? Who's tested?

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).

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?

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.

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

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.

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?

Good for me...

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.

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?

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.

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

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."

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.

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.

Montse/Michele - Are you OK with this change?

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

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...

Wording is fine with me...

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

The words look good to me. w=verah (wordsmith/review=verah) (that's a joke.) Thanks for tidying up the wording, Blake.

a=ben

reviewed (brendan) and approved (ben) fix in hand. PDT, can this patch still be taken? It prevents dataloss.

rtm++

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).

pmac: please verify on branch builds. Blake - if ok with you, pls change qa contact to pmac.

marking verified on all platforms (2000_10_14_08_MN6).