[FIX] web_widget_x2many_2d_matrix: _renderBodyCell

[alexey-pelykh]

No description provided.

[alexey-pelykh]

@simahawk @yajo @pedrobaeza @tarteo please review this fix, and sorry for introducing an issue

[tarteo]

Looks good. For the options parameter in the function I would rather use _.defaults once instead of checking every time (also on line 286) it options is something.

[JordiBForgeFlow]

@bjeficent @mreficent can you review plz?

[hbrunn]

this is an example why open source is so super cool: I introduced this widget a while ago, others picked it up, extended, and now I'm at a state where I don't understand the diff without reading all of what people did during the v11 and v12 migration. Looks great! But I'm still reading, so can't put a review currently

[simahawk]

@alexey-pelykh LG. I'd like to see a version upgrade and a line in the changelog. Can you do that?

[simahawk]

@tarteo checking that format thingy and how it changed... on abstract field they do this:

https://github.com/odoo/odoo/blob/1868713dbd07e0b518f91dffe73e62d85e6ab9a6/addons/web/static/src/js/fields/abstract_field.js#L164-L173

        // formatType is used to determine which format (and parse) functions
        // to call to format the field's value to insert into the DOM (typically
        // put into a span or an input), and to parse the value from the input
        // to send it to the server. These functions are chosen according to
        // the 'widget' attrs if is is given, and if it is a valid key, with a
        // fallback on the field type, ensuring that the value is formatted and
        // displayed according to the choosen widget, if any.
        this.formatType = this.attrs.widget in field_utils.format ?
                            this.attrs.widget :
this.field.type;

so, yes, there could be also a specific formatting by widget if found in the registry,

However, in the list renderer is ignored and I think it's because you already try to render via widget (which is going to handle it using formatType) and it just fallbacks when no option is left to the mere field type.

Seems legit, no?

[alexey-pelykh]

@simahawk done, also please check:

• removed button-related code (is it even possible to have one in 2d matrix?)
• added few comments

[tarteo]

@simahawk Ah yes, thanks. @alexey-pelykh Can you add some comments in the code on what's happening.

[yajo]

Yikes, this is really hard to review. Please, could you add some details on what are you fixing and how, and remove all the clutter introduced with all that commented code, to keep a smaller diff, easier to review?

Please abstain from opening big PRs without description. Thanks!

[AaronHForgeFlow]

I will only say that this makes OCA/timesheet#140 to work with hr_timesheet_sheet. So It'd be great if all comments here are attended and we can move on with that.

[alexey-pelykh]

All changes made as of now

[yajo]

@tarteo can you please update your review?

[alexey-pelykh]

Significant note: for some reason, editable mode is not working.

options.mode = this.getParent().mode;

worth checking

[alexey-pelykh]

readonly vs edit are getting propagated properly, that's all I know right now

[alexey-pelykh]

/me dumb - now fields can be editable properly

[pedrobaeza]

Several questions about this (some might be out of the scope of this PR):

• Can you use a many2one for values of the matrix?
• Can you specify editable by cell possibilities through attrs?
• Is [11.0][web_widget_x2many_2d_matrix] cannot store fields values not included in the matrix #1084 fixed here?

[alexey-pelykh]

@pedrobaeza unfortunately I'm unable to address any of these questions properly, this PR is related to migration, so I don't think it would fix anything by magic. Yet, it may have introduced support for widgets (question 1.), and right now the editable state is propagated from parent (question 2.)

[pedrobaeza]

Well, maybe 3 might be introduced in your migration, so that's why I asked... @tarteo please confirm the review for merging this.

[tarteo]

Thanks!