Allow Users to Pass Function to Measure-Tools Feature Tracking

[MikeNBentley]

Resolve Issue #980

• Description: Based off solutions on tree-widget package and idea mentioned in this PR.
  Now, users can pass their callback function onFeatureUsed to MeasureToolsUiItemsProvider to handle features being used. There is an usage example in the updated README.

• Affected File(s):
  (Modified) packages/itwin/measure-tools/src/ui-2.0/MeasureToolsUiProvider.tsx,
  (Modified) packages/itwin/measure-tools/README.md

[tcobbs-bentley]

I guess I'm OK with this, but I'm not entirely sure why it's even needed. Couldn't you just update the readme to instruct people to use FeatureTracking.onFeature.addListener directly? I'm pretty sure that's what they have been doing up until now, and it's not clear to me that this solution is a substantial improvement over that.

[ben-polinsky]

This is getting a bit confusing, especially since it doesn't seem like this._props.onFeatureUsed is used outside of the constructor: why should it be added to this._props at all? One idea is to each of the _props members into its own private member. Then we wouldn't have this unnecessary coupling between internal state and arguments.

[MikeNBentley]

Thank you, onFeatureUsed is indeed used in constructor only. I will remove & { onFeatureUsed?: (feature: Feature) => void; }; part and directly use props?.onFeatureUsed instead. This is also reasonable as it does not make a lot of sense to initialize onFeatureUsed with any default value if it is a member of this._props

this._props = {
      itemPriority: props?.itemPriority ?? 20,
      groupPriority: props?.groupPriority ?? 10,
      widgetPlacement: {
        location: props?.widgetPlacement?.location ?? StagePanelLocation.Right,
        section: props?.widgetPlacement?.section ?? StagePanelSection.Start,
      },
      enableSheetMeasurement: props?.enableSheetMeasurement ?? false,
      stageUsageList: props?.stageUsageList ?? [StageUsage.General],
      onFeatureUsed: props?.onFeatureUsed, // will also remove this line
    };

About the idea to each of the _props members into its own private member, I think we can do that in another PR where we can replace _props usage with those private properties. But does that mean we do not need RecursiveRequired introduced in this commit (to make sure all _props members initialized with at least some value) anymore?

[ben-polinsky]

Sure, I was just thinking that perhaps someone would come along and do something similar for another arg that wasn't used outside of the constructor, but whichever you feel more comfortable doing. Just an observation. This repo/file isn't my code so don't want to impose.

[ben-polinsky]

I guess I'm OK with this, but I'm not entirely sure why it's even needed. Couldn't you just update the readme to instruct people to use FeatureTracking.onFeature.addListener directly? I'm pretty sure that's what they have been doing up until now, and it's not clear to me that this solution is a substantial improvement over that.

I see that, but on the other hand it's sort of reaching into the library's internals? I know FeatureTracking is currently public, but if we do want to change the implementation it's better to have consumers use this. Consumers shouldn't be able to fire off feature tracking events, either, so we really shouldn't be exposing the BeEvent itself.

[bsy-nicholasw]

@aruniverse Is there a push for packages to adopt this pattern? It doesn't really add much other than better discoverability, and beginning to move away from package statics. I just want to understand what/who is driving this.

@ben-polinsky IMO the same can then be said for every public event on any public API, but it's a common pattern to expose it in the style of a C# event.

[bsy-nicholasw]

I'd nix the condition checking the number of listeners.

@aruniverse Do UI providers ever get cleaned up? Or only ever created once per app lifetime?

[MikeNBentley]

The reason for !FeatureTracking.onFeature.numberOfListeners is as I tested on a sample viewer this listener was added twice. I guess the UI provider might get cleaned up and we have not removed this listener before re-creating the UI provider (but I am not really clear on when it gets cleaned up or why it might be created twice).

[tcobbs-bentley]

With FeatureTracking.onFeature being public, I don't think it's appropriate to assume that nobody else is listening for that event. It doesn't seem acceptable to have this not work because the event is already being listened to by somebody else.

[bsy-nicholasw]

Looks like the interface has a cleanup mechanism: https://www.itwinjs.org/reference/appui-react/uiprovider/uiitemsprovider/onunregister/

TBH something like this belongs better in the MeasureTools.startup which the app has to call anyways. It also has an associated terminate, which should be called alongside the IModelApp startup/shutdown by the app.

[MikeNBentley]

Sure, I can implement it as a startup option instead. However, its usage will be different from tree-widget package (where you can provide onFeatureUsed callback function to UI Provider). If you are fine with that, I will do it right away. Thank you!

[ben-polinsky]

IMO the same can then be said for every public event on any public API, but it's a common pattern to expose it in the style of a C# event.

Perhaps, but doesn't make it great for situations where there's really no reason for consumers to emit. Just my two cents, I get it's already public, so maybe more trouble than it is worth.

[aruniverse]

@aruniverse Is there a push for packages to adopt this pattern? It doesn't really add much other than better discoverability, and beginning to move away from package statics. I just want to understand what/who is driving this.

There is no immediate push for packages to adopt this pattern. If packages need to provide feature tracking this is probably the most preferred way to do it so apps can inject that themselves, instead of every package having to create their own static feature tracker and event emitter. This PR mainly stems from removing the internal Feature tracking apis from core, and making it easier in case people need a replacement. I have no guarantees Apps are even hooking into the existing feature tracking.

[aruniverse]

Perhaps, but doesn't make it great for situations where there's really no reason for consumers to emit. Just my two cents, I get it's already public, so maybe more trouble than it is worth.

Although it's marked "public" but technically its a 0.x pkg so it could happen