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.

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

Interaction functions #10046

Merged
merged 14 commits into from Mar 24, 2022
Merged

Interaction functions #10046

merged 14 commits into from Mar 24, 2022

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Jan 4, 2022

For discussion / work in progress - cleaning up and expanding the interaction modes API, as discussed in #9961.

Specific changes:

  • Consolidate getRelativePosition. See c765938 for details. I added a test to verify this; it depends on a local, not-yet-submitted change in chartjs-test-utils' triggerMouseEvent to return the mouse event that it triggers. I'll open a separate PR for that if this change seems reasonable.
  • Add a new public Chart method, isPointInArea, to refactor and publicize logic that was duplicated in core.controller.js and core.interactions.js.
  • Change getAxisItems to have an interface consistent with other get...Items functions.
  • Consolidate evaluateAllVisibleItems and optimizedEvaluateItems - optimizeEvaluateItems falls back to evaluating everything if the optimization fails, and I couldn't think of a reason why getAxisItems wouldn't want to benefit from the optimized binary search if it could.

Proposed API:

  1. getRelativePosition is already exported by 'chart.js/helpers' and, after these changes, can efficiently be used as is.
  2. Add a new public Chart method, isPointInArea.
  3. Rename optimizedEvaluateItems to evaluateInteractionItems and export it.
  4. Optional: Export the individual get...Items functions: getIntersectItems, getNearestItems, getAxisItems. (I went ahead and marked these and evaluateInteractionItems as export in the current PR.)
  5. Optional: Export getDistanceMetricForAxis.

I'm currently thinking that the get...Items and getDistanceMetricForAxis functions would not be exported: They're straightforward to implement using existing public APIs, and they may need to vary due to specific interaction needs, so the costs of freezing them as part of the public API may not outweigh the benefits.

For reference, if I were to export some sort of get...Items functionality, here's what I'm using for my own code. It's based off of getNearestItems, modified to take a callback: (This function could be exported, and getNearestCartesianItems could be changed to use this.)

function evaluateNearestItems(
  callback: (
    element: Element & VisualElement,
    datasetIndex: number,
    index: number,
    distance: number
  ) => void,
  chart: Chart,
  position: Point,
  axis: Axis,
  intersect?: boolean,
  useFinalPosition?: boolean
) {
  const distanceMetric = getDistanceMetricForAxis(axis);

  const minPadding = (chart as Chart & { _minPadding: number })._minPadding;
  if (!_isPointInArea(position, chart.chartArea, minPadding)) {
    return false;
  }

  const evaluationFunc: EvaluateHandler = (element, datasetIndex, index) => {
    if (intersect && !element.inRange(position.x, position.y, useFinalPosition)) {
      return;
    }

    const center = element.getCenterPoint(useFinalPosition);
    if (
      !_isPointInArea(center, chart.chartArea, minPadding) &&
      !element.inRange(position.x, position.y, useFinalPosition)
    ) {
      return;
    }
    const distance = distanceMetric(position, center);
    callback(element, datasetIndex, index, distance);
  };

  optimizedEvaluateItems(chart, axis, position, evaluationFunc);
  return true;
}

To work toward exposing something like the get...Items functions.
optimizedEvaluateItems falls back to evaluating all items for unsorted items, and sorting / optimizing ought to be okay, so this ought to be equivalent.
helpers.dom.js's getRelativePosition already had logic to handle ChartEvent vs. Event (as demonstrated by the `native` check within `getCanvasPosition`), so it's redundant for core.interaction.js to have its own `native` check.

Update `getRelativePosition` to use the same `native` check as core.interaction.js's version.  As best as I can tell, the ChartEvent's x and y are populated from `getRelativePosition`, so the previous `getCanvasPosition` was effectively just duplicating `getRelativePosition'`s work.  I added a test to verify this; it depends on a local, not-yet-submitted change in chartjs-test-utils' `triggerMouseEvent` to return the mouse event that it triggers.
@kurkle
Copy link
Member

kurkle commented Jan 7, 2022

Optional: Export the individual get...Items functions: getIntersectItems, getNearestItems, getAxisItems. (I went ahead and marked these and evaluateInteractionItems as export in the current PR.)

Would it actually be enough to only export these "higher level" functions? Would probably leave more room for internal changes.

@joshkel
Copy link
Contributor Author

joshkel commented Jan 7, 2022

@kurkle For my purposes, I don't think that would help. Going off of the two motivating examples from #9961 (nearestPerDataset, and nearest but with some sort of intersectRadius or maxDistance limit):

  • I don't see a good way to implement nearestPerDataset using the higher-level get...Items functions; it would need optimizedEvaluateItems / evaluateInteractionItems in order to see all of the candidates, so that it can evaluate per dataset.
  • nearest-with-limit could be implemented using getNearestItems, although it would then have to duplicate getNearestItems' distance calculation to decide whether items are too close.

(I realize that there's discussion of other changes that might remove the need for either or both of these specific examples, but hopefully the examples can still demonstrate what might make for a more broadly useful API.)

@kurkle
Copy link
Member

kurkle commented Jan 12, 2022

I don't have any objections to this.

@kurkle
Copy link
Member

kurkle commented Feb 5, 2022

... chartjs-test-utils' triggerMouseEvent to return the mouse event that it triggers. I'll open a separate PR for that if this change seems reasonable.

There is a change pending release on the test-utils, could also include your changes if you open the PR.

@joshkel
Copy link
Contributor Author

joshkel commented Feb 8, 2022

@kurkle Submitted as chartjs/chartjs-test-utils#19. Thanks for the reminder.

For this PR: I listed some optional functions for export as items 4 and 5 (the individual get...Items functions: getIntersectItems, getNearestItems, getAxisItems - and getDistanceMetricForAxis ) Do you have an opinion / preference on whether those should be included? I can go ahead and finalize the PR based on that decision.

Thank you.

kurkle pushed a commit to chartjs/chartjs-test-utils that referenced this pull request Feb 12, 2022
This allows calling code to do additional tests on the event; see chartjs/Chart.js#10046
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Just a few questions / comments. Overall I like the direction this is going

src/helpers/helpers.dom.js Show resolved Hide resolved
test/specs/helpers.dom.tests.js Outdated Show resolved Hide resolved
Add a non-null assertion, as requested in code review.

Add JSDoc to clarify that `getCanvasPosition` now expects a native `Event`, not a `ChartEvent`.  Add `@ts-ignore`; `getCanvasPosition` relied on some loose conversions between `Event`, `TouchEvent`, and `Touch` that would require several changes to make TypeScript happy.
kurkle
kurkle previously approved these changes Feb 14, 2022
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Couple of nits

src/core/core.interaction.js Outdated Show resolved Hide resolved
src/helpers/helpers.dom.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Feb 14, 2022

TypeError: Cannot use 'in' operator to search for 'native' in undefined

Return the event directly, to speed up the code a bit.  Add JSDoc to help communicate its intent.  Update various comments.
@joshkel
Copy link
Contributor Author

joshkel commented Feb 15, 2022

TypeError: Cannot use 'in' operator to search for 'native' in undefined

@kurkle Are you seeing this error? If so, where?

@joshkel
Copy link
Contributor Author

joshkel commented Feb 15, 2022

@kurkle and @etimberg , please see my latest commits:

  • I responded to the code review feedback. Thanks!
  • I updated how the exports are done - including within the default export seems to be Chart.js's convention, instead of exporting directly, and exporting directly wouldn't make the files accessible via import { something } from 'chart.js'.
  • I updated the docs and the TypeScript type definitions.
  • For now, I'm not exporting the individual get...Items functions. I'm not sure that they're flexible enough for custom interaction modes (e.g., I haven't used them myself); based on discussions about not wanting to commit to a broader API than necessary, I think it's more useful to start with justing export the building blocks (evaluateInteractionItems, Chart.isPointInArea, and the updated getRelativePosition). However, I'm happy to add them back if you like.

etimberg
etimberg previously approved these changes Feb 16, 2022
@kurkle
Copy link
Member

kurkle commented Feb 16, 2022

TypeError: Cannot use 'in' operator to search for 'native' in undefined

@kurkle Are you seeing this error? If so, where?

CI fails with that. I guess the event can be undefined in some case.

@tomaszkrym
Copy link

@kurkle looking at CI history ( https://github.com/chartjs/Chart.js/actions?query=workflow%3ACI+branch%3Amaster ) the tests seem to not be 100% deterministic, they sometimes fail randomly on master. The changes submitted by @joshkel (thank you for PR!) don't seem to touch code that failed tests in CI. Is it possible to rerun tests?

I would love to see this PR merged and released as we also need the "nearestPerDataset" mode in our project.

src/helpers/helpers.dom.js Show resolved Hide resolved
src/helpers/helpers.dom.js Outdated Show resolved Hide resolved
Only thing needed actually is the update of chartjs-test-utils to 0.4.0
@kurkle
Copy link
Member

kurkle commented Mar 23, 2022

So the CI fails because of old chartjs-test-utils that does not return an event. @joshkel can you update that? (not easy enough for me to do)

@joshkel
Copy link
Contributor Author

joshkel commented Mar 23, 2022

@kurkle Done. Thanks for following up on this, and sorry for the delay.

@joshkel joshkel changed the title Interaction functions (WIP) Interaction functions Mar 23, 2022
@kurkle kurkle closed this Mar 24, 2022
@kurkle kurkle reopened this Mar 24, 2022
@etimberg
Copy link
Member

@kurkle am I OK to merge this?

@kurkle
Copy link
Member

kurkle commented Mar 24, 2022

@kurkle am I OK to merge this?

Sure, I only closed/re-opened for actions to restart (they were down yesterday)

@etimberg etimberg merged commit c057c96 into chartjs:master Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants