On Thu Apr 4, 2024 at 12:54 PM CEST, Stefan Sterz wrote: > On Thu Apr 4, 2024 at 12:10 PM CEST, Friedrich Weber wrote: > > On 04/04/2024 11:23, Stefan Sterz wrote: > > > -- >8 snip 8< -- > > >>> > > >>> i did a quick an dirty test and using a constructor like this seems to > > >>> rule out this class of bug completelly: > > >>> > > >>> ```js > > >>> constructor: function(conf) { > > >>> let me = this; > > >>> me.extraRequestParams = {}; > > >>> me.initConfig(conf); > > >>> me.callParent(); > > >>> }, > > >>> ``` > > >>> > > >>> basically it configures the edit window as usual, but overwrites the > > >>> `extraRequestParams` object for each instance with a new empty object. > > >>> so there are no more shared objects :) could you check whether that also > > >>> fixes the other instances? > > >>> > > >>> [1]: > > >>> https://docs-devel.sencha.com/extjs/7.0.0/classic/Ext.window.Window.html#method-constructor > > >> > > >> Nifty, didn't think about a constructor solution. Such a general > > >> solution would be way more elegant, thanks for suggesting it! > > >> > > >> However, this particular constructor seems to break the pattern of > > >> defining `extraRequestParams` in the subclass properties, as done by > > >> `PVE.Pool.AddVM` [1]. With the constructor above, the API request done > > >> by `AddVM` seems to be missing the `allow-move` parameter. > > >> > > >> Looks like once `PVE.Pool.AddVM` is instantiated and the constructor is > > >> called, `extraRequestParams` with `allow-move` is only defined in > > >> `me.__proto__`, so `me.extraRequestParams = {}` essentially shadows it > > >> with an empty object, losing the `allow-move`. > > >> > > > > > > not sure what you mean by that, if an `PVE.Pool.AddVM` is instantiated, > > > the `extraRequestParams` is already set, so it isn't just in `__proto__` > > > for me. but yeah, the problem is correct as `me.extraRequestParams = {}` > > > overwrites the field. > > > > I agree it doesn't matter here, but just for completeness, I meant that > > if I set a breakpoint before line 2, so before the overwrite: > > > > ```js > > constructor: function(conf) { > > let me = this; > > => me.extraRequestParams = {}; > > me.initConfig(conf); > > me.callParent(); > > }, > > ``` > > > > ... `extraRequestParams` is not a property of `me`, but inherited from > > its prototype: > > > > ``` > > >> me.extraRequestParams > > Object { "allow-move": 1 } > > >> "extraRequestParams" in me > > true > > >> Object.hasOwn(me, "extraRequestParams") > > false > > ``` > > > > Doesn't make a difference for the overwrite, though. > > > > ah yeah, that makes sense, but yeah, it doesn't really matter here i > suppose. > > > >> Do you have an idea how to fix this? Maybe making a copy of > > >> `extraRequestParams` would work (I suppose the overhead of creating a > > >> new object for all edit window (subclass) instances is negligible). > > >> > > >> [1] > > >> https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/PoolMembers.js;h=75f20cab;hb=4b06efb5#l9 > > > > > > this worked for me, can you confirm that this also does what it should > > > for you? > > > > > > ```js > > > extraRequestParams: undefined, > > > > > > constructor: function(conf) { > > > let me = this; > > > if (!me.extraRequestParams) { > > > me.extraRequestParams = {}; > > > } > > > me.initConfig(conf); > > > me.callParent(); > > > }, > > > ``` > > > > It works in the sense that it fixes the bug mentioned in my patch 1/3, > > and fixes the lost `allow-move` issue from the previous constructor. But > > with this constructor, all instances of `AddVM` share the same > > `extraRequestParams` (the body of the `if` never gets executed for > > `AddVM` instances), which is the condition that my patch 2/3 tries to > > avoid (even though it is currently not buggy). > > > > Maybe we could do: > > > > ```js > > extraRequestParams: {}, > > > > constructor: function(conf) { > > let me = this; > > me.extraRequestParams = Ext.clone(me.extraRequestParams); > > me.initConfig(conf); > > me.callParent(); > > }, > > ``` > > > > ... which, if I'm not missing anything, *should* cover everything (with > > the cost of allocating unnecessary empty objects)? > > yeah looks good to me. cloning shouldn't cost too much here. if we are > really worried we could check whether the object is empty, clone > it in that case and assign an empty object otherwise. >
i just looked through the code for `Ext.clone` [1], if i'm not mistaken in modern browsers (which we restrict ourselves to anyway, mostly), this basically returns an empty object anyway, we would skip a few assignments and checks, but i doubt performance would improve to the point where adding such a check makes sense. [1]: https://docs.sencha.com/extjs/7.0.0/modern/src/Ext.js.html#Ext-method-clone > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel