214548 - add possibility to copy (clone/duplicate) whole filter

Status: RESOLVED FIXED

(Thunderbird :: Filters, enhancement)

Description

[Seth Master]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030727
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030727

Request for Enhancement: 
in the message filters property box, we should have a box under delete, but
above help called copy rule. This activation box whould copy the rule that the
focus is on, and append the name of the original filter rule with copy of, then
you could choose edit on the new rule and change it to what you need.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.




This whould be a good enhancement, I think it whould be very useful for people
that could have a very big filter rules that need an almost identical rule for a
different filter

Comment 1

[Stefan Hoelldampf]

*** This bug has been confirmed by popular vote. ***

Comment 2

[Ray Booysen]

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

Comment 3

[Ray Booysen]

Moving to Core:  Enhancement affects TB too.

Comment 4

[Jean-Michel Reghem]

I really need this enhancement ...
I have more than 15 pop account, each with a inbox ...
I filter on some account some mails, sorted in several directories in Local Folders.
So, i have to duplicate some rules from a pop to an other.

It seems not so complicated as it is possible by hand. Currently, i open 2 msgFilterRules.dat from 2 pop account, and i copy wanted rules from one to other ... but it is not very clean and can cause errors ...

Comment 5

[Jean-Michel Reghem]

It could be related with bug 166842 if this RFE is well implemented ...
importing/exporting bug between 2 applications should also be able to import/export somes rules between 2 pop account

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to Seth Master from comment #0)
> I think it whould be very useful for people
yes

> that could have a very big filter rules that need an almost identical rule for a different filter

or even not a big list. 2-3 rules is all it takes to make it a pain to duplicate, especially if you want to clone it multiple times

Comment 12

[:aceman]

What is this report really about?
From the comments it looks like this wants to copy whole FILTERS inside account or even between accounts. But the summary is about copying RULES which are only the lines in the rules part of one filter (in contrast to actions).

Comment 13

[matteo sisti sette]

It's about duplicating filters.

It looks like it was written a long time ago against a different version of Thunderbird where the dialogs looked different (there's no "help" button at all below "delete").

Requests about duplicating filters such as 630046 have been marked as duplicate of this one.

Comment 14

[:aceman]

Axel, maybe you would be able to do this (after you land your other patch in the dialog).

Comment 15

[Axel Grude [:realRaven]]

Yes I'd like to do that. The only question is, where to stick the UI? The dialog is already chock-a-block full of UI elements when [Bug 450302] has landed.

The only thing I can think of is a context menu item (copy / paste) but that is kind of oblique. Obviously to have copy /cut/ paste across servers would be most beneficial. I would deliberately exclude any duplicate checking as it will make the whole thing a lot more complicated.

Comment 16

[Axel Grude [:realRaven]]

Attachment: A UI suggestion for Copy/Paste/Cut and other functions

I believe I have addressed all items previously flagged by Phil Chee; the change set includes a unified moveFilter function to streamline the code - I added an enum msgMoveMotion for this as i did not want to expand the XPCIM interface one (Ci.nsMsgFilterMotion)

Comment 17

[Axel Grude [:realRaven]]

Attachment: A UI suggestion for Copy/Paste/Cut and other functions

Sorry somehow I got the wrong thing in the last attachment due to Lazarus, AJAX or my own stupidity. I think if we add functionality like copy/cut/paste (and maybe later import / export) going the way of the Fx bookmarks / history dialogs might provide a nicer UI experience. Adding more buttons on the right hand side feels like a cul-de-sac to me. Opinions?

Comment 18

[:aceman]

Comment on attachment 622779 [details]
A UI suggestion for Copy/Paste/Cut and other functions

Maybe you could just enlarge the dialog to make space for a new Copy button.

The mockup is nice but it would need better specification of what operations are behind those new menuitems. But it is probably overkill for this bug report. Maybe you should file a new bug describing all the functionality you'd like to add. Let bwinton decide.

Comment 19

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

Comment on attachment 622779 [details]
A UI suggestion for Copy/Paste/Cut and other functions

So, it's really hard for me to give a reasonable review to this until I see what sort of things you plan on putting in each of the options.  But I will say this, since they're all drop-downs, why not just make it into a normal menubar?

Thanks,
Blake.

Comment 20

[Axel Grude [:realRaven]]

I would say for now, I will code copy / cut / paste as context menus into my quickFilters extension, so I have something to play with. One question is whether it should be intelligent enough to "merge" filters, should they have the same action (move to folder) or maybe that could be a separate feature...

Comment 21

[Thomas D. (:thomas8)]

Is this dupe of bug 229493 or perhaps vice versa?

Comment 22

[Axel Grude [:realRaven]]

(In reply to Axel Grude [:realRaven] from comment #20)
> I would say for now, I will code copy / cut / paste as context menus into my
> quickFilters extension, so I have something to play with. One question is
> whether it should be intelligent enough to "merge" filters, should they have
> the same action (move to folder) or maybe that could be a separate feature...

BY the way I have coded that functionalit in quickFilters 1.9

https://addons.mozilla.org/en-US/thunderbird/addon/quickfilters/

which is currently awaiting AMO review. Feel free to nick what you need (I can't write a patch as I cannot compile Thunderbird, its just too much hassle to make it work here)

Comment 23

[Axel Grude [:realRaven]]

(In reply to Thomas D. from comment #21)
> Is this dupe of bug 229493 or perhaps vice versa?

I have improved the functionality in quickFilters 2.5 - you can now copy / cut / paste within the same server so you can use it for re-ordering your filters as well as moving them across accounts.

Comment 24

[adamruka85]

Attachment: patch adding a 'Copy to New' filter option

Hello everyone,

sorry to resurrect this bug, as it's pretty old, but it seems to fit what I'm trying to do.

I've always wanted Thunderbird to have the capability to quickly and easily create a new filter by copying/cloning an existing one. Recently I decided to stop just dreaming, and try to implement this functionality. The attached patch does just that.

Note that this is my first attempt at submitting code to Thunderbird, so I would appreciate any feedback. For sure it lacks internationalization support, but I figured I would get some feedback first, and then polish it if you guys consider there's a chance of getting it merged.

Let me know what you think of this proposal.

Thanks,
Adam Ruka

Comment 27

[:aceman]

Comment on attachment 8801539 [details] [diff] [review]
patch adding a 'Copy to New' filter option

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

Thanks for the patch. The functionality seems to work nicely.
There are just some small problems that need fixing.

::: mail/base/content/FilterListDialog.js
@@ +415,5 @@
>      updateViewPosition(-1);
>    }
>  }
>  
> +function onCopyToNewFilter()

This seems to be mostly a copy of the code in onNewFilter(). Can you just use that function and make the differing code path (maybe just the setting of args.copiedFilter) conditional on an optional argument to it? Like onNewFilter(aCopySelected = false).

@@ +787,5 @@
>      // "delete" only disabled when no filters are selected
>      gDeleteButton.disabled = !numFiltersSelected;
>  
> +    // "copy to new" is the same as "edit"
> +    gCopyToNewButton.disabled = disabled;

Move this to the gEditButton above.

::: mail/base/content/FilterListDialog.xul
@@ +97,5 @@
>                    label="&deleteButton.label;"
>                    accesskey="&deleteButton.accesskey;"
>                    oncommand="onDeleteFilter();"/>
> +          <button id="copyToNewButton"
> +                  label="Copy to New"

You need to put this string into FilterListDialog.dtd file and reference the entity here.

@@ +98,5 @@
>                    accesskey="&deleteButton.accesskey;"
>                    oncommand="onDeleteFilter();"/>
> +          <button id="copyToNewButton"
> +                  label="Copy to New"
> +                  oncommand="onCopyToNewFilter();"/>

I'm not sure we want a whole new button in the filter list dialog. Could you merge this into the "new" button as a submenu, using type="menu-button"? "new" would be the default action, "copy" would be a possibility if the user pops up the submenu. We need UI review for this patch. Paenglab, what do you think of the current patch or my proposal?

::: mailnews/base/search/content/FilterEditor.js
@@ +129,5 @@
> +        let newFilter = gFilterList.createFilter(copiedName);
> +
> +        // copy the actions
> +        for (let i = 0; i < copiedFilter.actionCount; i++)
> +        {

In new code please use opening bracket on the end of the line, not start of new one.

@@ +148,5 @@
> +          newTerm.value = searchTerm.value;
> +          newFilter.appendTerm(newTerm);
> +        };
> +
> +        gPreFillName = copiedName;

For some reason, it is possible to create filters with duplicate name using this functionality (copy the same filter twice). That is not possible using normal "New" button, so this needs to be prevented here. There is a check to prevent duplicate names (in saveFilter()), but somehow it does not work here.

Comment 28

[Richard Marti (:Paenglab)]

Comment on attachment 8801539 [details] [diff] [review]
patch adding a 'Copy to New' filter option

Yes, a button type="menu-button" would be better.
Also is now a hardcoded "(copy)" added to the filter title. A localizable "Copy of " should be used. This would be user-friendlier because it's readable what this filter is.

Comment 29

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #28)
> Also is now a hardcoded "(copy)" added to the filter title. A localizable
> "Copy of " should be used. This would be user-friendlier because it's
> readable what this filter is.

You need to put this into filter.properties e.g. as "copyName=Copy of $S" and then use it via gFilterBundle.getFormattedString("copyName", [oldname]);

Comment 30

[adamruka85]

Attachment: patch adding a 'Copy to New' filter option - version 2

(In reply to :aceman from comment #27)
> Comment on attachment 8801539 [details] [diff] [review]
> patch adding a 'Copy to New' filter option
> 
> Review of attachment 8801539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/FilterListDialog.js
> @@ +415,5 @@
> >      updateViewPosition(-1);
> >    }
> >  }
> >  
> > +function onCopyToNewFilter()
> 
> This seems to be mostly a copy of the code in onNewFilter(). Can you just
> use that function and make the differing code path (maybe just the setting
> of args.copiedFilter) conditional on an optional argument to it? Like
> onNewFilter(aCopySelected = false).

onCopyToNewFilter() has actually parts from both onEditFilter() and onNewFilter(). I've extracted the common parts between onCopyToNewFilter and onNewFilter to a new function, calculatePositionAndShowCreateFilterDialog(). Let me know if that works.

> @@ +787,5 @@
> >      // "delete" only disabled when no filters are selected
> >      gDeleteButton.disabled = !numFiltersSelected;
> >  
> > +    // "copy to new" is the same as "edit"
> > +    gCopyToNewButton.disabled = disabled;
> 
> Move this to the gEditButton above.

Done.

> ::: mail/base/content/FilterListDialog.xul
> @@ +97,5 @@
> >                    label="&deleteButton.label;"
> >                    accesskey="&deleteButton.accesskey;"
> >                    oncommand="onDeleteFilter();"/>
> > +          <button id="copyToNewButton"
> > +                  label="Copy to New"
> 
> You need to put this string into FilterListDialog.dtd file and reference the
> entity here.

Done.

> @@ +98,5 @@
> >                    accesskey="&deleteButton.accesskey;"
> >                    oncommand="onDeleteFilter();"/>
> > +          <button id="copyToNewButton"
> > +                  label="Copy to New"
> > +                  oncommand="onCopyToNewFilter();"/>
> 
> I'm not sure we want a whole new button in the filter list dialog. Could you
> merge this into the "new" button as a submenu, using type="menu-button"?
> "new" would be the default action, "copy" would be a possibility if the user
> pops up the submenu. We need UI review for this patch. Paenglab, what do you
> think of the current patch or my proposal?

(In reply to Richard Marti (:Paenglab) from comment #28)
> Comment on attachment 8801539 [details] [diff] [review]
> patch adding a 'Copy to New' filter option
> 
> Yes, a button type="menu-button" would be better.

I've changed it to a menu-button and an expandable menu with 2 entries, 'New' (same as pressing the 'New...' button directly) and 'Copy selected'.

Let me know if that works.

> ::: mailnews/base/search/content/FilterEditor.js
> @@ +129,5 @@
> > +        let newFilter = gFilterList.createFilter(copiedName);
> > +
> > +        // copy the actions
> > +        for (let i = 0; i < copiedFilter.actionCount; i++)
> > +        {
> 
> In new code please use opening bracket on the end of the line, not start of
> new one.

Hmmm. I've actually just followed the local style in the file (same as in FilterListDialog.js). I think it's better to have the code consistent. If you feel strongly about it, I can of course change it (I actually prefer the brace on the same line).

> @@ +148,5 @@
> > +          newTerm.value = searchTerm.value;
> > +          newFilter.appendTerm(newTerm);
> > +        };
> > +
> > +        gPreFillName = copiedName;
> 
> For some reason, it is possible to create filters with duplicate name using
> this functionality (copy the same filter twice). That is not possible using
> normal "New" button, so this needs to be prevented here. There is a check to
> prevent duplicate names (in saveFilter()), but somehow it does not work here.

Yes, you were right. As it turns out, when the value in the 'name' input field is the same as the gFilter name, the uniqueness check is skipped (I assume to handle the 'Edit' case).

I changed the code to reset the gFilter name after the call to initializeDialog(), and added a comment explaining why it's needed.

I've tested it, and I'm no longer able to create a filter with a duplicate name. Let me know if your testing confirms that it's fixed also.

(In reply to :aceman from comment #29)
> (In reply to Richard Marti (:Paenglab) from comment #28)
> > Also is now a hardcoded "(copy)" added to the filter title. A localizable
> > "Copy of " should be used. This would be user-friendlier because it's
> > readable what this filter is.
> 
> You need to put this into filter.properties e.g. as "copyName=Copy of $S"
> and then use it via gFilterBundle.getFormattedString("copyName", [oldname]);

Done (I used the formatting string "Copy of %S", as "Copy of $S" didn't work).

Comment 31

[Richard Marti (:Paenglab)]

Comment on attachment 8805864 [details] [diff] [review]
patch adding a 'Copy to New' filter option - version 2

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

This is better than before. At least on Win10 the button is wider than the others. But this isn't your fault and can be fixed in a followup bug.

::: mail/locales/en-US/chrome/messenger/FilterListDialog.dtd
@@ +7,5 @@
>  <!ENTITY activeColumn.label "Enabled">
>  <!ENTITY newButton.label "New…">
>  <!ENTITY newButton.accesskey "N">
> +<!ENTITY newButton.dropdown.label "New">
> +<!ENTITY copyToNewButton.label "Copy selected">

First could you use a more consistent entity naming like newButton.popupNew.label and newButton.popupCopy.label/accesskey?
Also more descriptive labels would be better like "New Filter" and "Copy selected Filter".

Comment 32

[adamruka85]

Attachment: filters-dialog-screenshot.png

Comment 33

[adamruka85]

Attachment: patch adding a 'Copy to New' filter option - version 3

Hey Richard,

thanks for the comments!

(In reply to Richard Marti (:Paenglab) from comment #31)
> Comment on attachment 8805864 [details] [diff] [review]
> patch adding a 'Copy to New' filter option - version 2
> 
> Review of attachment 8805864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is better than before. At least on Win10 the button is wider than the
> others. But this isn't your fault and can be fixed in a followup bug.

Yeah, it looks fine for me - see attached screenshot (I'm on Ubuntu Xenial). No idea why it looks off on Win10.

> ::: mail/locales/en-US/chrome/messenger/FilterListDialog.dtd
> @@ +7,5 @@
> >  <!ENTITY activeColumn.label "Enabled">
> >  <!ENTITY newButton.label "New…">
> >  <!ENTITY newButton.accesskey "N">
> > +<!ENTITY newButton.dropdown.label "New">
> > +<!ENTITY copyToNewButton.label "Copy selected">
> 
> First could you use a more consistent entity naming like
> newButton.popupNew.label and newButton.popupCopy.label/accesskey?

Done.

> Also more descriptive labels would be better like "New Filter" and "Copy
> selected Filter".

Hmm, I'm actually not sure about this one, for 2 reasons:

1. It's inconsistent with the rest of the filters dialog. For example, the button for editing only says 'Edit'; it doesn't say 'Edit Filter'.
2. It makes the expandable menu too big. For example, on my local Thunderbird, the menu with the longer labels extends very far away outside the dialog border, which makes it look weird.

However, if you feel strongly about it, I can change the labels.

Comment 34

[:aceman]

Comment on attachment 8806141 [details] [diff] [review]
patch adding a 'Copy to New' filter option - version 3

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

Great, this works now. I give review+ but please fix the documentation nits.

::: mail/base/content/FilterListDialog.js
@@ +357,5 @@
> +
> +  calculatePositionAndShowCreateFilterDialog(args);
> +}
> +
> +function calculatePositionAndShowCreateFilterDialog(args)

Thanks, the common code looks nice now. Please document all the new functions (including onNewFilter). See the comment block before selectFilter() on how it is formatted.

::: mail/locales/en-US/chrome/messenger/FilterListDialog.dtd
@@ +7,5 @@
>  <!ENTITY activeColumn.label "Enabled">
>  <!ENTITY newButton.label "New…">
>  <!ENTITY newButton.accesskey "N">
> +<!ENTITY newButton.popupNew.label "New">
> +<!ENTITY newButton.popupCopy.label "Copy selected">

Can we add 3 dots (ellipsis on these 2 items too) ? They open a subdialog too. In that case you could use newButton.label for the main button and the menuitem too, and drop newButton.popupNew.label.

::: mail/locales/en-US/chrome/messenger/filter.properties
@@ +23,5 @@
>  stopButtonLabel=Stop
>  continueButtonLabel=Continue
>  cannotEnableFilter=This filter was probably created by future version of mozilla/netscape. You cannot enable this filter because we don't know how to apply it.
>  dontWarnAboutDeleteCheckbox=Don't ask me again
> +copyToNewFilterName=Copy of %S

Please add a localization note on what %S represents. See the next lines in this file on how such a note is formatted.

::: mailnews/base/search/content/FilterEditor.js
@@ +154,5 @@
> +        gFilter = newFilter;
> +
> +        initializeDialog(gFilter);
> +
> +        /*

Please use // style comments inside function code. Also you can make the lines longer (up to 80 columns) so this could fit into 2 lines.

@@ +159,5 @@
> +         * We reset the filter name, because otherwise the
> +         * saveFilter() function thinks we are editing a filter,
> +         * and will thus skip the name uniqueness check.
> +         */
> +        gFilter.filterName = '';

Please use double quotes for strings.

Comment 35

[adamruka85]

Attachment: patch adding a 'Copy to New' filter option - version 4

(In reply to :aceman from comment #34)
> Comment on attachment 8806141 [details] [diff] [review]
> patch adding a 'Copy to New' filter option - version 3
> 
> Review of attachment 8806141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, this works now. I give review+ but please fix the documentation nits.

Thanks!

> ::: mail/base/content/FilterListDialog.js
> @@ +357,5 @@
> > +
> > +  calculatePositionAndShowCreateFilterDialog(args);
> > +}
> > +
> > +function calculatePositionAndShowCreateFilterDialog(args)
> 
> Thanks, the common code looks nice now. Please document all the new
> functions (including onNewFilter). See the comment block before
> selectFilter() on how it is formatted.

Done.
 
> ::: mail/locales/en-US/chrome/messenger/FilterListDialog.dtd
> @@ +7,5 @@
> >  <!ENTITY activeColumn.label "Enabled">
> >  <!ENTITY newButton.label "New…">
> >  <!ENTITY newButton.accesskey "N">
> > +<!ENTITY newButton.popupNew.label "New">
> > +<!ENTITY newButton.popupCopy.label "Copy selected">
> 
> Can we add 3 dots (ellipsis on these 2 items too) ? They open a subdialog
> too. In that case you could use newButton.label for the main button and the
> menuitem too, and drop newButton.popupNew.label.

Done. Also changed 'Copy selected' to 'Copy...', to be consistent with the naming.
 
> ::: mail/locales/en-US/chrome/messenger/filter.properties
> @@ +23,5 @@
> >  stopButtonLabel=Stop
> >  continueButtonLabel=Continue
> >  cannotEnableFilter=This filter was probably created by future version of mozilla/netscape. You cannot enable this filter because we don't know how to apply it.
> >  dontWarnAboutDeleteCheckbox=Don't ask me again
> > +copyToNewFilterName=Copy of %S
> 
> Please add a localization note on what %S represents. See the next lines in
> this file on how such a note is formatted.

Done.

> ::: mailnews/base/search/content/FilterEditor.js
> @@ +154,5 @@
> > +        gFilter = newFilter;
> > +
> > +        initializeDialog(gFilter);
> > +
> > +        /*
> 
> Please use // style comments inside function code. Also you can make the
> lines longer (up to 80 columns) so this could fit into 2 lines.

Done (the comments extend a little further than 80 columns, as sticking to that limit would require them to span 3 lines instead of 2, and some lines in the file are already longer than that).
 
> @@ +159,5 @@
> > +         * We reset the filter name, because otherwise the
> > +         * saveFilter() function thinks we are editing a filter,
> > +         * and will thus skip the name uniqueness check.
> > +         */
> > +        gFilter.filterName = '';
> 
> Please use double quotes for strings.

Done.

-------------------------------------------------------------

Assuming the code is now OK what is the exact procedure of getting it pushed? Sorry if it's obvious, but like I said, this is my first contribution to Thunderbird.

Thanks,
Adam

Comment 36

[:aceman]

Comment on attachment 8806533 [details] [diff] [review]
patch adding a 'Copy to New' filter option - version 4

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

::: mail/base/content/FilterListDialog.js
@@ +344,1 @@
>  function onNewFilter(emailAddress)

Yeah, it seems emailAddress argument is not used and nothing passes it in. Please remove it.

@@ +360,5 @@
> +    return;
> +
> +  let args = {
> +    copiedFilter: selectedFilter
> +  };

You can have this on a single line.

@@ +372,5 @@
> + *
> + * @param args  The object containing the arguments for the dialog,
> + *              passed to the filterEditorOnLoad() function.
> + *              It will be augmented with the insertion position
> + *              and global filters list properties by this function.

Nice thanks.

::: mail/base/content/FilterListDialog.xul
@@ +95,5 @@
> +              <menuitem label="&newButton.label;"
> +                        accesskey="&newButton.accesskey;"/>
> +              <menuitem id="copyToNewButton"
> +                        label="&newButton.popupCopy.label;"
> +                        accesskey="&newButton.popupCopy.accesskey;"

Forgot to use newButton.* here.

::: mail/locales/en-US/chrome/messenger/filter.properties
@@ +25,5 @@
>  cannotEnableFilter=This filter was probably created by future version of mozilla/netscape. You cannot enable this filter because we don't know how to apply it.
>  dontWarnAboutDeleteCheckbox=Don't ask me again
> +# LOCALIZATION NOTE(copyToNewFilterName)
> +# %1$S=the name of the filter that is being copied
> +copyToNewFilterName=Copy of %1$S

Only use %S when there is a single argument.

::: mailnews/base/search/content/FilterEditor.js
@@ +155,5 @@
> +
> +        initializeDialog(gFilter);
> +
> +        // We reset the filter name, because otherwise the saveFilter() function thinks
> +        // we are editing a filter, and will thus skip the name uniqueness check.

Please keep the lines below 80 chars. The other lines in the file that are longer need to be fixed int he future.

Comment 37

[:aceman]

(In reply to adamruka85 from comment #35)
> Assuming the code is now OK what is the exact procedure of getting it
> pushed? Sorry if it's obvious, but like I said, this is my first
> contribution to Thunderbird.

I see this is your first patch and it is very nice. Thanks for submitting it.

After the patch is done and you have all the reviews, you upload the final patch with a modified commit message ("Bug 214548 - Implemented 'Copy to New' filter functionality."), where you tack on the reviewers. E.g. "Bug 214548 - Implemented 'Copy to New' filter functionality. r=aceman". Then put "checkin-needed" into the "keywords" field in this bug.

Comment 38

[adamruka85]

Attachment: patch adding a 'Copy to New' filter option - version 5

(In reply to :aceman from comment #36)
> Comment on attachment 8806533 [details] [diff] [review]
> patch adding a 'Copy to New' filter option - version 4
> 
> Review of attachment 8806533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/FilterListDialog.js
> @@ +344,1 @@
> >  function onNewFilter(emailAddress)
> 
> Yeah, it seems emailAddress argument is not used and nothing passes it in.
> Please remove it.

Done.

> @@ +360,5 @@
> > +    return;
> > +
> > +  let args = {
> > +    copiedFilter: selectedFilter
> > +  };
> 
> You can have this on a single line.

Done.
 
> @@ +372,5 @@
> > + *
> > + * @param args  The object containing the arguments for the dialog,
> > + *              passed to the filterEditorOnLoad() function.
> > + *              It will be augmented with the insertion position
> > + *              and global filters list properties by this function.
> 
> Nice thanks.

Thanks!
 
> ::: mail/base/content/FilterListDialog.xul
> @@ +95,5 @@
> > +              <menuitem label="&newButton.label;"
> > +                        accesskey="&newButton.accesskey;"/>
> > +              <menuitem id="copyToNewButton"
> > +                        label="&newButton.popupCopy.label;"
> > +                        accesskey="&newButton.popupCopy.accesskey;"
> 
> Forgot to use newButton.* here.

Sorry, I don't follow. What is it that needs to be done here?

> ::: mail/locales/en-US/chrome/messenger/filter.properties
> @@ +25,5 @@
> >  cannotEnableFilter=This filter was probably created by future version of mozilla/netscape. You cannot enable this filter because we don't know how to apply it.
> >  dontWarnAboutDeleteCheckbox=Don't ask me again
> > +# LOCALIZATION NOTE(copyToNewFilterName)
> > +# %1$S=the name of the filter that is being copied
> > +copyToNewFilterName=Copy of %1$S
> 
> Only use %S when there is a single argument.

Done.
 
> ::: mailnews/base/search/content/FilterEditor.js
> @@ +155,5 @@
> > +
> > +        initializeDialog(gFilter);
> > +
> > +        // We reset the filter name, because otherwise the saveFilter() function thinks
> > +        // we are editing a filter, and will thus skip the name uniqueness check.
> 
> Please keep the lines below 80 chars. The other lines in the file that are
> longer need to be fixed int he future.

Done.

Comment 39

[:aceman]

(In reply to adamruka85 from comment #38)
> > ::: mail/base/content/FilterListDialog.xul
> > @@ +95,5 @@
> > > +              <menuitem label="&newButton.label;"
> > > +                        accesskey="&newButton.accesskey;"/>
> > > +              <menuitem id="copyToNewButton"
> > > +                        label="&newButton.popupCopy.label;"
> > > +                        accesskey="&newButton.popupCopy.accesskey;"
> > 
> > Forgot to use newButton.* here.
> 
> Sorry, I don't follow. What is it that needs to be done here?

Seems I saw wrong, all is done here.

Comment 40

[adamruka85]

Attachment: patch adding a 'Copy to New' filter option - final version

(In reply to :aceman from comment #39)
> (In reply to adamruka85 from comment #38)
> > > ::: mail/base/content/FilterListDialog.xul
> > > @@ +95,5 @@
> > > > +              <menuitem label="&newButton.label;"
> > > > +                        accesskey="&newButton.accesskey;"/>
> > > > +              <menuitem id="copyToNewButton"
> > > > +                        label="&newButton.popupCopy.label;"
> > > > +                        accesskey="&newButton.popupCopy.accesskey;"
> > > 
> > > Forgot to use newButton.* here.
> > 
> > Sorry, I don't follow. What is it that needs to be done here?
> 
> Seems I saw wrong, all is done here.

Great.

Comment 41

[adamruka85]

(In reply to :aceman from comment #37)
> (In reply to adamruka85 from comment #35)
> > Assuming the code is now OK what is the exact procedure of getting it
> > pushed? Sorry if it's obvious, but like I said, this is my first
> > contribution to Thunderbird.
> 
> I see this is your first patch and it is very nice. Thanks for submitting it.
> 
> After the patch is done and you have all the reviews, you upload the final
> patch with a modified commit message ("Bug 214548 - Implemented 'Copy to
> New' filter functionality."), where you tack on the reviewers. E.g. "Bug
> 214548 - Implemented 'Copy to New' filter functionality. r=aceman". Then put
> "checkin-needed" into the "keywords" field in this bug.

Awesome. I' submitted the final vesion of the patch,

Comment 42

[:aceman]

Checked in:
https://hg.mozilla.org/comm-central/rev/ee260c01780d479660b5c901a3a92f6b7c43758f

All is done here, thanks for the nice patch.

The main code is done in /mailnews, but is wired up in the UI only in TB. Ian, I CC you in case you want to mark this bug somehow for porting to SM.

Comment 43

[Jorg K (CEST = GMT+2)]

I tried this in Daily. The new button is wider than the others which is rather ugly. That was already observed in comment #31. Why does it need to be so complicated? Why not just have two buttons "New" and "Copy". There is enough space on the panel. And the user will see the new "Copy" button straight away.

Comment 44

[:aceman]

We do not want to overdo it with too many buttons (see the attached picture). Also I think copy is not such an often used operation to need a full button. The others are much more used.
Anyway, I expect Paenglab to fix the button sizes in a new bug later (but on Linux they are fine).
We need to get features and strings baking on trunk now.

Comment 45

[Jorg K (CEST = GMT+2)]

I'm not sure which attached picture you're referring to, there isn't one with a "Copy" button.

Also, I think a sub-menu with the same item as the top button is bad design: New > New. We don't have anything like this anywhere.

Comment 46

[Jorg K (CEST = GMT+2)]

I think it should just be:
        <vbox>
          <button id="newButton"
                  label="&newButton.label;"
                  accesskey="&newButton.accesskey;"
                  oncommand="onNewFilter();"/>
          <button id="copyButton"
                  label="&newButton.popupCopy.label;"
                  accesskey="&newButton.popupCopy.accesskey;"
                  oncommand="onCopyToNewFilter();"/>
And you'd have to fix the string names
newButton.popupCopy.label --> copyButton.label
newButton.popupCopy.accesskey --> copyButton.accesskey

Richard, what do you think? Do you like the menu button with the same entry twice? Where do we have anything similar?

Comment 47

[:aceman]

I mean attachment 622779 [details].

Yes, I thought about the duplicate menuitem. We do have such cases, e.g. in the "Get messages" toolbar button. The menu does contain the same action as if you just pressed the button.

I'm more concerned that the popup is narrowed (in width) than the main button. That seems to be a bug in XUL, there is no value equivalent to 'sizetopopup' to make it at least the size of the parent button.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: fix-copy-filter.patch

I think it should be like this. The patch applies to what was landed already.

"copy to new" also doesn't make much sense. Surely a copy is new.

Comment 49

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+1) from comment #43)
> I tried this in Daily. The new button is wider than the others which is
> rather ugly. That was already observed in comment #31. Why does it need to
> be so complicated? Why not just have two buttons "New" and "Copy". There is
> enough space on the panel. And the user will see the new "Copy" button
> straight away.

Aceman already explained this.

(In reply to Jorg K (GMT+1) from comment #45)
> I'm not sure which attached picture you're referring to, there isn't one
> with a "Copy" button.
> 
> Also, I think a sub-menu with the same item as the top button is bad design:
> New > New. We don't have anything like this anywhere.

This is normal behavior for splitbuttons. The first menu entry is the default action when you press the button. An example is also the combined reply toolbarbutton where the default action is the first entry.

Comment 50

[Jorg K (CEST = GMT+2)]

Comment on attachment 8807899 [details] [diff] [review]
fix-copy-filter.patch

> An example is also the combined reply toolbarbutton ...
Ah well, looks like I can't win here.

Comment 51

[Axel Grude [:realRaven]]

Any suggestions whether I should disable the cut / copy / paste buttons on my addons quickFilters when this lands? I think mine does a little more in that it supports choosing an insert point and pulling together multiple filters if non-contiguous ones are selected - also across servers. Or should I remove this button and leave mine in there?