507103 - Composition's "Save" button remembers last "Save as" choice (draft, template, or file), but no indication of current choice in dropdown menu (menuitems should be type="radio")

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[[:Aureliano Buendía]]

Attachment: Screenshot 1: Save button dropdown - what is current choice?

In message compose window TB remember my last choise when I "save as" the message.

But when I remake this action, If I click on button "save" to show dropdown menu list, I can't see any items flagged ON as show on attached screenshot.

I cannot find any dupe of this.

Comment 1

[[:Aureliano Buendía]]

Attachment: Screenshot 2: Security button dropdown with radio-style visual indicator (expected behaviour)

see difference between this screenshot and previous "what is current choice"

Comment 2

[Magnus Melin [:mkmelin]]

> TB remember my last choice when I "save as"

What is remembered?

Comment 3

[[:Aureliano Buendía]]

(In reply to comment #2)
> > TB remember my last choice when I "save as"
> 
> What is remembered?

STR:
1. open a draft and "save as" file (called [A]) to desktop clicking first item on dropdown action list;
2. while draft is open, go to desktop and delete [A];
3. re-"save as" (clicking the button on toolbar on compose window): TB re-save [A] on the desktop, but if I show the dropdow action list, I not have a visual indicator of current choice.


Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090816 Lightning/1.0pre Shredder/3.0b4pre ID:20090816033932

Comment 4

[Thomas D. (:thomas8)]

Confirming exactly as described. The "remember last choice" behaviour is certainly arguable, but not indicating the current choice makes this pretty intransparent.
Even the button tooltip should change, depending on currently remembered choice (Save-as-file vs. save-as-draft vs. save-as-template).
The title bar also keeps "Subject [file:/.../test.txt]" even after changing back to save-as-draft, which I find confusing.

xref bug 78607: Implement Ctrl+Shift+S as a separate shortcut to "Save As File"

Comment 5

[Thomas D. (:thomas8)]

This shouldn't be very hard to fix.

Comment 6

[sakshi]

Hello,

I would like to fix this bug.

Comment 7

[sakshi]

Hello,

Could someone tell me the file in which the security button drop-down is implemented using radio button. I tried to search but could not find it.

Comment 8

[Thomas D. (:thomas8)]

1) Install Dom Inspector *for Thunderbird* plugin (different from DomI for FF)
2) Compose, subject: Test123
3) From TB main window, start DomI (Ctrl+Shift+I)
4) From DomI window's menu:
File > Inspect Chrome Document > Write: Test123
-> Title bar of DomI is now "Write: Test123 - Dom Inspector
View > Document Viewer > Dom Nodes (or same from button on the left)
-> Caption of left panel of DomI is now "Document > Dom Nodes"
-> "Find a Node" button is now enabled (icon has mouse pointer)
5) In DomI window, click "Find a node to inspect by clicking on it" button
6) Go back to composition window, click on security button *dropdown*
7) Go back to DomI window, toolbarbutton of security button is now highlighted
8) Click on all nested [+] twisties until you find the menuitem you're looking for
9) copy the ID of the target menuitem, and search for that ID on MXR code mirror to find the source file

hth

If anyone knows easier strategies for this task, pls share with us :)

Comment 9

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #8)
> 1) Install Dom Inspector *for Thunderbird* plugin (different from DomI for FF)

https://addons.mozilla.org/en-US/thunderbird/addon/dom-inspector-6622/

Comment 10

[Kent James (:rkent)]

"If anyone knows easier strategies for this task, pls share with us :)"

I frequently do precisely what Thomas D suggests. But another alternative, if a menu has a rather unique wording, is to search for those menu words using mxr (http://mxr.mozilla.org/comm-central/), hopefully you'll find a unique identifier for that message, then continue using mxr to backtrace to find the code that generates that message.

Comment 11

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #10)
> "If anyone knows easier strategies for this task, pls share with us :)"
> 
> I frequently do precisely what Thomas D suggests. But another alternative,
> if a menu has a rather unique wording, is to search for those menu words
> using mxr (http://mxr.mozilla.org/comm-central/), hopefully you'll find a
> unique identifier for that message, then continue using mxr to backtrace to
> find the code that generates that message.

Yep.

Translated to our example:

1) search for unique string found in UI
http://mxr.mozilla.org/comm-central/search?string=%22Encrypt+this+message%22

2) scan results for possible unique identiefiers of menuitem you're looking for:
> line 7 -- <!ENTITY menu_securityEncryptRequire.label "Encrypt This Message">
So because we have menu_securityEncryptRequire.label, there might be a unique identifier menu_securityEncryptRequire

3) search for alleged unique identifier:
http://mxr.mozilla.org/comm-central/ident?i=menu_securityEncryptRequire&filter=
-> fail

4) freetext search for alleged unique identifier:
http://mxr.mozilla.org/comm-central/search?string=menu_securityEncryptRequire

5) scan search results for occurences that matter, so we're only looking at xul files (which is where menus are defined), and those lines with <menuitem...>:

5a) http://mxr.mozilla.org/comm-central/source/mail/extensions/smime/content/msgCompSMIMEOverlay.xul#27
http://mxr.mozilla.org/comm-central/source/mail/extensions/smime/content/msgCompSMIMEOverlay.xul#47

5b) http://mxr.mozilla.org/comm-central/source/mailnews/extensions/smime/content/msgCompSMIMEOverlay.xul#27
5b) http://mxr.mozilla.org/comm-central/source/mailnews/extensions/smime/content/msgCompSMIMEOverlay.xul#47

Not sure exactly why we have these dupes between /mailnews/ and /mail/, so trying /mail/...(5a) which is the TB-specific branch

6) inspect code on mxr
http://mxr.mozilla.org/comm-central/source/mail/extensions/smime/content/msgCompSMIMEOverlay.xul#27
-> realize that this is not what we're looking for because the the UI of attachment 394830 [details] has been streamlined to have a single <menuitem type=checkbox> instead of the former two <menuitem>s of type=radio

7) Start over again using strategies of comment 8 and/or comment 10 to find another <menuitem> in the current UI that still has type=radio, e.g.

View > Headers >   All
                 o Normal

Or the spelling button in compose window if you have more than one dictionary installed.

Comment 12

[sakshi]

Hello, 

Thanks for the stratergy. It is really effective. 

Should I add the new menuitem in 
comm-central/mail/extensions/smime/content/msgCompSMIMEOverlay.xul ?

Comment 13

[Thomas D. (:thomas8)]

(In reply to sakshi from comment #12)
> Thanks for the strategy. It is really effective. 
> Should I add the new menuitem in 
> comm-central/mail/extensions/smime/content/msgCompSMIMEOverlay.xul ?

No :)

Because you're not adding a new <menuitem>, you just want to change *existing* <menuitems> of the Save button's menupopup and make them type="radio".

So you need to locate them where they currently are (and then change things there), using the "really effective" strategies of comment 8 (or comment 11). In this case, you really need to use Dom Inspector Addon per comment 8 to locate the menuitems, because none of "Save", "File", or "Template" are unique enough strings to allow efficient search with strategy of comment 11.

So please follow the instructions of comment 8, i. e. install Dom Inspector Addon and use DomI's "Find a node by clicking on it" button to find out the ID attribute of the [Save|v] <toolbarbutton>, and report that back here (or any problems you're facing on the way). There are no stupid questions, but blame it on my mindset as a teacher that I'd like you to find as many answers as possible yourself so that you can learn on the way. You'll learn a lot less if we skip comment 8 and just point you to the file.

(In reply to Thomas D. from comment #8)
> 1) Install Dom Inspector *for Thunderbird* plugin (different from DomI for
> FF)
> 2) Compose, subject: Test123
> 3) From TB main window, start DomI (Ctrl+Shift+I)
> 4) From DomI window's menu:
> File > Inspect Chrome Document > Write: Test123
> -> Title bar of DomI is now "Write: Test123 - Dom Inspector
> View > Document Viewer > Dom Nodes (or same from button on the left)
> -> Caption of left panel of DomI is now "Document > Dom Nodes"
> -> "Find a Node" button is now enabled (icon has mouse pointer)

And, just to ensure that we're seeing the same thing:
View > *Object Viewer* > Dom Node
(showing as "Object - Dom Node" in the caption of DomI's right pane)

> 5) In DomI window, click "Find a node to inspect by clicking on it" button

That's the first button on the left, whose icon features a mouse pointer

> 6) Go back to composition window, click on security button *dropdown*

Here you want to click on the "Save" button dropdown part instead, and if you did it right, you should see a red border flashing up around the button as you single-click without moving mouse, indicating that DomI has found the node you clicked on and is now showing that node selected in the DomI window.

> 7) Go back to DomI window, toolbarbutton of security button is now
> highlighted
> 8) Click on all nested [+] twisties until you find the menuitem you're
> looking for
> 9) copy the ID of the target menuitem, and search for that ID on MXR code
> mirror to find the source file

You'll find the ID in DomI's right pane (Properties pane), titled "Object - Dom Node", in the tabular list of attribute names and values.

Comment 14

[Thomas D. (:thomas8)]

Mental note (for later):

Requirements for the patch:
- in addition to changing the 3 save <menuitems> on the toolbar's menupopup, also change these 3 <menuitems> in composition's *File menu* to type="radio":
File > Save as > File... | Draft | Template
- add javascript code to ensure that the current indication of the two radio groups (toolbar popup and file menu popup) is always in sync. E.g. when user saves using file menu, we need to toggle the radio values of the toolbar popup accordingly.

Comment 15

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #14)
> - add javascript code to ensure that the current indication of the two radio
> groups (toolbar popup and file menu popup) is always in sync. E.g. when user
> saves using file menu, we need to toggle the radio values of the toolbar
> popup accordingly.

...and vice versa :)

Comment 16

[sakshi]

Here is the link where the menuitem for save button is
http://mxr.mozilla.org/comm-central/search?string=button-savePopup.

Comment 17

[Thomas D. (:thomas8)]

(In reply to sakshi from comment #16)
> Here is the link where the menuitem for save button is
> http://mxr.mozilla.org/comm-central/search?string=button-savePopup.

Great. That mxr search returns save button menupopup code:

Thunderbird (aka Mail):
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#719

Seamonkey (aka Suite):
http://mxr.mozilla.org/comm-central/source/suite/mailnews/compose/messengercompose.xul#504

As a courtesy, we can fix this for Seamonkey, too, respective code looks the same.

(In reply to Thomas D. from comment #14)
> Mental note (for later):
> - add javascript code to ensure that the current indication of the two radio
> groups (toolbar popup and file menu popup) is always in sync. E.g. when user
> saves using file menu, we need to toggle the radio values of the toolbar
> popup accordingly.

I think we could add a few lines of javascript code to MsgComposeCommands.js which will keep the two sets of radio buttons in sync. Any other ideas / requirements where/how this should be realized?

Perhaps near this function which is functionally similar:
> 881 function updateAllItems(aDisable)
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#881

What about adding inserting our new function before this one:
929 function openEditorContextMenu(popup)
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#929

Comment 18

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #17)
> I think we could add a few lines of javascript code to MsgComposeCommands.js

FTR, I picked MsgComposeCommands.js because
a) it lives in the same folder (1) with messengercompose.xul and has a similar file name
b) it is already referenced inside messengercompose.xul (2), so it's available in the composition context
c) because it already contains functionally similar menu updating functions (3)

(1) http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
(2) http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#59
(3) http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#881

Comment 19

[sakshi]

view-> folder->  is in sync. This is a link to the code:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2399

Comment 20

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #17)
> I think we could add a few lines of javascript code to MsgComposeCommands.js
> which will keep the two sets of radio buttons in sync. Any other ideas /
> requirements where/how this should be realized?

Richard, can you comment and/or point out code on mxr what you did to keep two sets of radio menuitems synchronized between your appmenu and the main menu, so that switching on one menu will switch the other menu accordingly?

Main menu bar: View > Headers > All vs. Normal
App menu: View > Headers > All vs. Normal

Comment 21

[sakshi]

Attachment: This patch adds type="radio" for the menuitem

Comment 22

[Richard Marti (:Paenglab)]

(In reply to Thomas D. from comment #20)
> (In reply to Thomas D. from comment #17)
> > I think we could add a few lines of javascript code to MsgComposeCommands.js
> > which will keep the two sets of radio buttons in sync. Any other ideas /
> > requirements where/how this should be realized?

I've found this: http://mxr.mozilla.org/comm-central/ident?i=defaultSaveOperation and it looks like the default is only saved during TB is running. After restart the default is again on draft: http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#119

I've found no code where the default menuitem is initialized. This would need new initialization code.

> Richard, can you comment and/or point out code on mxr what you did to keep
> two sets of radio menuitems synchronized between your appmenu and the main
> menu, so that switching on one menu will switch the other menu accordingly?
> 
> Main menu bar: View > Headers > All vs. Normal
> App menu: View > Headers > All vs. Normal

I searched for the menu in View = viewheadersmenu ( http://mxr.mozilla.org/comm-central/search?string=viewheadersmenu ) This pointed me to http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2470. On line 2471 I saw the <menupopup id="menu_HeadersPopup" onpopupshowing="InitViewHeadersMenu();">
The search for http://mxr.mozilla.org/comm-central/ident?i=InitViewHeadersMenu showed me http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#553. I saw here the intialization was made on document.getElementById("cmd_...") and not on menuitem IDs. This gave me the possibility to use the same init function as on main menu.

If it was initialised on menuitem ID then I had to copy the original function and add my function with my AppMenu IDs.

Comment 23

[Thomas D. (:thomas8)]

Comment on attachment 738408 [details] [diff] [review]
This patch adds type="radio" for the menuitem

Review of attachment 738408 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this WIP patch. I adviced Sakshi to initially omit the "menu sync" part so that we can get started with the basics while figuring out the details for syncing per comment 17 ff. Btw, per comment 22 we might also need to ensure a default choice when the menu is first initialized in a TB session.

We need name attribute on all the radio menuitems to identify the radio group - pls fix that.

::: mail/components/compose/content/messengercompose.xul
@@ +446,5 @@
>            <menu id="menu_SaveAsCmd" label="&saveAsCmd.label;" accesskey="&saveAsCmd.accesskey;">
>              <menupopup id="menu_SaveAsCmdPopup">
>                <menuitem id="menu_SaveAsFileCmd"
>                          label="&saveAsFileCmd.label;" accesskey="&saveAsFileCmd.accesskey;"
> +                        command="cmd_saveAsFile;" type="radio"/>

As described in XUL documentation (1), pls add a shared name attribute to all the menuitems which belong to this radio group. I suggest name="radiogroup_SaveAs", for both menus.

(1) https://developer.mozilla.org/en-US/docs/XUL/PopupGuide/MenuItems#Radio_menu_items

Comment 24

[sakshi]

Attachment: patch2: shared name attribute for menuitem and default value

Comment 25

[Ludovic Hirlimann [:Usul]]

Comment on attachment 738849 [details] [diff] [review]
patch2: shared name attribute for menuitem and default value

Thomas can't review code, he can give feedback, but not reviews.

Comment 26

[sakshi]

Okay, who can I set for review?

Comment 27

[sakshi]

(In reply to sakshi from comment #26)
> Okay, who can I set for review?

Sorry, I did not see that you have already set Mike for reviewing.

Comment 28

[Thomas D. (:thomas8)]

(In reply to Ludovic Hirlimann [:Usul] from comment #25)
> Comment on attachment 738849 [details] [diff] [review]
> patch2: shared name attribute for menuitem and default value
> 
> Thomas can't review code, he can give feedback, but not reviews.

(In reply to Ludovic Hirlimann [:Usul] from comment #25)
> Comment on attachment 738849 [details] [diff] [review]
> patch2: shared name attribute for menuitem and default value
> 
> Thomas can't review code, he can give feedback, but not reviews.

That's right, per role definitions. Accordingly, that's why I cleared review request on attachment 738408 [details] [diff] [review], and only provided feedback (using bmo's review feature for precise, fast and easy code feedback). Pls don't request review? from me because I can't grant review+ flag. Just request feedback? instead.

Comment 29

[Thomas D. (:thomas8)]

Comment on attachment 738849 [details] [diff] [review]
patch2: shared name attribute for menuitem and default value

As explained in Comment 23, this WIP patch is still incomplete and hence not  ready for review yet, so let's preserve Mike's precious review time for later and avoid double work.

I'm mentoring Sakshi and we'll re-request review from Mike (:mconley) when the patch is ready and complete.

Comment 30

[Thomas D. (:thomas8)]

Comment on attachment 738849 [details] [diff] [review]
patch2: shared name attribute for menuitem and default value

Review of attachment 738849 [details] [diff] [review]:
-----------------------------------------------------------------

Disclaimer: This is not a review. I'm providing *feedback* on the code using bmo's splinter feature.

Sakshi, pls address comments on the code below.

::: mail/components/compose/content/messengercompose.xul
@@ +446,5 @@
>            <menu id="menu_SaveAsCmd" label="&saveAsCmd.label;" accesskey="&saveAsCmd.accesskey;">
>              <menupopup id="menu_SaveAsCmdPopup">
>                <menuitem id="menu_SaveAsFileCmd"
>                          label="&saveAsFileCmd.label;" accesskey="&saveAsFileCmd.accesskey;"
> +                        command="cmd_saveAsFile" name="radiogroup_SaveAs" checked="true"/>

We need /both/ attributes, type="radio" and name="...", on all of the respective menuitems.

Pls don't add checked="true" bc we shouldn't hardcode indication of default SaveAs flavor in the UI / xul (besides, saveAsFile is not the default). Instead, per Richard's comment 22, we need some javascript to check the actual initial value of the variable defaultSaveOperation (1) and then set the checked attribute of initial default dynamically from within the script. Perhaps that could even be the same script we'll need for synchronizing between the two menus.

(1) http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#120

Comment 31

[sakshi]

I tried to implement a function to keep the two menuitmens in sync based on the function given here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2399

But it is not working. What should I replace gFolderTreeView.mode with?? Or should I go for another approach in writing the code?

Comment 32

[Richard Marti (:Paenglab)]

I'm not a JS specialist but I would try something like this function: http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#553 but not use the preferences query (it's not a preference we have). You should try to check the defaultSaveOperation variable (http://mxr.mozilla.org/comm-central/ident?i=defaultSaveOperation) and set the "checked" correlating to the value this variable has.

As said before, I'm more a copy/paste JS programmer and don't know if my proposal works.

Comment 33

[sakshi]

Well that is what I had in mind initially, but I did not know what should I replace Components.interfaces.nsMimeHeaderDisplayTypes in the new function.

Comment 34

[Thomas D. (:thomas8)]

This is the existing function comment 32 and comment 33 refer to:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#553

553 function InitViewHeadersMenu()
554 {
555   const dt = Components.interfaces.nsMimeHeaderDisplayTypes;
556   var headerchoice = Services.prefs.getIntPref("mail.show_headers");
557   document.getElementById("cmd_viewAllHeader")
558           .setAttribute("checked", headerchoice == dt.AllHeaders);
559   document.getElementById("cmd_viewNormalHeader")
560           .setAttribute("checked", headerchoice == dt.NormalHeaders);
561   document.commandDispatcher.updateCommands("create-menu-mark");
562 }

(In reply to sakshi from comment #33)
> Well that is what I had in mind initially, but I did not know what should I
> replace Components.interfaces.nsMimeHeaderDisplayTypes in the new function.

For your new function:
* Omit line 555 and 556, because we don't have that level of abstraction here
* document.getElementById("cmd_saveAsFile")
          .setAttribute("checked", defaultSaveOperation == "file");
* add code for the other two commands the same way
* check if that updates radio buttons across menus without the explicit updating code of line 561; if not, we need to tweak that line for our purposes here (slightly more tricky).

Sakshi - can you pls try this and report back if it works... :)

How does it work?

> document.getElementById("cmd_saveAsFile")
This gets the <command> element with id="cmd_saveAsFile"
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#112

>          .setAttribute("checked", defaultSaveOperation == "file");
If (defaultSaveOperation == "file") returns /true/, this will create and set/toggle the checked attribute of the *<command>* to /true/, otherwise /false/:
112   <command id="cmd_saveAsFile" oncommand="goDoCommand('cmd_saveAsFile')" checked="true"/>

So instead of changing the checked attribute on each menuitem, we're just setting the checked attribute on the central <command> element itself to which the respective menuitems are pointing (linked with command attribute). That's a smart way of setting their checked attribute remotely:

https://developer.mozilla.org/en-US/docs/XUL/command
> Like a broadcaster, commands forward attributes to other elements.

More explanation here:
https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Broadcasters_and_Observers#Command_Attribute_Forwarding
> We've already seen that elements such as buttons can be hooked up to commands.
> In addition, if you place the disabled attribute on the command element, any
> elements hooked up to it will also become disabled automatically. This is a
> useful way to simplify the amount of code you need to write. The technique
> also works for other attributes as well.

Then we need to check if clicking the commands actually changes the defaultSaveOperation variable (or if it's just the initial default). So let's track the command handling for cmd_saveAsFile (using mxr searches, these are the results):
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#112
http://mxr.mozilla.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js#449
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3027
> 3027 function SaveAsFile(saveAs)
> 3036   defaultSaveOperation = "file";
> 3037 }

So each time the user saves in one of the flavors, that flavor is saved in defaultSaveOperation variable for the current session. The next time you save your composition, the same flavor will be used.

So finally, what are the values that defaultSaveOperation can have?
Some lines above SaveAsFile() function, there's Save() function (File > Save, or Ctrl+S):

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3017

3017 function Save()
3018 {
3019   switch (defaultSaveOperation)
3020   {
3021     case "file"     : SaveAsFile(false);      break;
3022     case "template" : SaveAsTemplate(false);  break;
3023     default         : SaveAsDraft(false);     break;
3024   }
3025 }

...where default is "draft", as seen here:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#3045

Comment 35

[Thomas D. (:thomas8)]

OT: [Bugfix Tutorial] I'd like to keep track of those bugs which provide good samples / tutorials for new contributors how to go about fixing bugs.

Comment 36

[sakshi]

Well, I did the changes mentioned by Thomas, but it still does not work. May be we have to update it somehow.

Comment 37

[Thomas D. (:thomas8)]

(In reply to sakshi from comment #36)
> Well, I did the changes mentioned by Thomas, but it still does not work. May
> be we have to update it somehow.

We'll certainly need more information than "it still does not work" if you want advice ;)

Can you attach another "work in progress" (wip) patch to request feedback?

Comment 38

[sakshi]

Attachment: Patch: Add radio group behaviour to menuitems in "Save As" submenus

Added a function, but the two menuitems are still not in sync.

Comment 39

[Thomas D. (:thomas8)]

Comment on attachment 739932 [details] [diff] [review]
Patch: Add radio group behaviour to menuitems in "Save As" submenus

Well done, looks good to me (from reading the patch, not testing), don't see anything wrong here, does anybody?

Sakshi, anything in Tools > Error Console after clicking on our menuitems?
Did you verify that the function actually gets called (e.g., place alert('hello world') inside the function)? Does the function have access to defaultSaveOperation variable (e.g. test with alert(defaultSaveOperation) inside the function, or some more suitable debug output to error console)?

If there are no errors and everything except the actual visual updating seems to work as expected (which I assume is the case), then I conclude we do need that extra line which forces the update:

(In reply to Thomas D. from comment #34)
> http://mxr.mozilla.org/comm-central/source/mail/base/content/
> mailWindowOverlay.js#561
> 561   document.commandDispatcher.updateCommands("create-menu-mark");
> we need to tweak that line for our purposes here (slightly more tricky).

So we need to find out how that updateCommands() function works, and then add an appropriate similar line at the end of your InitFileSaveAsMenu() function.

Comment 40

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #39)
> Comment on attachment 739932 [details] [diff] [review]
> So we need to find out how that updateCommands() function works, and then
> add an appropriate similar line at the end of your InitFileSaveAsMenu()
> function.

Perhaps this can help:

https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Updating_Commands#Command_Updaters

Comment 41

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #40)
> (In reply to Thomas D. from comment #39)
> > Comment on attachment 739932 [details] [diff] [review]
> > So we need to find out how that updateCommands() function works, and then
> > add an appropriate similar line at the end of your InitFileSaveAsMenu()
> > function.
> 
> Perhaps this can help:
> 
> https://developer.mozilla.org/en-US/docs/XUL/Tutorial/
> Updating_Commands#Command_Updaters

Plus tracking the codeflow of existing updateCommands() function on MXR, starting from http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#561

Comment 42

[sakshi]

Yes, I tried that. The function is getting called. When I tried alert(defaultSaveOperation), it only gives 'draft'. SO I think it is not getting updated.

Comment 43

[Richard Marti (:Paenglab)]

Here it works. Initial is "Draft". When I use "File", both the button menu and menu have the radio set to "File". Pressing the "Save" button saves also to file now.

As wrote before, this setting is only stored as long this window is open. After close and reopen a new write window it's again on "Draft" and it's like it was all the time before. If the setting should be stored during the TB session it needs a global variable where status is set (not sure if this is right). If it should be set over TB sessions then it needs to be stored in a preference.

Comment 44

[Thomas D. (:thomas8)]

(In reply to sakshi from comment #42)
> Yes, I tried that. The function is getting called. When I tried
> alert(defaultSaveOperation), it only gives 'draft'. SO I think it is not
> getting updated.

Since you're using onPopupShowing, the alert will always show you the *previous* value of defaultSaveOperation. If you click on "Save as > File", perform saving, then re-open the menu again from the same composition without closing it, the *next* time the alert should be 'file', and on both menus (File > Save As > ... and Toolbar Save-Button dropdown menu), the radio checkmark (round dot) should be on "File". Does that happen?

Comment 45

[sakshi]

The radio checkmark is working fine. But the alert always gives 'draft'. Also the radio checkmark on both the menuitems never become the same. How about using 'create-menu-file' for update?

Comment 46

[sakshi]

Richard, as per your comment 43, is there any more changes I must do?

Comment 47

[Richard Marti (:Paenglab)]

I would say it's complete like it is. I've sent Thomas a Try build. So let's him decide if it's okay.

Comment 48

[sakshi]

Ya sure.

Comment 49

[Thomas D. (:thomas8)]

(In reply to Richard Marti [:Paenglab] from comment #47)
> I would say it's complete like it is. I've sent Thomas a Try build. So let's
> him decide if it's okay.

Very okay :)

Code of Sakshi's current patch attachment 739932 [details] [diff] [review], based on my comment 34, works like a charme without any additions.

I'm on Windows XP, and I've tested both by tweaking my local installation and using Richard's try build, and both radiogroups are synchronized and behaving as expected all the way, including showing the correct initial default.

So unless this is something OS-specific which only fails on Linux, patch attachment 739932 [details] [diff] [review] is ready for review :)

Comment 50

[Thomas D. (:thomas8)]

Comment on attachment 739932 [details] [diff] [review]
Patch: Add radio group behaviour to menuitems in "Save As" submenus

>+function InitFileSaveAsMenu()
>+{
>+    document.getElementById("cmd_saveAsFile")
>+            .setAttribute("checked", defaultSaveOperation == "file");
>+    document.getElementById("cmd_saveAsDraft")
>+            .setAttribute("checked", defaultSaveOperation == "draft");
>+    document.getElementById("cmd_saveAsTemplate")
>+            .setAttribute("checked", defaultSaveOperation == "template");
>+}
>+

Nitfix: Pls reduce the indentation of the inner block from 4 to 2 spaces.

Comment 51

[sakshi]

Attachment: Patch: Add radio group behaviour to menuitems in "Save As" submenus

Comment 52

[Thomas D. (:thomas8)]

Comment on attachment 740042 [details] [diff] [review]
Patch: Add radio group behaviour to menuitems in "Save As" submenus

Perfect. Tested working exactly as described per Comment 43 and Comment 49.

Comment 53

[Thomas D. (:thomas8)]

Comment on attachment 740042 [details] [diff] [review]
Patch: Add radio group behaviour to menuitems in "Save As" submenus

Mike (:mconley), if you could spare a minute for this little review-to-go, the second patch from new contributor Sakshi who was eager to build on the successful fast fix experience of bug 581470, also encouraged by your rapid review :)

To ease your review, :paenglab and I have already scrutinized and tested the patch, looks & works flawlessly as expected.

The patch adds radio group behaviour to composition's two SaveAs submenus because we already remember the flavor per composition window (save as draft|file|template). Code is straightforward:

* add type="radio" and name="radiogroup_SaveAs" to SaveAs <menuitems> in
  messengercompose.xul
* for initializing the two menus and keeping the radio checkmarks in sync:
- add onpopupshowing="InitFileSaveAsMenu()" to the <menupopup>s
- add InitFileSaveAsMenu() function in MsgComposeCommands.js

In that function, we simply toggle the checked attribute of the respective SaveAs <commands> in messengercompose.xul. Because of Command Attribute Forwarding (1), the checked attribute then automatically propagates to the respective radio menuitems which observe these commands.

(1) https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Broadcasters_and_Observers#Command_Attribute_Forwarding

Comment 54

[Magnus Melin [:mkmelin]]

Comment on attachment 740042 [details] [diff] [review]
Patch: Add radio group behaviour to menuitems in "Save As" submenus

Review of attachment 740042 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +930,5 @@
> +          .setAttribute("checked", defaultSaveOperation == "file");
> +  document.getElementById("cmd_saveAsDraft")
> +          .setAttribute("checked", defaultSaveOperation == "draft");
> +  document.getElementById("cmd_saveAsTemplate")
> +          .setAttribute("checked", defaultSaveOperation == "template");

Since this is a radio group, i don't think you're supposed to do update every item manually like this. You just setAttribute("checked", "true") for the item that's supposed to be checked, if any.

Besides, for checked is false removeAttribute("checked") should be used.

::: mail/components/compose/content/messengercompose.xul
@@ +443,5 @@
>            <menuseparator/>
>            <menuitem id="menu_SaveCmd" label="&saveCmd.label;" accesskey="&saveCmd.accesskey;"
>                      key="key_save" command="cmd_saveDefault"/>
>            <menu id="menu_SaveAsCmd" label="&saveAsCmd.label;" accesskey="&saveAsCmd.accesskey;">
> +            <menupopup id="menu_SaveAsCmdPopup" onpopupshowing="InitFileSaveAsMenu()">

Nit: InitFileSaveAsMenu();

@@ +717,5 @@
>      <toolbarbutton class="toolbarbutton-1" type="menu-button"
>                 id="button-save" label="&saveButton.label;"
>                 tooltiptext="&saveButton.tooltip;"
>                 command="cmd_saveDefault">
> +         <menupopup id="button-savePopup" onpopupshowing="InitFileSaveAsMenu()">

Nit: InitFileSaveAsMenu();

Comment 55

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #54)
> Comment on attachment 740042 [details] [diff] [review]
> Patch: Add radio group behaviour to menuitems in "Save As" submenus
> 
> Review of attachment 740042 [details] [diff] [review]:
> -----------------------------------------------------------------

Magnus, thanks for the drive-by review.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +930,5 @@
> > +          .setAttribute("checked", defaultSaveOperation == "file");
> > +  document.getElementById("cmd_saveAsDraft")
> > +          .setAttribute("checked", defaultSaveOperation == "draft");
> > +  document.getElementById("cmd_saveAsTemplate")
> > +          .setAttribute("checked", defaultSaveOperation == "template");
> 
> Since this is a radio group, i don't think you're supposed to do update
> every item manually like this. You just setAttribute("checked", "true") for
> the item that's supposed to be checked, if any.

Well, as explained in Comment 34, we just copied existing code which is already there and working flawlessly...:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#553

Maybe there's a misunderstanding about the purpose of this code. We're using this to keep two different radio groups synchronized. We are *not* updating <menuitem>s of any single radiogroup itself. We're updating *<command>s* so that all the <menuitems> (or any other UI elements!) that observe these commands will then update automatically. If we follow your suggestion to just update the command which will become the new default (checked), both radiogroups in the UI will still work correctly. However, internally, the other two <command>s will still have checked="true" even though they are not checked in the UI nor the current default programmatically. I think that's a disadvantage. If somebody wants to add other UI elements, like a single(!) checkbox showing whether "Save as File" is currently default or not, it will be more difficult to find the right status. With our proposed solution, you can just observe each command separately as it will always have the correct checked attribute.

Is this what you are proposing?

function InitFileSaveAsMenu()
{
  switch (defaultSaveOperation) {
    case "file":
      document.getElementById("cmd_saveAsFile")
              .setAttribute("checked", true);
      break;
    case "template":
      document.getElementById("cmd_saveAsTemplate")
              .setAttribute("checked", true);
      break;
    default:
      document.getElementById("cmd_saveAsDraft")
              .setAttribute("checked", true);
  }
}

If we use removeAttribute("checked") as you proposed, that's even more code.
My impression is that this approach is not beneficial in terms of code size, readability, and even performance.

> Besides, for checked is false removeAttribute("checked") should be used.

Again, I think we'll just need more and more complex code to do that, without substantial benefit. Documentation even mentions benefits of *not* using removeAttribute("checked"), e.g. in the context of persistance:

https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-checked
> ... (i.e. do menuitem.setAttribute("checked", "false") instead of
> menuitem.removeAttribute("checked")) when the user unchecks the menuitem, as a
> value of false will both correctly hide the checkmark and persist its hidden
> state.

> > onpopupshowing="InitFileSaveAsMenu()">
> 
> Nit: InitFileSaveAsMenu();

Sure.

Comment 56

[Magnus Melin [:mkmelin]]

Ah, I missed the part that it's the command, in that case i suppose i don't have a problem with it. 

https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-checked also says
 Use hasAttribute() to determine whether this attribute is set instead of getAttribute()

I do interpret that as setting it to "false" isn't the preferred way.
Anyways, maybe "false" is fine in this case.

Comment 57

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #56)
> Ah, I missed the part that it's the command, in that case i suppose i don't
> have a problem with it. 

Thanks for quick feedback.

> https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-checked also says
>  Use hasAttribute() to determine whether this attribute is set instead of
> getAttribute()

Yeah, for those cases of attribute minimization which just use <tag checked/> as a shorthand for the more complete attribute=value pair. Well, we're not using either of hasAttribute or getAttribute (only setAttribute true|false), so at least it's fully under our control, and attribute minimization won't ever occur.

Comment 58

[Thomas D. (:thomas8)]

Sakshi, would you pls add a new patch addressing the nits per Magnus' review comment 54:

> > onpopupshowing="InitFileSaveAsMenu()">
> Nit: InitFileSaveAsMenu();
...for both popup menus.

Everything else is OK as it stands, per Magnus' comment 56.

Comment 59

[sakshi]

Attachment: Patch: Add radio group behaviour to menuitems in "Save As" submenus

Comment 60

[Magnus Melin [:mkmelin]]

Comment on attachment 740225 [details] [diff] [review]
Patch: Add radio group behaviour to menuitems in "Save As" submenus

Review of attachment 740225 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, r=mkmelin

Comment 61

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #60)
> Review of attachment 740225 [details] [diff] [review]
Thanks! That was quick! :)

Comment 62

[Richard Marti (:Paenglab)]

I've set checkin-needed as keyword to inform this patch is ready for check-in.

Comment 63

[Thomas D. (:thomas8)]

(In reply to Richard Marti [:Paenglab] from comment #62)
> I've set checkin-needed as keyword to inform this patch is ready for
> check-in.

...which means applying the changes of your patch to the original source code files:
https://hg.mozilla.org/comm-central/

...from where they will be picked up by the automated build system for compiling and creating installation files which can then be downloaded.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/ae455e4b6d11

Comment 65

[Thomas D. (:thomas8)]

Given the slow release cycle of TB, I believe we should land most things as early as possible, so I'm proposing this for uplifting to Aurora and Beta.

Risk evaluation: This is a small and absolutely no-risk patch.

Comment 66

[:aceman]

Then I think you should request the approval‑comm‑aurora? flags on the patch, and leave the status-* flags for standard8.

Also, isn't this patch missing a ui-review? We can assume paenglab silently agrees with the patch by watching this bug, but it is not explicitly marked anywhere. Of course, ui-review in this case may be a formality, as this is just keeping existing style of buttons with dropdowns.

But I like the change. Good work guys :)

Comment 67

[Mark Banner (:standard8)]

Not tracking, as this isn't a) a regression, b) something that's significant for users.

If you want approval then you should be requesting the approval-comm-aurora/beta flags on the patch. However, in this case, I do not think we need to advance this forward fast - it is a small piece of additional functionality that is unlikely to break anything, and hence shouldn't need major testing. It also has 2 release cycles before the next major release anyway.

Note that I did just find a bug in the toolbar menu - I filed bug 870313.

Comment 68

[Blake Winton (:bwinton) (:☕️)]

Since it's a user-facing change, it should get a ui-review.  Further, since Paenglab has already been in this bug, you should request it from him.  ;)