Bug 1565696 - UI plugin API - action button callbacks invoked multiple times
Summary: UI plugin API - action button callbacks invoked multiple times
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: ovirt-engine
Classification: oVirt
Component: Frontend.WebAdmin
Version: future
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ovirt-4.4.0
: ---
Assignee: rszwajko
QA Contact: Pavel Novotny
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-04-10 14:41 UTC by Vojtech Szocs
Modified: 2020-05-20 20:00 UTC (History)
6 users (show)

Fixed In Version: ovirt-engine 4.4.0 alpha 551beb5b
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-05-20 20:00:14 UTC
oVirt Team: UX
pm-rhel: ovirt-4.4+
pm-rhel: ovirt-4.5?
sgratch: planning_ack?
sgratch: devel_ack+
lleistne: testing_ack+


Attachments (Terms of Use)
test-ui-plugin (653 bytes, application/x-gzip)
2018-06-08 19:33 UTC, Vojtech Szocs
no flags Details


Links
System ID Private Priority Status Summary Last Updated
oVirt gerrit 106753 0 master MERGED webadmin: each action button updated n times 2020-04-20 16:04:41 UTC
oVirt gerrit 107090 0 master MERGED webadmin: unify handling of nested action buttons 2020-04-20 16:04:41 UTC
oVirt gerrit 107243 0 master MERGED webadmin: new callback for updating button state 2020-04-20 16:04:41 UTC

Description Vojtech Szocs 2018-04-10 14:41:57 UTC
When using following oVirt UI plugin API functions:

- addMenuPlaceActionButton
- addMainTabActionButton (legacy version of addMenuPlaceActionButton)

the button callbacks like "isEnabled" and "isAccessible" might be called multiple times after main grid selection change.

The expected behavior is to call such callbacks once after main grid selection change.

Steps to reproduce:

1, Use following code in your UI plugin:

```
const api = window.top.pluginApi('my-plugin')
api.addMainTabActionButton('VirtualMachine', 'Test', {
  isEnabled: function () {
    window.top.console.log('isEnabled called with', arguments)
    return arguments.length === 1
  }
  // same code as above for "isAccessible" callback
})
```

2, Go to VM main grid, observe that "isEnabled" is called once, as it should.

3, Select something in the grid, observe that "isEnabled" is called 14x, but it should be called only once.

3, Select something else in the grid, observe that "isEnabled" is called 3x, but it should be called only once.

Comment 1 Vojtech Szocs 2018-06-08 19:33:35 UTC
Created attachment 1449230 [details]
test-ui-plugin

Comment 2 Vojtech Szocs 2018-06-08 19:37:31 UTC
Update: this issue exists on both 4.2 and master.

4.2 behavior
- select one VM (click)
    button isAccessible called 17x (arguments ok)
    button isEnabled called 17x (arguments ok)
- select more VMs (shift-click)
    button isAccessible called 10x (arguments ok)
    button isEnabled called 10x (arguments ok)

master behavior
- select one VM (click)
    button isAccessible called 20x (arguments ok)
    button isEnabled called 20x (arguments ok)
- select more VMs (shift-click)
    button isAccessible called 12x (arguments ok)
    button isEnabled called 12x (arguments ok)

Attached UI plugin (test-ui-plugin) to reproduce the issue.

Comment 3 Sandro Bonazzola 2019-01-28 09:40:02 UTC
This bug has not been marked as blocker for oVirt 4.3.0.
Since we are releasing it tomorrow, January 29th, this bug has been re-targeted to 4.3.1.

Comment 5 rszwajko 2020-02-04 16:33:42 UTC
Scenarios from https://bugzilla.redhat.com/1513005 might be usefull for regression.

Comment 10 rszwajko 2020-02-27 16:01:48 UTC
Summary how things worked before and how they work after 3 patches - the number next to the steps is the how many times isEnabled was called in the test extension (attached to this issue).

Before the fix:                                                                        
1. reload page with Virtual Machines                                                         
2. change selection from running to stopped vm - 27x 
3. change selection from stopped to stopped - 12x
4. run a selected vm - ~78x
5. enter vm details  and go back
6. change selection from stopped to stopped - 48x
7. change selection from stopped to running - 108x
8. run a selected vm - ~56x

Note that before every button update(IntilizeEvent) triggered all other buttons to re-calculate their availability.
So with "n" buttons every selection change could result in n^2 calls to our test extension isEnabled method.
In practice the number of events is lower i.e. when changing selection from stopped vm to another stopped vm some buttons are not changing their state(no InitilizeEvent getting fired)  
Problems: 
1. (potentially) large number of calls - main issue report here
2. no direct link from nested button to its parent. However since every button listens to every InitilizeEvent then eventually parent also got triggered. 
3. no direct link between model changes(i.e. vm status) and buttons added via UI extension. In order to get triggered at least one other unrelated button had to emit InitilizeEvent. 
4. duplicated listeners if user enters detailed view. There is an assumption that one button definition is linked to one button. If a second button is added (i.e by entering detail view) then all the listeners are registered again. 

After the fix:
1. reload page 
2. change selection from vm A to vm B -  3x
3. run a selected vm - ~15x
4. enter vm details and go back to the list
5. change selection from row A to B  - 12x
6. run a selected vm - ~60x

Fixed problems:
1. large number of calls - fixed by limiting the scope of InitilizeEvent - each button reacts only to events addressed to him. 
   The number of callback invocation is now related to the number of sources that emit "refresh" event not to number of buttons. 
   In case of Virtual Machines screen it's a constant factor: 3x for standard case and 12x in case of duplicated listeners.
2. no direct link between parent and nested buttons - fixed by adding a rule that all nested buttons trigger update of the parent(root) button
3. no direct link between model changes - fixed by adding a common property change event that triggers refreshing both GWT based buttons and buttons originating from UI extensions
4. duplicated listeners - left as it is.

Comment 11 Pavel Novotny 2020-04-21 19:54:03 UTC
Verified with
ovirt-engine-4.4.0-0.33.master.el8ev.noarch
ovirt-engine-webadmin-portal-4.4.0-0.33.master.el8ev.noarch

UI test plugin from attachment 1449230 [details] was used for the verification.

I followed the steps from comment 10 and the number of calls were the same or close to those mentioned in "After the fix" paragraph.
1. select VM - 3x
2. change VM selection from VM A to VM B - 12x
2. run VM - ~12-14x
3. go to VM detail and back to VM list - 12x
4. run VM/power off VM - ~56-60x

Comment 12 Sandro Bonazzola 2020-05-20 20:00:14 UTC
This bugzilla is included in oVirt 4.4.0 release, published on May 20th 2020.

Since the problem described in this bug report should be
resolved in oVirt 4.4.0 release, it has been closed with a resolution of CURRENT RELEASE.

If the solution does not work for you, please open a new bug report.


Note You need to log in before you can comment on or make changes to this bug.