iTwin / appui

Monorepo for iTwin.js AppUi
MIT License
8 stars 2 forks source link

SimplePropertyDataProvider does not update when data changes #299

Closed Jake-Screen closed 1 year ago

Jake-Screen commented 1 year ago

Describe the bug The MeasurementPropertyWidget from measure-tools-react uses the SimplePropertyDataProvider as its data provider. The data provider addProperty function is called, raising the onDataChanged event, but the data provider in the widget is never updated with the new data. This leads to the widget never being updated with the data from the measurements we create.

To Reproduce Steps to reproduce the behavior: There is an available test app in the viewer repo, under the branch next.

  1. clone the viewer repo
  2. checkout the next branch
  3. run rush install
  4. run rush build
  5. modify the .env file in /packages/apps/web-viewer-test with a client id, itwin id, and imodel id
  6. run rush start in the /packages/apps/web-viewer-test folder
  7. use the distance tool from the measure-tools provider
  8. widget should appear without the measurement just created

Expected behavior A section should be added to the measurement widget that appears on the right side of the screen that includes information on the measurement just created

Screenshots widget screenshot

Desktop (please complete the applicable information):

mdastous-bentley commented 1 year ago

I have a similar issue migrating from deprecated PropertyGrid to VirtualizedPropertyGridWithDataProvider: onDataChangedis fired but my data provider's getData() is never being called, and obviously the grid remains empty.

raplemie commented 1 year ago

@grigasp This component is usually maintained by your team, can someone in your team have a look.

A small comparison of presentation usage vs measure tool usage is that in the case of presentation, the whole data provider is recreated (as a filteredDataProvider) when the data actually changes, whereas other implementation expects the onDataChanged event to update the data in the view., which is likely why the current implementation issue seen by others were missed.

A simple workaround like below made the MeasureTool work, however, the problem likely lies in the current logic of this component.

image (Then use uniqueProvider instead of dataProvider in the VirtualizedPropertyGridWithDataProvider dataProvider props)

@Jake-Screen @mdastous-bentley Could this workaround be something that would allow you to release ?

grigasp commented 1 year ago

I haven't looked at the repro, but filtering has nothing to do with this working or not working - this sandbox clearly shows that.

grigasp commented 1 year ago

Ok, I think I see the problem - SimpleDataProvider:

  public async getData(): Promise<PropertyData> {
    return this;
  }

The property grid doesn't rebuild property grid model if the data doesn't change. And in this case it's mutating the state of the same PropertyData...

Quick workaround that I think should work:

class MyDataProvider extends SimplePropertyDataProvider {
  public async getData(): Promise<PropertyData> {
    return {...this};
  }
}

The property grid is likely to call this multiple times, so it would be good to memoize the value until the onDataChanged is raised.