1297425 - Offer a way to save an event/task in a tab without closing the tab

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Paul Morris [:pmorris]]

Currently the only option to save an event/task in a tab involves closing the tab.  It would be good to be able to just save it.  This might also involve adding a Save option to the toolbar and/or adding Save, Save and Close, and Delete to the file menu.

Comment 1

[Paul Morris [:pmorris]]

Attachment: 0-rename-save-to-save-and-close.patch

1/2 patches for adding save button to event/task toolbars.

Comment 2

[Paul Morris [:pmorris]]

Attachment: 1-add-save-button.patch

2/2 patches for adding save button to event/task toolbars.  

We will probably want a different icon for "save" and "save and close," but I have left this for a follow-up patch, especially given that this impacts localization.  Next step is to add a save menu item and keyboard shortcut.

Comment 3

[Paul Morris [:pmorris]]

Attachment: 0-rename-save-to-saveandclose-2.patch

Accidentally uploaded the wrong file.  This is the 1st of 2 patches to add a save button to event/task toolbars.

Comment 4

[Paul Morris [:pmorris]]

Attachment: 2-add-save-menu-item.patch

Third in the series.  Adds a Save item to the File menu with keyboard shortcut and access key.

Known issue: The "Save" item does not consistently stay disabled when regular TB tabs are active.

To do: add this menu item to the top level of the app/hamburger menu, but hide it when an event/task tab is not active.

Comment 5

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 8816182 [details] [diff] [review]
1-add-save-button.patch

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

Just a suggestion, I don't mind if you keep it as is though:

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +336,5 @@
>                  ? cal.createDateTime(aState.initialStartDate) : getDefaultStartDate();
>  
> +            aState.args.onOk = function(item, calendar, originalItem, listener) {
> +                doTransaction("modify", item, calendar, originalItem, listener);
> +            };

aState.args.onOk = doTransaction.bind(null, "modify");

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 8816186 [details] [diff] [review]
0-rename-save-to-saveandclose-2.patch

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

I didn't check this patch too closely as it looks like it is just renames. As the migration code will stick around forever, is it really needed or could we get away with not migrating?

Comment 7

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 8816562 [details] [diff] [review]
2-add-save-menu-item.patch

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

r+ with comments:

::: calendar/base/content/calendar-common-sets.js
@@ +233,5 @@
>  
> +            // for saving events/tasks in a tab
> +            case "cmd_save":
> +                let tabType = document.getElementById("tabmail").currentTabInfo.mode.type;
> +                return tabType == "calendarTask" || tabType == "calendarEvent";

You'll need to add brackets if you use variables inside a case block, or you will get the following eslint error:

  235:13  error  Unexpected lexical declaration in case block.  no-case-declarations (eslint)

::: calendar/lightning/content/lightning-item-panel.xul
@@ +186,5 @@
>                 modifiers="accel"
>                 key="&event.dialog.close.key;"
>                 command="cmd_item_close"/>
> +          <key id="save-key-2"
> +               modifiers="accel, shift"

Not quite sure why you need to change the id here? Given the id is not the entity, no need to change for localization reasons.

::: calendar/lightning/content/lightning-menus.xul
@@ +7,5 @@
>    <!ENTITY % lightningDTD SYSTEM "chrome://lightning/locale/lightning.dtd"> %lightningDTD;
>    <!ENTITY % calendarDTD SYSTEM "chrome://calendar/locale/calendar.dtd" > %calendarDTD;
>    <!ENTITY % toolbarDTD SYSTEM "chrome://lightning/locale/lightning-toolbar.dtd" > %toolbarDTD;
>    <!ENTITY % menuOverlayDTD SYSTEM "chrome://calendar/locale/menuOverlay.dtd" > %menuOverlayDTD;
> +  <!-- calendar-event-dialog.dtd is used for Save menu item (ltnSave) -->

No need for a comment here (which I am also not sure is legal inside a doctype definition)

::: calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
@@ +71,5 @@
>  <!ENTITY  event.menu.item.close.label                     "Close">
>  <!ENTITY  event.menu.item.close.accesskey                 "C">
>  <!ENTITY  event.menu.item.save.label                      "Save">
>  <!ENTITY  event.menu.item.save.accesskey                  "S">
> +<!ENTITY  event.menu.item.save.two.accesskey              "a">

This either needs a more descriptive name, or you should add a LOCALIZATION_NOTE for the two save accesskey entities.

Comment 8

[[:MakeMyDay]]

Comment on attachment 8816182 [details] [diff] [review]
1-add-save-button.patch

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

::: calendar/lightning/content/lightning-item-toolbar.xul
@@ +22,5 @@
> +                   mode="dialog"
> +                   class="cal-event-toolbarbutton toolbarbutton-1"
> +                   label="&event.toolbar.save.label;"
> +                   tooltiptext="&event.toolbar.save.tooltip;"
> +                   command="cmd_save"/>

You are recycling the previously string names here, but now they have a different meaning. E.g. the existing label is Save and Close and you now use it just for Save. That said, please count up the string names here event.toolbar.save.label2 and event.toolbar.save.tooltip2, otherwise this may result in a localization problem.

Comment 9

[Paul Morris [:pmorris]]

(In reply to Philipp Kewisch [:Fallen] from comment #6)
> Comment on attachment 8816186 [details] [diff] [review]
> 0-rename-save-to-saveandclose-2.patch
> 
> Review of attachment 8816186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't check this patch too closely as it looks like it is just renames.
> As the migration code will stick around forever, is it really needed or
> could we get away with not migrating?

Without the migration code the "Save and Close" button will disappear from event dialog toolbars that have been customized by users.  They would have to customize their toolbar again to get it back.  On the other hand, not renaming would be confusing for developers.  So I think the migration code is worth it.

Comment 10

[Paul Morris [:pmorris]]

(In reply to Philipp Kewisch [:Fallen] from comment #7)
> Comment on attachment 8816562 [details] [diff] [review]
> 2-add-save-menu-item.patch
> 
> Review of attachment 8816562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments:
> 
> ::: calendar/base/content/calendar-common-sets.js
> @@ +233,5 @@
> >  
> > +            // for saving events/tasks in a tab
> > +            case "cmd_save":
> > +                let tabType = document.getElementById("tabmail").currentTabInfo.mode.type;
> > +                return tabType == "calendarTask" || tabType == "calendarEvent";
> 
> You'll need to add brackets if you use variables inside a case block, or you
> will get the following eslint error:
> 
>   235:13  error  Unexpected lexical declaration in case block. 
> no-case-declarations (eslint)

Okay, will do, thanks.

> ::: calendar/lightning/content/lightning-item-panel.xul
> @@ +186,5 @@
> >                 modifiers="accel"
> >                 key="&event.dialog.close.key;"
> >                 command="cmd_item_close"/>
> > +          <key id="save-key-2"
> > +               modifiers="accel, shift"
> 
> Not quite sure why you need to change the id here? Given the id is not the
> entity, no need to change for localization reasons.

Indeed, this is not needed for localization reasons. My thinking was just to differentiate this save key from the other one that is used for the dialog window, to make it easier for developers to tell them apart.  But a better name would make that clearer.  

Maybe the other one should be something like "save-key-dialog" and this one should just be "save-key"?  Or maybe it's simplest to leave them both as "save-key"?

> ::: calendar/lightning/content/lightning-menus.xul
> @@ +7,5 @@
> >    <!ENTITY % lightningDTD SYSTEM "chrome://lightning/locale/lightning.dtd"> %lightningDTD;
> >    <!ENTITY % calendarDTD SYSTEM "chrome://calendar/locale/calendar.dtd" > %calendarDTD;
> >    <!ENTITY % toolbarDTD SYSTEM "chrome://lightning/locale/lightning-toolbar.dtd" > %toolbarDTD;
> >    <!ENTITY % menuOverlayDTD SYSTEM "chrome://calendar/locale/menuOverlay.dtd" > %menuOverlayDTD;
> > +  <!-- calendar-event-dialog.dtd is used for Save menu item (ltnSave) -->
> 
> No need for a comment here (which I am also not sure is legal inside a
> doctype definition)

Okay, will remove it.
 
> ::: calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
> @@ +71,5 @@
> >  <!ENTITY  event.menu.item.close.label                     "Close">
> >  <!ENTITY  event.menu.item.close.accesskey                 "C">
> >  <!ENTITY  event.menu.item.save.label                      "Save">
> >  <!ENTITY  event.menu.item.save.accesskey                  "S">
> > +<!ENTITY  event.menu.item.save.two.accesskey              "a">
> 
> This either needs a more descriptive name, or you should add a
> LOCALIZATION_NOTE for the two save accesskey entities.

Okay, I'll go with "file.menu.item.save.accesskey" for the new "a" one.  Let me know if there's a better one to use.

Comment 11

[Paul Morris [:pmorris]]

(In reply to [:MakeMyDay] from comment #8)
> Comment on attachment 8816182 [details] [diff] [review]
> 1-add-save-button.patch
> 
> Review of attachment 8816182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: calendar/lightning/content/lightning-item-toolbar.xul
> @@ +22,5 @@
> > +                   mode="dialog"
> > +                   class="cal-event-toolbarbutton toolbarbutton-1"
> > +                   label="&event.toolbar.save.label;"
> > +                   tooltiptext="&event.toolbar.save.tooltip;"
> > +                   command="cmd_save"/>
> 
> You are recycling the previously string names here, but now they have a
> different meaning. E.g. the existing label is Save and Close and you now use
> it just for Save.  That said, please count up the string names here
> event.toolbar.save.label2 and event.toolbar.save.tooltip2, otherwise this
> may result in a localization problem.

Ah, good point.  I'll add the '2' to both.

Comment 12

[Paul Morris [:pmorris]]

Attachment: 1-add-save-button-2.patch

Addresses comments on previous version.

Comment 13

[Paul Morris [:pmorris]]

Attachment: 2-add-save-menu-item-2.patch

Addresses comments on previous version.

Comment 14

[Paul Morris [:pmorris]]

Attachment: 2-add-save-menu-item-3.patch

Adds a LOCALIZATION NOTE to better address comment on previous version.

Comment 15

[Paul Morris [:pmorris]]

Attachment: 2-add-save-menu-item-4.patch

Better LOCALIZATION NOTE and better string name.

Comment 16

[Paul Morris [:pmorris]]

Attachment: 2-add-save-menu-item-5.patch

Fix problem with previous upload of this patch (ugh).

Comment 17

[[:MakeMyDay]]

I've just tested this. In genral it does what it should, but there are 3 findings:

1) In tab mode, the save icon is present by default in the toolbar. Was that orginally intended? Similar to what we have in window mode, Save and Close only would be better as default (especially since you still can open the same item multiple time, which may end up in write conflicts when editing in different tabs)

Apart from that, we still need a follow-up bug to make the icon for Save and Close different from Save

2) In tab mode there is a menu item and a shortcut for save & close missing - (Ctrl +) L for the key and an appropriate accesskeys would be suitable

3) If switching in tab mode from an event tab to the main tab, the save menu item in the file menu does not get disabled

Apart from that, it's a little bit unfortunate that we need to use different shortcuts in tab and window mode. We should try to harmonize the keys and accesskey use for standard operations all accross Thunderbird. But this is a more general topic and not to be resolved in this bug.

Can you at least fix #1 and #2 still today, so I can land the patches?

Comment 18

[Paul Morris [:pmorris]]

(In reply to [:MakeMyDay] from comment #17)
> I've just tested this. In genral it does what it should, but there are 3
> findings:
> 
> 1) In tab mode, the save icon is present by default in the toolbar. Was that
> orginally intended? 

I added it because I thought it would be a useful default but I'll take it out again.

> Apart from that, we still need a follow-up bug to make the icon for Save and
> Close different from Save

Yes.

> 2) In tab mode there is a menu item and a shortcut for save & close missing
> - (Ctrl +) L for the key and an appropriate accesskeys would be suitable

Should I add "Delete..." as well (with no shortcut) while I'm at it, for parity with window mode?

> Can you at least fix #1 and #2 still today, so I can land the patches?

Yes, will do.

Comment 19

[Paul Morris [:pmorris]]

Attachment: 3-add-save-and-close-menu-item.patch

Adds "Save and Close" menu item and removes "Save" button from default toolbar set for tab mode.

Comment 20

[[:MakeMyDay]]

Comment on attachment 8816797 [details] [diff] [review]
3-add-save-and-close-menu-item.patch

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

Thanks. r+ with the below comment considered.

::: calendar/base/content/calendar-common-sets.js
@@ +239,5 @@
>              }
> +            case "cmd_accept": {
> +                let tabType = document.getElementById("tabmail").currentTabInfo.mode.type;
> +                return tabType == "calendarTask" || tabType == "calendarEvent";
> +            }

To avoid the code duplication here please use

case "cmd_save":
// falls through
case "cmd_accept": {
...
}

The comment is required to satisfy the linter.

Comment 21

[Paul Morris [:pmorris]]

Attachment: 3-add-save-and-close-menu-item-2.patch

Addresses comment on previous version.

Comment 22

[[:MakeMyDay]]

https://hg.mozilla.org/comm-central/rev/2eb8ce0c171d8283ef63cee630f5c8845b9f6b1a
https://hg.mozilla.org/comm-central/rev/bdaf90347f93a51aa79df1a89037b69afeaa6979
https://hg.mozilla.org/comm-central/rev/0f80a3625f9fbc1e5f2d1fcc5323412685ec5432
https://hg.mozilla.org/comm-central/rev/9290eb29cde94f5912fef14d0fa67c445c4f522c

Comment 23

[[:MakeMyDay]]

Comment on attachment 8816186 [details] [diff] [review]
0-rename-save-to-saveandclose-2.patch

a+ from Philipp on irc

Comment 24

[[:MakeMyDay]]

Comment on attachment 8816690 [details] [diff] [review]
1-add-save-button-2.patch

a+ from Philipp on irc

Comment 25

[[:MakeMyDay]]

Comment on attachment 8816697 [details] [diff] [review]
2-add-save-menu-item-5.patch

a+ from Philipp on irc

Comment 26

[[:MakeMyDay]]

Comment on attachment 8816798 [details] [diff] [review]
3-add-save-and-close-menu-item-2.patch

a+ from Philipp on irc

Comment 27

[Jorg K (CEST = GMT+2)]

Aurora (Thunderbird 52 / Lightning 5.4)
https://hg.mozilla.org/releases/comm-aurora/rev/a1fa1bfcd232
https://hg.mozilla.org/releases/comm-aurora/rev/d22d5c769896
https://hg.mozilla.org/releases/comm-aurora/rev/3b0997e37f03
https://hg.mozilla.org/releases/comm-aurora/rev/0dc604fb8e3e