127993 - View | Apply Theme should use check for current theme

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect)

Description

[Jerry]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; Q312461)
BuildID:    20020206

when user select one theme for mozilla,
after sometime, he doesnot know which theme is currently used
for there is no checked flag for selected themes.

Reproducible: Always
Steps to Reproduce:
1.open View menu
2.click apply-theme
3.

Actual Results:  it lists all theme avaiable with no one checked.

Expected Results:  it lists all theme avaiable with the current one checked.
so the user know oh, I am using this one now.

Comment 1

[Jerry]

assign it to me , I want to do it.

Comment 2

[Jerry]

I have one patch for it, modified navigatorOverlay.xul and navigator.js, works 
fine except one thing.I dont know how to set checked flag when browser startup.

Index: xpfe/browser/resources/content/navigatorOverlay.xul
===================================================================
RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigatorOverlay.xul,v
retrieving revision 1.252
diff -u -r1.252 navigatorOverlay.xul
--- xpfe/browser/resources/content/navigatorOverlay.xul	27 Mar 2002 
03:17:53 -0000	1.252
+++ xpfe/browser/resources/content/navigatorOverlay.xul	29 Mar 2002 
06:11:24 -0000
@@ -267,7 +267,8 @@
             <menuitem uri="..."
                       label="rdf:http://www.mozilla.org/rdf/chrome#displayName"
                       
accesskey="rdf:http://www.mozilla.org/rdf/chrome#accessKey"
-                      name="rdf:http://www.mozilla.org/rdf/chrome#name"/>
+                      name="themeGroup"
+                      type="radio"/>                      
             </template>
           </menupopup>
         </menu>

I know I should put one line like checked="..." in this file,
but I dont know what is the check flag for theme.

hewitt, can you help me?

Comment 3

[Jerry]

Attachment: first patch

I post first patch here.
the problem of it is that:
 in xul, it should have one checked flag, to tell user what is the theme
currectly used now when browser is up.

Comment 4

[Jerry]

according to blake's suggestion, the patch need to update preference module
when user change theme in preference, it will affect check flag here.

I think that it may be done in two ways.

1.  every theme has one checked flag, so whereever user change theme, user will
also set flag for every theme. 
just like this:
 modify chrome.rdf, add one line
<RDF:Description about="urn:mozilla:skin:classic/1.0"
                   c:displayName="Classic"
                   c:accessKey="C"
                   c:author="mozilla.org"
                   c:description="This theme simulates the familiar appearance
of previous Netscape versions for the Windows operating system."
                   c:name="classic/1.0"
                  
c:image="resource:/chrome/classic/skin/classic/global/preview.gif"
                   c:checked="true"
                   c:locType="install">
    <c:packages resource="urn:mozilla:skin:classic/1.0:packages"/>
  </RDF:Description>
  
  and modify navigatorOverlay.xul like this  
   <menuitem uri="..."
                      label="rdf:http://www.mozilla.org/rdf/chrome#displayName"
                      accesskey="rdf:http://www.mozilla.org/rdf/chrome#accessKey"
                      name="themeGroup"
                      checked="rdf:http://www.mozilla.org/rdf/chrome#checked"
                      type="radio"/>
            </template>

but patch for this need to update chrome.rdf, including function in menu and in
preference windows.


2. dont modify chrome, 
modify navigatorOverlay.xul like this  
   <menuitem uri="..."
                      label="rdf:http://www.mozilla.org/rdf/chrome#displayName"
                      accesskey="rdf:http://www.mozilla.org/rdf/chrome#accessKey"
                      name="themeGroup"
                     
checked="checkTheme(rdf:http://www.mozilla.org/rdf/chrome#name)"
                      type="radio"/>
            </template>

modify navigator.js like this            
function checkTheme(name) 
{
  var chromeRegistry = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
      .getService(Components.interfaces.nsIChromeRegistry);	
  var selected = chromeRegistry.isSkinSelected(name, true);
  if (selected == Components.interfaces.nsIChromeRegistry.FULL) 
    return true;
  else 
    return false;
}

but my question is that
checked="checkTheme(rdf:http://www.mozilla.org/rdf/chrome#name)" doesnot work.

please give me some suggestion.
        

Comment 5

[Jerry]

Attachment: updated patch

Comment 6

[Jerry]

thank for blaker's suggestion,
I do some change to my patch.

add onpopupshowing to theme menu, 
so when theme menu popup, it will compare  menuitem's id with currently used theme,
if it is, set checked.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 78671 [details] [diff] [review]
updated patch

r=bryner

Comment 8

[Blake Ross]

Comment on attachment 78671 [details] [diff] [review]
updated patch

sr=blake

Comment 9

[Matthias Versen [:Matti]]

*** Bug 138844 has been marked as a duplicate of this bug. ***

Comment 10

[Jerry]

Attachment: changed patch for latest trunk

As there are changes to nsIChromeRegistry interface,
so make another patch for latest trunk.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 80354 [details] [diff] [review]
changed patch for latest trunk

r=bryner

Comment 12

[Blake Ross]

Comment on attachment 80354 [details] [diff] [review]
changed patch for latest trunk

sr=blake

Comment 13

[Carl Schrader]

On build 2002042403 the current theme is marked on the view menu, however, it's not 
consistant with the other marked items. The Theme marking is a circle bullet yet
the rest of the menus show checked items using a check. Shouldn't we make this
consistant? The checkmark looks better to me...

Comment 14

[Jacob Kjome]

No, I don't think it is inconsistent.  The bullet is for things that are
uniquely selected.  Take a look at "Use Style" or "Character Coding".  In each
case, only one thing can be selected.  In the case of something like
"Show/Hide", those items have a checkmark because multiple things in that menu
can be selected because they are just options that can be toggled on and off and
are somewhat independent of each other.

so, I think the bullet fix is the correct one.

Jake

Comment 15

[[e-mail]]

Um, is it a good idea to set id on templated items?
I thought that the template needs to set its own ids on the items...

Comment 16

[Jerry]

fix has been checked into trunk by petez.
verified with 20020424 build.

Comment 17

[Jerry]

if menuitem's type is checkbox, or radio box without the same name,
it is inconsistent.

In the patch, I set menuitem type as radio and with the same name,
so dont warry about inconsistent.

Comment 18

[Jerry]

*** Bug 54681 has been marked as a duplicate of this bug. ***

Comment 19

[Jesse Ruderman]

When I first upgraded from 0426 to 0509, there wasn't a bullet next to any of
the items (I use Modern).  After selecting "Modern" in the menu, the bullet
appeared immediately and persisted through a restart.

Comment 20

[benc]

V/fixed. Mozilla 1.6f, allplats.

Bug 141090 still applies, the check is not set for newly created profiles by
default.