Issue 7914

Title
Trim char/text field
Priority
feature
Status
in-progress
Superseder
Product with newline in name is lost (issue 9248)
Nosy list
Timitos, ced, lordvan, nicoe, pokoli, reviewbot, yangoon
Assigned to
nicoe
Keywords
review

Created on 2018-12-10.13:31:35 by ced, last changed yesterday by reviewbot.

Messages

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-04-02.19:29:14

Please assign the issue to yourself and set correct status, see https://www.tryton.org/develop#report-issue

Author: [hidden] (nicoe) Tryton committer
Date: 2021-04-02.19:17:55

Here is the review adding the strip option.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2020-10-24.17:26:23

This is partially solved by issue9248. But I still sting it is good to trim trailing spaces for most Char fields on client side.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2020-10-24.17:08:10

I think you're confusing the value and the way the value is displayed.

But as the trim will happen at the UI, for me it is the same behavior.

Yet I don't think it's a good idea because it breaks the principle of least surprise

It will not be a surprise if it happens on the UI so the user will see it (but not sure because we remove invisible chars).

Author: [hidden] (nicoe) Tryton committer
Date: 2018-12-12.10:12:31
* Cédric Krier [2018-12-11 18:26:17]:
> 
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
> 
> Indeed we already do such transformation on some fields. E.g. we
> remove the non-significant zero (and trim space) on numeric; we
> transform text into int for Many2One, we recompute sequence field.

I think you're confusing the value and the way the value is displayed.
In case of char data they're both the same, while it's not the case
when the data is a relation, a number or an order relationship.

> In many char/text fields, trailing spaces have no signification for
> the user.

Usually it's the case but sometimes it might not be it.

> I would not activate on Party.name because it is too much special
> but I think it should on almost all other names,

If party.name is an exception then trim should default to True for
fields.Char. Yet I don't think it's a good idea because it breaks the
principle of least surprise: users and developers expect that the data
they filled will be saved as-is.

> codes, numbers and description.

I agree that it can be useful for those fields though.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2018-12-11.18:26:17
Indeed we already do such transformation on some fields. E.g. we remove the non-significant zero (and trim space) on numeric; we transform text into int for Many2One, we recompute sequence field.
In many char/text fields, trailing spaces have no signification for the user.
I would not activate on Party.name because it is too much special but I think it should on almost all other names, codes, numbers and description.
Author: [hidden] (yangoon) Tryton translator
Date: 2018-12-11.13:58:22
I absolutely agree with nicoe (msg45232).
Author: [hidden] (nicoe) Tryton committer
Date: 2018-12-10.16:38:34
It's a bit strange to consider that you can fiddle with user input but I can understand that for codes or other identifiers it makes sense to trim the data (according to me those fields shouldn't usually be editable in the interface anyway).

Making it an opt-in feature is OK but we should use it parsimoniously.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2018-12-10.15:57:44
In sao line breaks are also not shown so I agree that it will be better if we can remove it. Using str.strip sounds like the more reasonable solution for this case. 

I agree that it should be enabled for name's but I'm wondering if it  makes sense to activate it also for codes (i.e: Product code, account code, etc) and also for description (i.e: product description, sale line description, invoice line description).
Author: [hidden] (Timitos) Tryton translator
Date: 2018-12-10.15:50:13
+1
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2018-12-10.15:47:16
If we use str.strip, it will remove any line breaks at the end or beginning but not inside. I think it is a good behavior.
Author: [hidden] (Timitos) Tryton translator
Date: 2018-12-10.15:40:12
They are not visible when the client is used on windows for example. So if you use tryton in a environment with client on different os, for example a combination of linux and windows the line breaks are only visible on the linux machines but not on the windows machines.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2018-12-10.15:30:05
On 2018-12-10 13:45, Korbinian Preisler wrote:
> There is another point that could be considered with this. We sometimes have to deal with line separators at the end of the string, especially when pasting from ms excel or libreoffice calc. Could this feature be extended to deal with things like that too?

Those line breaks are visible in GTK entry (I do not know for HTML
input)
So if the user can see and correct them, I do not think we should
overrule their usage.
Author: [hidden] (Timitos) Tryton translator
Date: 2018-12-10.13:45:57
There is another point that could be considered with this. We sometimes have to deal with line separators at the end of the string, especially when pasting from ms excel or libreoffice calc. Could this feature be extended to deal with things like that too?
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2018-12-10.13:31:35
I saw this: https://github.com/odoo/odoo/commit/fb6c9c24434c941383ffd73ab3056cafd7740c52

I think it is not a bad idea to have such behavior as I saw many times trailing spaces on names (like product, party etc.).
I think it will be wrong to manage it only on client side (like Odoo) but it should also be enforced on server side. Because it will be inconsistent is trailing spaces are added via some RPC calls and changed on the fly by the client.
Also I think it should be an opt-in feature not an opt-out.
For text it should trim each line.
History
Date User Action Args
2021-06-15 19:06:35reviewbotsetmessages: + msg68275
2021-06-10 13:00:44reviewbotsetmessages: + msg68184
2021-05-19 15:35:10lordvansetnosy: + lordvan
2021-05-11 19:42:04reviewbotsetmessages: + msg67520
2021-05-11 18:21:51nicoesetassignedto: nicoe
2021-05-11 18:21:42nicoesetstatus: chatting -> in-progress
2021-04-02 19:29:14cedsetmessages: + msg66008
2021-04-02 19:21:39reviewbotsetmessages: + msg66007
nosy: + reviewbot
2021-04-02 19:17:55nicoesetkeyword: + review
messages: + msg66006
reviews: 340511008
2020-10-24 17:26:23cedsetmessages: + msg61364
superseder: + Product with newline in name is lost

Showing 10 items. Show all history (warning: this could be VERY long)