[9.0][MIG] web_widget_x2many_2d_matrix: Migration and enhancement #427
Conversation
* README update to newest OCA template * Example in README * Massive performance boost for big matrices, specially on Firefox * Assign id on row in order to find it back in all cases * Fix OCA#321, choked on cached writes
Declare as many options prefixed with this string as you need for binding a field value with an HTML node attribute (disabled, class, style...) called as the `<name>` passed in the option. NOTE: This doesn't prevent to require to fill the full matrix with all the combination records.
…s_clickable attrs XML attributes for the widget that allows to configure if the axis will be clickable or not in case the source field is a many2one field.
|
cc @Tecnativa |
|
@hbrunn you'll be interested in this one too. |
There was a problem hiding this comment.
looks good! But the onchange-story bothers me a bit. I must admit I never tried this on 8, do we have the same problem there? For my feeling, if a widget needs help here, the widget is wrong. Can't we make this behave exactly as a standard one2many in this respect?
did you also consider having the widget generate empty records as necessary to get the full Cartesian product of the values available? Then using the widget would be a lot simpler.
I also experimented a bit with injecting fields here instead of using custom inputs, this turned out to be quite a hassle in 8. I think (didn't try yet) this should be a lot simpler in 9
[the last two are just ideas of course, not in any way blocking]
|
About the field injection and the Cartesian product, I have exhausted the development time (and also my spirit for trying something more in JS), so that can go better to roadmap section. It's of course very interesting. The problem with onchange seems something inherent to ORM. We have found the same problem on one2many fields and in onchange OCA/server-tools#492 (comment), so we need to see if we can patch it at one2many general widget or at ORM. |
|
@hbrunn I'm facing a problem resolving name of many2one fields when using a default value. Do you figure out why it's not resolved in that case? From onchange:
From default:
|
| @@ -50,10 +53,18 @@ openerp.web_widget_x2many_2d_matrix = function(instance) | |||
| this.field_y_axis = node.attrs.field_y_axis || this.field_y_axis; | |||
| this.field_label_x_axis = node.attrs.field_label_x_axis || this.field_x_axis; | |||
| this.field_label_y_axis = node.attrs.field_label_y_axis || this.field_y_axis; | |||
| this.x_axis_clickable = node.attrs.x_axis_clickable != undefined ? this.parse_boolean(node.attrs.x_axis_clickable) : this.x_axis_clickable; | |||
| this.y_axis_clickable = node.attrs.y_axis_clickable != undefined ? this.parse_boolean(node.attrs.y_axis_clickable) : this.y_axis_clickable; | |||
There was a problem hiding this comment.
To feed your love for JS a bit more... null != undefined is false.
Maybe you will want to use !==.
There was a problem hiding this comment.
how about this.parse_boolean(node.attrs.x_axis_clickable || (this.x_axis_clickable ? '1': '0'))?
There was a problem hiding this comment.
Well, in this case I think it can't be null. It exists or not, but not assigned to null. Am I wrong?
There was a problem hiding this comment.
@hbrunn what do we win with that expression? Take into account that this.x_axis_clickable has been declared a couple lines above, and its value is fixed to true
There was a problem hiding this comment.
then this.parse_boolean(node.attrs.x_axis_clickable || '1') would be simpler. The proposal is mainly about using javascript's notion of truthyness as we'd do in Python
There was a problem hiding this comment.
I'd be cautious about 'correctly', there are a lot of weirdnesses like empty arrays not being falsy and similar things. But for checking if something is set value || default works perfectly fine, and then you don't need to go into the details of undefined, null, false etc, as all of those are falsy
There was a problem hiding this comment.
But for this case I think it is. I'm making tests right now to check.
There was a problem hiding this comment.
Tried in all cases and it's working correctly, so I let Holger's version.
There was a problem hiding this comment.
Like in Python, "0" is true. Not sure if it applies here, otherwise ignore this 😁
There was a problem hiding this comment.
No, it doesn't apply here. We only want to have a default in case you don't specify this attribute in the XML. If the widget user is putting specifically "0" in the option, then it should be passed to parseBoolean function, which is what is done right now.
|
@pedrobaeza I'd put a breakpoint at https://github.com/OCA/web/pull/427/files/00ab29fbe49960abdd66c1f77b71923b9fd6961c#diff-d25cee8c92486a0e4cbf5ba21a360bbaR156 to check if the value there is something funny in the case of a default value. Probably you need an extra check for some other data type |
|
@yajo, can you make the final check and approve this? |
|
@hbrunn can we proceed with the merge? |
|
Thanks! |
Migration of the module to v9 with these features:
There's also this improvement:
x_axis_clickableandy_axis_clickable