Bugzilla

Thomas, thanks for looking at the patch but please, don't change the flags based on assumptions.
Reviews and tests have been done and, as you can read from the previous comments, all the "issues" you reported have been addressed and explained.

> First, when recipients are moved from CC to BCC, that's clearly a change of recipients, so *not* firing onRecipientsChanged violates the inherent notion of onRecipientsChanged and will cause even more confusion in the future for anyone consuming and relying on this as a pseudo-Event.

I'm preventing the firing of that method only if the pills where not removed by a user.
This might happen when changing identity, which might remove pills if changing from an identity that has pre-filled Cc/Bcc fields, to an identity that has nothing.

> Second (and worse), as you're skipping onRecipientsChanged, I understand that you're also skipping "gContentChanged = true;" which is contained in onRecipientsChanged. So the net result is - pls correct me if I'm wrong - that when I move 100 recipients from To to BCC (and that's the only change in this draft-editing session), Thunderbird will now discard that change without warning when closing composition window. If that's what happens, I find that not acceptable.

Did you load the patch and tried it?
Once again, that method is called if is the user that moves the pills.

> So here you're skipping `updateSendCommands(true);` which is inside `onRecipientsChanged(true);` for the case where identity is changed (not at compose startup). That looks wrong to me because changing an identity can remove CC/BCC/Reply-To and if these were the only recipients, we must make sure the send button is disabled when they are removed by identity change.

I'm not skipping `updateSendCommands(true);`. The tests cover that area and they all pass, meaning the original behaviour is maintained.
I'm calling the `onRecipientsChanged(true);` only if the fields have been changed when switching identity.
There's no need to call that method if loading identities doesn't fill/remove any pill.

Also, we've been always ignoring the `Reply-To` field when updating the sensitivity of the Send button, I didn't change that behaviour.

> Hmmm. Did we really change recipients here? If not, why are we firing the entire `onRecipientsChanged()` function? Or maybe we should just `updateSendCommands(true);` if that's what we want as the above comment suggests?

Why? If no pills was deleted in the process of hiding an addressing row, what's the point of updating the send command? No recipients changed.

> 
I think this is problematic. I think we need more changes to ultimately trigger `gContentChanged = true`. You're not running onRecipientsChanged() for changes of reply-to and several other things that can change with identity like vCard etc. So gContentChanged = true; inside onRecipientsChanged() does no longer occur for such changes, but should. Yes, some have their own dedicated global variables to check for changes (e.g. gAttachVCardOptionChanged), but those are cunning, because I found that the variables of change are *not* set true when the change occurs via identity change, so as to preserve them for *direct manual* changes only, which are part of the conditions used here. It's all very twisted and undocumented.

This is outside the scope of this bug, which has the purpose of not triggering the `onRecipientsChanged` when not necessary.

Thomas, thanks for looking at the patch but please, don't change the flags based on assumptions.
Reviews and tests have been done and, as you can read from the previous comments, all the "issues" you reported have been addressed and explained.

> First, when recipients are moved from CC to BCC, that's clearly a change of recipients, so *not* firing onRecipientsChanged violates the inherent notion of onRecipientsChanged and will cause even more confusion in the future for anyone consuming and relying on this as a pseudo-Event.

I'm preventing the firing of that method only if the pills where not removed by a user.
This might happen when changing identity, which might remove pills if changing from an identity that has pre-filled Cc/Bcc fields, to an identity that has nothing.
Also, I'm preventing the firing of the `onRecipientsChanged` twice if the delete is done during a move. As you know, moving a pill means: *copy > remove > add*, and since we trigger that method when adding a pill, we don't need to trigger it twice during the move process.
If you remove the pill, that's not a "move", so the method gets called.

> Second (and worse), as you're skipping onRecipientsChanged, I understand that you're also skipping "gContentChanged = true;" which is contained in onRecipientsChanged. So the net result is - pls correct me if I'm wrong - that when I move 100 recipients from To to BCC (and that's the only change in this draft-editing session), Thunderbird will now discard that change without warning when closing composition window. If that's what happens, I find that not acceptable.

Did you load the patch and tried it?
Once again, that method is called if is the user that moves the pills.

> So here you're skipping `updateSendCommands(true);` which is inside `onRecipientsChanged(true);` for the case where identity is changed (not at compose startup). That looks wrong to me because changing an identity can remove CC/BCC/Reply-To and if these were the only recipients, we must make sure the send button is disabled when they are removed by identity change.

I'm not skipping `updateSendCommands(true);`. The tests cover that area and they all pass, meaning the original behaviour is maintained.
I'm calling the `onRecipientsChanged(true);` only if the fields have been changed when switching identity.
There's no need to call that method if loading identities doesn't fill/remove any pill.

Also, we've been always ignoring the `Reply-To` field when updating the sensitivity of the Send button, I didn't change that behaviour.

> Hmmm. Did we really change recipients here? If not, why are we firing the entire `onRecipientsChanged()` function? Or maybe we should just `updateSendCommands(true);` if that's what we want as the above comment suggests?

Why? If no pills was deleted in the process of hiding an addressing row, what's the point of updating the send command? No recipients changed.

> 
I think this is problematic. I think we need more changes to ultimately trigger `gContentChanged = true`. You're not running onRecipientsChanged() for changes of reply-to and several other things that can change with identity like vCard etc. So gContentChanged = true; inside onRecipientsChanged() does no longer occur for such changes, but should. Yes, some have their own dedicated global variables to check for changes (e.g. gAttachVCardOptionChanged), but those are cunning, because I found that the variables of change are *not* set true when the change occurs via identity change, so as to preserve them for *direct manual* changes only, which are part of the conditions used here. It's all very twisted and undocumented.

This is outside the scope of this bug, which has the purpose of not triggering the `onRecipientsChanged` when not necessary.

Thomas, thanks for looking at the patch but please, don't change the flags based on assumptions.
Reviews and tests have been done and, as you can read from the previous comments, all the "issues" you reported have been addressed and explained.

> First, when recipients are moved from CC to BCC, that's clearly a change of recipients, so *not* firing onRecipientsChanged violates the inherent notion of onRecipientsChanged and will cause even more confusion in the future for anyone consuming and relying on this as a pseudo-Event.

I'm preventing the firing of that method only if the pills where not removed by a user.
This might happen when changing identity, which might remove pills if changing from an identity that has pre-filled Cc/Bcc fields, to an identity that has nothing.
Also, I'm preventing the firing of the `onRecipientsChanged` twice if the delete is done during a move. As you know, moving a pill means: *copy > remove > add*, and since we trigger that method when adding a pill, we don't need to trigger it twice during the move process.
If you remove the pill, that's not a "move", so the method gets called.

> Second (and worse), as you're skipping onRecipientsChanged, I understand that you're also skipping "gContentChanged = true;" which is contained in onRecipientsChanged. So the net result is - pls correct me if I'm wrong - that when I move 100 recipients from To to BCC (and that's the only change in this draft-editing session), Thunderbird will now discard that change without warning when closing composition window. If that's what happens, I find that not acceptable.

Did you load the patch and tried it?
Once again, that method is called if is the user that moves the pills.

> So here you're skipping `updateSendCommands(true);` which is inside `onRecipientsChanged(true);` for the case where identity is changed (not at compose startup). That looks wrong to me because changing an identity can remove CC/BCC/Reply-To and if these were the only recipients, we must make sure the send button is disabled when they are removed by identity change.

I'm not skipping `updateSendCommands(true);`. The tests cover that area and they all pass, meaning the original behaviour is maintained.
I'm calling the `onRecipientsChanged(true);` only if the fields have been changed when switching identity.
There's no need to call that method if loading identities doesn't fill/remove any pill.

Also, we've been always ignoring the `Reply-To` field when updating the sensitivity of the Send button, I didn't change that behaviour.

> Hmmm. Did we really change recipients here? If not, why are we firing the entire `onRecipientsChanged()` function? Or maybe we should just `updateSendCommands(true);` if that's what we want as the above comment suggests?

Why? If no pills was deleted in the process of hiding an addressing row, what's the point of updating the send command? No recipients changed.

> I think this is problematic. I think we need more changes to ultimately trigger `gContentChanged = true`. You're not running onRecipientsChanged() for changes of reply-to and several other things that can change with identity like vCard etc. So gContentChanged = true; inside onRecipientsChanged() does no longer occur for such changes, but should. Yes, some have their own dedicated global variables to check for changes (e.g. gAttachVCardOptionChanged), but those are cunning, because I found that the variables of change are *not* set true when the change occurs via identity change, so as to preserve them for *direct manual* changes only, which are part of the conditions used here. It's all very twisted and undocumented.

This is outside the scope of this bug, which has the purpose of not triggering the `onRecipientsChanged` when not necessary.

Thomas, thanks for looking at the patch but please, don't change the flags based on assumptions.
Reviews and tests have been done and, as you can read from the previous comments, all the "issues" you reported have been addressed and explained.

> First, when recipients are moved from CC to BCC, that's clearly a change of recipients, so *not* firing onRecipientsChanged violates the inherent notion of onRecipientsChanged and will cause even more confusion in the future for anyone consuming and relying on this as a pseudo-Event.

I'm preventing the firing of that method only if the pills where not removed by a user.
This might happen when changing identity, which might remove pills if changing from an identity that has pre-filled Cc/Bcc fields, to an identity that has nothing.
Also, I'm preventing the firing of the `onRecipientsChanged` twice if the delete is done during a move. As you know, moving a pill means: *copy > remove > add*, and since we trigger that method when adding a pill, we don't need to trigger it twice during the move process.
If you remove the pill, that's not a "move", so the method gets called.

> Second (and worse), as you're skipping onRecipientsChanged, I understand that you're also skipping "gContentChanged = true;" which is contained in onRecipientsChanged. So the net result is - pls correct me if I'm wrong - that when I move 100 recipients from To to BCC (and that's the only change in this draft-editing session), Thunderbird will now discard that change without warning when closing composition window. If that's what happens, I find that not acceptable.

Did you load the patch and tried it?
Once again, that method is called if is the user that moves the pills.

> So here you're skipping `updateSendCommands(true);` which is inside `onRecipientsChanged(true);` for the case where identity is changed (not at compose startup). That looks wrong to me because changing an identity can remove CC/BCC/Reply-To and if these were the only recipients, we must make sure the send button is disabled when they are removed by identity change.

I'm not skipping `updateSendCommands(true);`. The tests cover that area and they all pass, meaning the original behaviour is maintained.
I'm calling the `onRecipientsChanged(true);` only if the fields have been changed when switching identity.
There's no need to call that method if loading identities doesn't fill/remove any pill.

Also, we've been always ignoring the `Reply-To` field when updating the sensitivity of the Send button, I didn't change that behaviour.

> Hmmm. Did we really change recipients here? If not, why are we firing the entire `onRecipientsChanged()` function? Or maybe we should just `updateSendCommands(true);` if that's what we want as the above comment suggests?

Why? If no pill was deleted in the process of hiding an addressing row, what's the point of updating the send command? No recipients changed.

> I think this is problematic. I think we need more changes to ultimately trigger `gContentChanged = true`. You're not running onRecipientsChanged() for changes of reply-to and several other things that can change with identity like vCard etc. So gContentChanged = true; inside onRecipientsChanged() does no longer occur for such changes, but should. Yes, some have their own dedicated global variables to check for changes (e.g. gAttachVCardOptionChanged), but those are cunning, because I found that the variables of change are *not* set true when the change occurs via identity change, so as to preserve them for *direct manual* changes only, which are part of the conditions used here. It's all very twisted and undocumented.

This is outside the scope of this bug, which has the purpose of not triggering the `onRecipientsChanged` when not necessary.