Issue 357911002: tryton-env: Get directly the string value of Selection fields in non editable tree views
Descriptiontryton-env: Get directly the string value of Selection fields in non editable tree views
issue10331
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fix remarks #
Total comments: 8
Patch Set 3 : Fix remarks #Patch Set 4 : Set related value #
Total comments: 37
Patch Set 5 : Fix remarks #Patch Set 6 : Add CHANGELOG #
Total comments: 9
Patch Set 7 : Update to tip #
Total comments: 34
Patch Set 8 : Fix remarks #
Total comments: 7
MessagesTotal messages: 28
flake8 OK URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1032: if self.view._editable: For me it should use the public attribute. https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1033: self.init_selection() Not calling init_selection make that some attributes are missing like help etc. I think we can afford to still call it in order to prevent future side effect. https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1045: if (self.attrs['name'] + '.') not in record.value: I think the test should also be based on the view editable property. By the way I think it is clearer to set as first branch, the shortcut. https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:918: selections[name] = selection = dict(f_selection) I think it is bad that this all code could not be using the TranslatedSelection. I think it should be possible to add a classmethod for shared code. https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:921: 'value': '', The value should be the real value. https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:922: 'string': '', The string could be different. https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:928: } I'm not sure it is correct to use a dict and the trailing dot for this because it creates confusion with the nested dot notation. Also it duplicate the value. What about 'selection:string' ?
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1032: if self.view._editable: On 2021/04/25 22:28:27, ced wrote: > For me it should use the public attribute. Done. https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1033: self.init_selection() On 2021/04/25 22:28:27, ced wrote: > Not calling init_selection make that some attributes are missing like help etc. > I think we can afford to still call it in order to prevent future side effect. Done. https://codereview.tryton.org/357911002/diff/355311002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1045: if (self.attrs['name'] + '.') not in record.value: On 2021/04/25 22:28:27, ced wrote: > I think the test should also be based on the view editable property. > > By the way I think it is clearer to set as first branch, the shortcut. Done. https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:918: selections[name] = selection = dict(f_selection) On 2021/04/25 22:28:27, ced wrote: > I think it is bad that this all code could not be using the TranslatedSelection. > I think it should be possible to add a classmethod for shared code. Done. https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:922: 'string': '', On 2021/04/25 22:28:27, ced wrote: > The string could be different. Done. https://codereview.tryton.org/357911002/diff/355311002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:928: } On 2021/04/25 22:28:27, ced wrote: > I'm not sure it is correct to use a dict and the trailing dot for this because > it creates confusion with the nested dot notation. Also it duplicate the value. > What about 'selection:string' ? Done.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js#n... sao/src/view/tree.js:2285: if (this.tree && this.tree.editable) { I think if tree is not set we should initialize the selection. https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js#n... sao/src/view/tree.js:2286: Sao.common.selection_mixin.init.call(this); I think this init should always be called. In case init_selection is called later. https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js#n... sao/src/view/tree.js:2298: if (record._values[this.field.name + '.']) { The value is this.field.name + ':string' https://codereview.tryton.org/357911002/diff/336101002/trytond/trytond/tests/... File trytond/trytond/tests/test_field_selection.py (right): https://codereview.tryton.org/357911002/diff/336101002/trytond/trytond/tests/... trytond/trytond/tests/test_field_selection.py:241: [arabic.id, null.id], ['select.string']) For me we must read 'select:string'
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js#n... sao/src/view/tree.js:2285: if (this.tree && this.tree.editable) { On 2021/05/16 17:05:32, ced wrote: > I think if tree is not set we should initialize the selection. Done. https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js#n... sao/src/view/tree.js:2286: Sao.common.selection_mixin.init.call(this); On 2021/05/16 17:05:32, ced wrote: > I think this init should always be called. In case init_selection is called > later. Done. https://codereview.tryton.org/357911002/diff/336101002/sao/src/view/tree.js#n... sao/src/view/tree.js:2298: if (record._values[this.field.name + '.']) { On 2021/05/16 17:05:32, ced wrote: > The value is this.field.name + ':string' Done. https://codereview.tryton.org/357911002/diff/336101002/trytond/trytond/tests/... File trytond/trytond/tests/test_field_selection.py (right): https://codereview.tryton.org/357911002/diff/336101002/trytond/trytond/tests/... trytond/trytond/tests/test_field_selection.py:241: [arabic.id, null.id], ['select.string']) On 2021/05/16 17:05:32, ced wrote: > For me we must read 'select:string' Done.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
Missing changelog entry https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js File sao/src/model.js (right): https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js#newco... sao/src/model.js:723: ((fdescription.loading || 'eager') == 'eager')) { Should it not be just 'loading == 'eager' https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js#newco... sao/src/model.js:724: fnames_to_fetch.push(fname + '.string'); it is :string https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js#newco... sao/src/model.js:891: (this.model.fields[name] instanceof Sao.field.Selection)) { 80 cols https://codereview.tryton.org/357911002/diff/344371002/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/344371002/sao/src/view/tree.js#n... sao/src/view/tree.js:2298: if (record._values[this.field.name + '.']) { it is ':' https://codereview.tryton.org/357911002/diff/344371002/sao/src/view/tree.js#n... sao/src/view/tree.js:2302: this.update_selection(record, function() { I do not think the update_selection will call init_selection if it was not yet called. I think we must add such check in update_selection in order to make the loading strategy independent of the widget. https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/model/record.py (right): https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/model/record.py:73: for fname in fnames[:]: it is probably more readable to use list(fnames) https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/model/record.py:78: and f_attrs.get('loading', 'eager') == 'eager'): idem https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1032: self.init_selection() I think it should not be called if not editable https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1049: self.update_selection(record, field) idem about calling init_selection https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... File trytond/trytond/model/fields/selection.py (right): https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/fields/selection.py:76: if not isinstance(selection, (tuple, list)): Is it not more flexible to test if selection is a string because it is the only possibility to be an attribute. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:711: fields_related[field_name] I think it is good to add ':string' https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:904: elif field._type == 'selection': I would change the key here as: for target in targets: key = name + target https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:906: selection = fields.Selection.get_selection( Why not calling: field.get_selection https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:907: cls, name, cls(row['id'])) It is a little bit annoying that we call multiple times get_selection even if it is a fixed selection or a classmethod. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:913: value = switch_value This part could maybe be shared with TranslatedSelection as a function on Selection like get_selection. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:969: targets = {} I think it should contain list(fields_related[fname])
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js File sao/src/model.js (right): https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js#newco... sao/src/model.js:723: ((fdescription.loading || 'eager') == 'eager')) { On 2021/08/08 16:31:26, ced wrote: > Should it not be just 'loading == 'eager' 'loading' is set for name here we loop over fnames and get their loading status. In case 'name' loading status is lazy then we loop over all the fields also some for which their loading is not 'eager'. In case 'name' loading status is 'eager' then we loop only over eagerly loaded fields. https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js#newco... sao/src/model.js:724: fnames_to_fetch.push(fname + '.string'); On 2021/08/08 16:31:26, ced wrote: > it is :string Done. https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js#newco... sao/src/model.js:891: (this.model.fields[name] instanceof Sao.field.Selection)) { On 2021/08/08 16:31:26, ced wrote: > 80 cols Done. https://codereview.tryton.org/357911002/diff/344371002/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/344371002/sao/src/view/tree.js#n... sao/src/view/tree.js:2298: if (record._values[this.field.name + '.']) { On 2021/08/08 16:31:27, ced wrote: > it is ':' Done. https://codereview.tryton.org/357911002/diff/344371002/sao/src/view/tree.js#n... sao/src/view/tree.js:2302: this.update_selection(record, function() { On 2021/08/08 16:31:26, ced wrote: > I do not think the update_selection will call init_selection if it was not yet > called. > I think we must add such check in update_selection in order to make the loading > strategy independent of the widget. Done. https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/model/record.py (right): https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/model/record.py:73: for fname in fnames[:]: On 2021/08/08 16:31:27, ced wrote: > it is probably more readable to use list(fnames) Funny I have the impression [:] conveys better the fact that it's a copy then list(). Done anyway. https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/model/record.py:78: and f_attrs.get('loading', 'eager') == 'eager'): On 2021/08/08 16:31:27, ced wrote: > idem Done. https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1032: self.init_selection() On 2021/08/08 16:31:27, ced wrote: > I think it should not be called if not editable Done. https://codereview.tryton.org/357911002/diff/344371002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1049: self.update_selection(record, field) On 2021/08/08 16:31:27, ced wrote: > idem about calling init_selection Done. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... File trytond/trytond/model/fields/selection.py (right): https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/fields/selection.py:76: if not isinstance(selection, (tuple, list)): On 2021/08/08 16:31:27, ced wrote: > Is it not more flexible to test if selection is a string because it is the only > possibility to be an attribute. Done. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:711: fields_related[field_name] On 2021/08/08 16:31:27, ced wrote: > I think it is good to add ':string' It's not possible because we loop over the names in fields_related and get the fields with the same name. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:904: elif field._type == 'selection': On 2021/08/08 16:31:27, ced wrote: > I would change the key here as: > > for target in targets: > key = name + target I don't understand. OK I do now, I don't see the point as add_related is called for every field in fields_related. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:906: selection = fields.Selection.get_selection( On 2021/08/08 16:31:27, ced wrote: > Why not calling: field.get_selection Done. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:907: cls, name, cls(row['id'])) On 2021/08/08 16:31:27, ced wrote: > It is a little bit annoying that we call multiple times get_selection even if it > is a fixed selection or a classmethod. Indeed but selection could be instance dependent. It shouldn't be too costly in the static or classmethod case, should it? https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:913: value = switch_value On 2021/08/08 16:31:27, ced wrote: > This part could maybe be shared with TranslatedSelection as a function on > Selection like get_selection. Done. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:969: targets = {} On 2021/08/08 16:31:27, ced wrote: > I think it should contain list(fields_related[fname]) See remark above.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js File sao/src/model.js (right): https://codereview.tryton.org/357911002/diff/344371002/sao/src/model.js#newco... sao/src/model.js:723: ((fdescription.loading || 'eager') == 'eager')) { On 2021/09/02 15:35:18, nicoe wrote: > On 2021/08/08 16:31:26, ced wrote: > > Should it not be just 'loading == 'eager' > > 'loading' is set for name here we loop over fnames and get their loading status. > In case 'name' loading status is lazy then we loop over all the fields also some > for which their loading is not 'eager'. In case 'name' loading status is 'eager' > then we loop only over eagerly loaded fields. What are you calling "'name' loading status"? For me this optimization should only be done when loading == 'eager' which means it is a list otherwise it is a form and we do not need it because selection widget will any way fetch the selections. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:711: fields_related[field_name] On 2021/09/02 15:35:19, nicoe wrote: > On 2021/08/08 16:31:27, ced wrote: > > I think it is good to add ':string' > > It's not possible because we loop over the names in fields_related and get the > fields with the same name. Acknowledged. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:904: elif field._type == 'selection': On 2021/09/02 15:35:19, nicoe wrote: > On 2021/08/08 16:31:27, ced wrote: > > I would change the key here as: > > > > for target in targets: > > key = name + target > > I don't understand. > > OK I do now, I don't see the point as add_related is called for every field in > fields_related. Indeed I think the problem is to reuse add_related for something that is not a relation. I think we must have a method only for selection with a proper API. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:907: cls, name, cls(row['id'])) On 2021/09/02 15:35:19, nicoe wrote: > On 2021/08/08 16:31:27, ced wrote: > > It is a little bit annoying that we call multiple times get_selection even if > it > > is a fixed selection or a classmethod. > > Indeed but selection could be instance dependent. > It shouldn't be too costly in the static or classmethod case, should it? If there is a lot of rows, yes it will be. I think we must has a test to call only for each row if it is an instance method. Or better `get_selection` takes a list of instances so it can optimize the case of classmethod. https://codereview.tryton.org/357911002/diff/344371002/trytond/trytond/model/... trytond/trytond/model/modelsql.py:969: targets = {} On 2021/09/02 15:35:19, nicoe wrote: > On 2021/08/08 16:31:27, ced wrote: > > I think it should contain list(fields_related[fname]) > > See remark above. Indeed I think it is better to not reused add_related for selection, it will be cleaner.
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/353871002/sao/CHANGELOG File sao/CHANGELOG (right): https://codereview.tryton.org/357911002/diff/353871002/sao/CHANGELOG#newcode1 sao/CHANGELOG:1: * Get directly the string value of Selection fields in non editable tree views I think it is more in the general style: Use string value of Selection for non editable tree view https://codereview.tryton.org/357911002/diff/353871002/sao/src/model.js File sao/src/model.js (right): https://codereview.tryton.org/357911002/diff/353871002/sao/src/model.js#newco... sao/src/model.js:891: (this.model.fields[name] instanceof Sao.field.Selection) select must use ':string' https://codereview.tryton.org/357911002/diff/353871002/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/353871002/sao/src/view/tree.js#n... sao/src/view/tree.js:2285: if ((this.tree && this.tree.editable) || !this.tree) { Why is it testing !this.tree? In tryton there is no such test. https://codereview.tryton.org/357911002/diff/353871002/sao/src/view/tree.js#n... sao/src/view/tree.js:2300: if (record._values[this.field.name + '.']) { I do not understand this test. For me it should test if "this.field.name + ':string'" exists. https://codereview.tryton.org/357911002/diff/353871002/sao/src/view/tree.js#n... sao/src/view/tree.js:2300: if (record._values[this.field.name + '.']) { I think it should also test if it is not editable like in tryton. https://codereview.tryton.org/357911002/diff/353871002/sao/src/view/tree.js#n... sao/src/view/tree.js:2301: var text_value = record._values[this.field.name + ':string'].string; There is no '.string' https://codereview.tryton.org/357911002/diff/353871002/tryton/CHANGELOG File tryton/CHANGELOG (right): https://codereview.tryton.org/357911002/diff/353871002/tryton/CHANGELOG#newcode1 tryton/CHANGELOG:1: * Get directly the string value of Selection fields in non editable tree views idem https://codereview.tryton.org/357911002/diff/353871002/tryton/tryton/common/s... File tryton/tryton/common/selection.py (right): https://codereview.tryton.org/357911002/diff/353871002/tryton/tryton/common/s... tryton/tryton/common/selection.py:52: if not self.selection: I think it should test if it is not None because an empty list could be valid. https://codereview.tryton.org/357911002/diff/353871002/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/353871002/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1045: if not self.view.editable and record.value.get(related): I would test if value.get(related) is not None.
Sign in to reply to this message.
patch is not applicable URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/443391003/sao/CHANGELOG File sao/CHANGELOG (right): https://codereview.tryton.org/357911002/diff/443391003/sao/CHANGELOG#newcode1 sao/CHANGELOG:1: * Get directly the string value of Selection fields in non editable tree views I think we could simplify: * Eagerly read string value of selection field https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js File sao/src/model.js (right): https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:674: if ((fdescription.type == 'selection') && could be else if like in tryton https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:838: (this.model.fields[name] instanceof Sao.field.Reference) || 80 cols https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:839: (this.model.fields[name] instanceof Sao.field.Selection) Maybe be good to set a field variable instead of retrieve 3 times. https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:841: var related = name + '.'; For Selection it should be name + ':string' https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:842: this._values[related] = values[related] || {}; It should not be {} for Selection. https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js#n... sao/src/view/tree.js:2508: if ((this.tree && this.tree.editable) || !this.tree) { Why not: if (!this.tree || this.tree.editable) { https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js#n... sao/src/view/tree.js:2523: if (record._values[this.field.name + '.']) { I do not understand the test. https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js#n... sao/src/view/tree.js:2523: if (record._values[this.field.name + '.']) { Should it not test that the tree is not editable otherwise we can not know if the read value is still the same value as the selected. https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js#n... sao/src/view/tree.js:2524: var text_value = record._values[this.field.name + ':string'].string; 80 cols https://codereview.tryton.org/357911002/diff/443391003/tryton/CHANGELOG File tryton/CHANGELOG (right): https://codereview.tryton.org/357911002/diff/443391003/tryton/CHANGELOG#newcode1 tryton/CHANGELOG:1: * Get directly the string value of Selection fields in non editable tree views idem https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/model/record.py (right): https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/model/record.py:480: self.value[related] = val.get(related) idem could set a variable field. https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1037: if self.view.editable: Should not also test if self.view is set? https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... File trytond/trytond/model/fields/selection.py (right): https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... trytond/trytond/model/fields/selection.py:86: def get_selection_value(selection, value): should it not be named: get_selection_string ? https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... trytond/trytond/model/modelsql.py:976: elif field._type == 'selection': Maybe we should support also multiselection for completeness and return a list of string. https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... trytond/trytond/model/modelsql.py:980: cls, name, cls(row['id'])) I'm a little worry to create an instance in the read but also to not use a browse with all the ids. As this happen after the getting the Function fields which also create instances, I guess the instances are not really a problem (especially since we have cache on readonly transaction). For the browse maybe we could manage to have targets parameter being a browse of the ids for the selection. https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/tests/... File trytond/trytond/tests/test_field_selection.py (right): https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/tests/... trytond/trytond/tests/test_field_selection.py:249: Selection = Pool().get('test.selection') I think we must enforce the style of separate instantiation of Pool for new tests.
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/443391003/sao/CHANGELOG File sao/CHANGELOG (right): https://codereview.tryton.org/357911002/diff/443391003/sao/CHANGELOG#newcode1 sao/CHANGELOG:1: * Get directly the string value of Selection fields in non editable tree views On 2022/07/18 21:07:23, ced wrote: > I think we could simplify: > > * Eagerly read string value of selection field Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js File sao/src/model.js (right): https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:674: if ((fdescription.type == 'selection') && On 2022/07/18 21:07:23, ced wrote: > could be else if like in tryton Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:838: (this.model.fields[name] instanceof Sao.field.Reference) || On 2022/07/18 21:07:23, ced wrote: > 80 cols Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:839: (this.model.fields[name] instanceof Sao.field.Selection) On 2022/07/18 21:07:23, ced wrote: > Maybe be good to set a field variable instead of retrieve 3 times. Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:841: var related = name + '.'; On 2022/07/18 21:07:23, ced wrote: > For Selection it should be name + ':string' Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/model.js#newco... sao/src/model.js:842: this._values[related] = values[related] || {}; On 2022/07/18 21:07:23, ced wrote: > It should not be {} for Selection. Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js#n... sao/src/view/tree.js:2508: if ((this.tree && this.tree.editable) || !this.tree) { On 2022/07/18 21:07:23, ced wrote: > Why not: > > if (!this.tree || this.tree.editable) { Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js#n... sao/src/view/tree.js:2523: if (record._values[this.field.name + '.']) { On 2022/07/18 21:07:23, ced wrote: > Should it not test that the tree is not editable otherwise we can not know if > the read value is still the same value as the selected. Done. https://codereview.tryton.org/357911002/diff/443391003/sao/src/view/tree.js#n... sao/src/view/tree.js:2524: var text_value = record._values[this.field.name + ':string'].string; On 2022/07/18 21:07:23, ced wrote: > 80 cols Done. https://codereview.tryton.org/357911002/diff/443391003/tryton/CHANGELOG File tryton/CHANGELOG (right): https://codereview.tryton.org/357911002/diff/443391003/tryton/CHANGELOG#newcode1 tryton/CHANGELOG:1: * Get directly the string value of Selection fields in non editable tree views On 2022/07/18 21:07:24, ced wrote: > idem Done. https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/model/record.py (right): https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/model/record.py:480: self.value[related] = val.get(related) On 2022/07/18 21:07:24, ced wrote: > idem could set a variable field. Done. https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/443391003/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1037: if self.view.editable: On 2022/07/18 21:07:24, ced wrote: > Should not also test if self.view is set? Done. https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... File trytond/trytond/model/fields/selection.py (right): https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... trytond/trytond/model/fields/selection.py:86: def get_selection_value(selection, value): On 2022/07/18 21:07:24, ced wrote: > should it not be named: get_selection_string ? Done. https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... trytond/trytond/model/modelsql.py:976: elif field._type == 'selection': On 2022/07/18 21:07:24, ced wrote: > Maybe we should support also multiselection for completeness and return a list > of string. Done. https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/model/... trytond/trytond/model/modelsql.py:980: cls, name, cls(row['id'])) On 2022/07/18 21:07:24, ced wrote: > I'm a little worry to create an instance in the read but also to not use a > browse with all the ids. > > As this happen after the getting the Function fields which also create > instances, I guess the instances are not really a problem (especially since we > have cache on readonly transaction). > > For the browse maybe we could manage to have targets parameter being a browse of > the ids for the selection. Done. https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/tests/... File trytond/trytond/tests/test_field_selection.py (right): https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/tests/... trytond/trytond/tests/test_field_selection.py:249: Selection = Pool().get('test.selection') On 2022/07/18 21:07:24, ced wrote: > I think we must enforce the style of separate instantiation of Pool for new > tests. What do you mean?
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/357911002
Sign in to reply to this message.
https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/tests/... File trytond/trytond/tests/test_field_selection.py (right): https://codereview.tryton.org/357911002/diff/443391003/trytond/trytond/tests/... trytond/trytond/tests/test_field_selection.py:249: Selection = Pool().get('test.selection') On 2022/09/17 08:09:40, nicoe wrote: > On 2022/07/18 21:07:24, ced wrote: > > I think we must enforce the style of separate instantiation of Pool for new > > tests. > > What do you mean? pool = Pool() Selection = pool.get('test.selection')
Sign in to reply to this message.
Missing documentation about the new read API, the new function on Selection and MultiSelection fields. Missing test for multiselection fields. https://codereview.tryton.org/357911002/diff/433501003/sao/src/model.js File sao/src/model.js (right): https://codereview.tryton.org/357911002/diff/433501003/sao/src/model.js#newco... sao/src/model.js:838: field = this.model.fields[name]; I think it will be better to define it as const here. https://codereview.tryton.org/357911002/diff/433501003/sao/src/view/tree.js File sao/src/view/tree.js (right): https://codereview.tryton.org/357911002/diff/433501003/sao/src/view/tree.js#n... sao/src/view/tree.js:2572: update_text: function(cell, record) { The same could be done here. https://codereview.tryton.org/357911002/diff/433501003/tryton/tryton/gui/wind... File tryton/tryton/gui/window/view_form/view/list_gtk/widget.py (right): https://codereview.tryton.org/357911002/diff/433501003/tryton/tryton/gui/wind... tryton/tryton/gui/window/view_form/view/list_gtk/widget.py:1135: def get_textual_value(self, record): The same could be done here. https://codereview.tryton.org/357911002/diff/433501003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/357911002/diff/433501003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Eagerly read string value of selection field trytond is not reading eargerly but expose selection string to read API so the changelog should be about that. https://codereview.tryton.org/357911002/diff/433501003/trytond/trytond/model/... File trytond/trytond/model/modelsql.py (right): https://codereview.tryton.org/357911002/diff/433501003/trytond/trytond/model/... trytond/trytond/model/modelsql.py:980: for row, instance in zip(rows, targets): why not named instance target? https://codereview.tryton.org/357911002/diff/433501003/trytond/trytond/model/... trytond/trytond/model/modelsql.py:983: value = fields.Selection.get_selection_string( Why not access the method from the field like field.get_selection_string() https://codereview.tryton.org/357911002/diff/433501003/trytond/trytond/model/... trytond/trytond/model/modelsql.py:988: value.append(fields.Selection.get_selection_string( idem
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
