-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
webadmin: Fix reseting of graphics #328
Conversation
/ost |
Looking at https://gerrit.ovirt.org/c/ovirt-engine/+/118132 it seems that the main problem was that:
However looking at ListModel constructor (link below) this should work - the handler should synchronize value from OvirtSelectionModel to ListModel. Regarding the new flag Line 155 in f9a19e2
|
Yes, the value is synchronized from the selection model to the selected item(s) but the problem is the other way around - when you set item(s) using This could be fixed but given that this is the key model used across the whole webadmin, it could break many other dialogs. |
OK. Fixing this problem is risky indeed. I would rather leave the responsibility for correct handling such cases close to the usage: either the code uses selection model or not. If it does use it then it should make sure that In case of
What do you think? |
I like the idea to explicitly mark "do not use the selection model" in the constructor. I'll update the PR. |
8ef7cb0
to
952c511
Compare
GWT library code seems to perform null-checks before accessing selection models but this is not consistent. |
When the BIOS type changed, the graphics list was always reset even though the video type stayed the same. This was because the setting of the items and selected item was separated in https://gerrit.ovirt.org/c/ovirt-engine/+/118132 This patch introduces a new constructor ListModel that allows to configure the selection model. That allows to set the video type field with NO_SELECTION model and avoid the synchronization that caused problems mentioned in https://gerrit.ovirt.org/c/ovirt-engine/+/118132. Bug-Url: https://bugzilla.redhat.com/2078193
e001822
to
c98e999
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment, other than that - lgtm
When the BIOS type changed, the graphics list was always reset even though the video type stayed the same.
This was because the setting of the items and selected item was separated in https://gerrit.ovirt.org/c/ovirt-engine/+/118132
This patch goes back to setting the items and selected item at once but making sure that the selection model is not
used to avoid issues fixed by https://gerrit.ovirt.org/c/ovirt-engine/+/118132
Bug-Url: https://bugzilla.redhat.com/2078193