M2M field definition wrongly sent 'add_remove' data which breaks domain inversion

The M2M's definition method has probably been copied from O2M and it kept the add_remove handling of its origin.

As a consequence the domain inversion is now broken because the domain to invert is wrongly twice the same domain and it prevents to set the value of fields.

assigned to @nicoe

added trytond type::behavior + 1 deleted label

New review397301002 at https://codereview.tryton.org/397301002/#ps368121002

I do not agree. This was added on purpose #4112 (closed). What is the problem with domain inversion(this is probably that that must be fixed).

* Cédric Krier [2022-02-12 02:27 +0100]:
>
>Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
>I do not agree. This was added on purpose #4112 (closed).

OK I see it now, still I don't get what a M2M displayed as a O2M has to gain
to define add_remove as this is basically what the standard widget for M2M
already does. Maybe being used in form view? But it's quite marginal.

>What is the problem with domain inversion(this is probably that that must be
>fixed).

The domain used for the domain inversion is [domain, add_remove_domain]. Thus
for M2M it's twice the same domain. Inverting it results in [domain, domain]
which won't trigger the setting mechanism.

As we don't know on the field level if it's a M2M or a O2M we can not reliably
not add the domain from add_remove. Indeed we could check that both domains are
equals before adding them, but it looks to me like a more fragile solution as
I don't see the use case for add_remove in M2M.

On 2022-02-12 11:13, Nicolas Évrard wrote:
> * Cédric Krier [2022-02-12 02:27 +0100]:
> >I do not agree. This was added on purpose #4112 (closed).
>
> OK I see it now, still I don't get what a M2M displayed as a O2M has to gain
> to define add_remove as this is basically what the standard widget for M2M
> already does. Maybe being used in form view? But it's quite marginal.

The goal is to be able to use a Many2Many with the One2Many widget not
only to create new instances but also to add existing targets.

> >What is the problem with domain inversion(this is probably that that must be
> >fixed).
>
> The domain used for the domain inversion is [domain, add_remove_domain]. Thus
> for M2M it's twice the same domain. Inverting it results in [domain, domain]
> which won't trigger the setting mechanism.

OK I understand.

> As we don't know on the field level if it's a M2M or a O2M we can not reliably
> not add the domain from add_remove. Indeed we could check that both domains are
> equals before adding them, but it looks to me like a more fragile solution as

For me this sounds like a good solution. I do not see why it will be
fragile. There is not point to test twice the same part of the domain.

review397301002 updated at https://codereview.tryton.org/397301002/#ps376951002

I think it will be better to make the domain inversion automatically ignore clause that are duplicates in an AND-ed expression. I think it will be a more generic solution that will also solve any other similar issue which can happen when modules extending domain the same way or complex PYSON expression with similar result.

review397301002 updated at https://codereview.tryton.org/397301002/#ps386201002

review397301002 updated at https://codereview.tryton.org/397301002/#ps366861005

review397301002 updated at https://codereview.tryton.org/397301002/#ps397511002

review397301002 updated at https://codereview.tryton.org/397301002/#ps393831002

review397301002 updated at https://codereview.tryton.org/397301002/#ps382111002

review397301002 updated at https://codereview.tryton.org/397301002/#ps380491002

review397301002 updated at https://codereview.tryton.org/397301002/#ps390221002

review397301002 updated at https://codereview.tryton.org/397301002/#ps382131030

review397301002 updated at https://codereview.tryton.org/397301002/#ps397581002

review397301002 updated at https://codereview.tryton.org/397301002/#ps384671003

review397301002 updated at https://codereview.tryton.org/397301002/#ps386281002

New changeset 5f35f461245c by Nicolas Évrard in branch 'default':
Deduplicate entries in domain inversion's simplify
https://hg.tryton.org/sao/rev/5f35f461245c