Closed Bug 49759 Opened 24 years ago Closed 24 years ago

Put the theme switching warning dialog back

Categories

(Core Graveyard :: Skinability, defect, P2)

Product:
Core Graveyard
Component:
Skinability
Platform:
x86
Other
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: johng, Assigned: bugzilla)

References

Details

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

Attachments

(2 files)

patch to readd dialog
2.71 KB, patch
Details | Diff | Splinter Review
patch with new wording
2.60 KB, patch
Details | Diff | Splinter Review 
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
Depends on: 43350, 44437, 47345
Keywords: nsbeta3
Whiteboard: [nsbeta3+]
.
Assignee: ben → BlakeR1234
fixed
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
vrfy fixed with a build just pulled on win98
Status: RESOLVED → VERIFIED
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+]
Summary: Remove the skin switch warning dialog → Put the skin switch warning dialog back (revert the previous fix for this bug)
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]
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.
Assignee: trudelle → blakeross
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)
Keywords: rtm
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
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
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-
Whiteboard: [nsbeta3+][pdtp2] → [nsbeta3-][pdtp2]
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.
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]
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?
No longer depends on: 43350

    
    

    
  
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...
Keywords: approval, 
          
            patch, 
          
            review

    
    

    
  
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.

    
    

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

    
    

    
  
rtm++
Whiteboard: [nsbeta3-][pdtp2][FIX IN HAND][rtm+] → [nsbeta3-][pdtp2][FIX IN HAND][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).
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED

    
    

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

    
  
QA Contact: blakeross → pmac

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: