Skip to content
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.

Sign up for GitHub

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

Allow Users to Pass Function to Measure-Tools Feature Tracking #1029

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
+63 −1
Draft

Allow Users to Pass Function to Measure-Tools Feature Tracking #1029

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
634bf48
feat: add feature tracking based off uievent
MikeNBentley Aug 30, 2024
b0f3dc3
fix: remove onFeaturedUsed from _props
MikeNBentley Sep 6, 2024
5afb9d4
refactor: add missing semicolon
MikeNBentley Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions packages/itwin/measure-tools/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,60 @@ An application can further customize UI event behavior by registering override h

A concrete example of this customization is an application that has measurements organized into multiple groups. One group may be "frozen" due to some application state (state that the measure-tools library may be unaware of) and should not be cleared by the clear measurements tool.
So the application would register a custom UI event handler that would cause those measurements to be ignored when the clear measurement tool is invoked.

### Usage Tracking

This package allows consumers to track the usage of specific features.

This can be achieved by passing `onFeatureUsed` function to `MeasureToolsUiProvider`. The function is invoked with the information about the feature being used. The feature information is based off Feature interface (that can be imported from `"@itwin/measure-tools-react"`)

```
export interface Feature {
name: string;
guid: GuidString;
metaData?: Map<string, any>;
}
```

As an example, for Measure Distance, we will have

```
{
name: "CRT_Tools_MeasureDistance",
guid: "10e474ee-9af8-4262-a505-77c9d896b065",
}
```

### Example for Usage Tracking

In this case, we create a sample [Itwin Viewer](https://www.npmjs.com/package/@itwin/web-viewer-react) and configure `MeasureToolsUiProvider`

```ts
import { Feature, MeasureToolsUiItemsProvider } from "@itwin/measure-tools-react";

const App: React.FC = () => {
// Viewer Setup here...
return (
<div className="viewer-container">
<Viewer
iTwinId={iTwinId ?? ""}
iModelId={iModelId ?? ""}
changeSetId={changesetId}
authClient={authClient}
viewCreatorOptions={viewCreatorOptions}
enablePerformanceMonitors={true}
onIModelAppInit={onIModelAppInit}
uiProviders={[
new MeasureToolsUiItemsProvider({
onFeatureUsed: (feature: Feature) => {
console.log(`MeasureTools [${feature.name}] used`);
},
}),
]}
/>
</div>
);
};

export default App;
```
10 changes: 9 additions & 1 deletion packages/itwin/measure-tools/src/ui-2.0/MeasureToolsUiProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { MeasureToolDefinitions } from "../tools/MeasureToolDefinitions";
import type { RecursiveRequired } from "../utils/types";
import { MeasurementPropertyWidget, MeasurementPropertyWidgetId } from "./MeasurementPropertyWidget";
import { IModelApp } from "@itwin/core-frontend";
import { Feature, FeatureTracking } from "../measure-tools-react";

// Note: measure tools cannot pick geometry when a sheet view is active to snap to and therefore must be hidden
// to avoid giving the user the impression they should work
Expand All @@ -32,11 +33,15 @@ export interface MeasureToolsUiProviderOptions {
// If we check for sheet to 3d transformation when measuring in sheets
enableSheetMeasurement?: boolean;
stageUsageList?: string[];
// Callback that is invoked when a tracked feature is used.
onFeatureUsed?: (feature: Feature) => void;
}

export class MeasureToolsUiItemsProvider implements UiItemsProvider {
public readonly id = "MeasureToolsUiItemsProvider";
private _props: RecursiveRequired<MeasureToolsUiProviderOptions>;
private _props: Omit<RecursiveRequired<MeasureToolsUiProviderOptions>, 'onFeatureUsed'> & {
onFeatureUsed?: (feature: Feature) => void;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


constructor(props?: MeasureToolsUiProviderOptions) {
this._props = {
Expand All @@ -48,7 +53,10 @@ export class MeasureToolsUiItemsProvider implements UiItemsProvider {
},
enableSheetMeasurement: props?.enableSheetMeasurement ?? false,
stageUsageList: props?.stageUsageList ?? [StageUsage.General],
onFeatureUsed: props?.onFeatureUsed,
};
if (!FeatureTracking.onFeature.numberOfListeners && this._props.onFeatureUsed)
Copy link
Contributor

@bsy-nicholasw bsy-nicholasw Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@MikeNBentley MikeNBentley Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@tcobbs-bentley tcobbs-bentley Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

FeatureTracking.onFeature.addListener(this._props.onFeatureUsed);
}

public provideToolbarItems(
Expand Down
Loading