Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions __tests__/plots/interaction/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export { cars3LineBrushAxis } from './cars3-line-brush-axis';
export { cars3LineVerticalBrushAxis } from './cars3-line-vertical-brush-axis';
export { penguinsPointBrushAxis } from './penguins-point-brush-axis';
export { penguinsPointBrushFilter } from './penguins-point-brush-filter';
export { penguinsPointBrushFilterWithHistory } from './penguins-point-brush-filter-with-history';
export { settleWeatherCellBrushFilter } from './seattle-weather-cell-brush-filter';
export { profitIntervalBrushFilter } from './profit-interval-brush-filter';
export { penguinsPointBrushXFilter } from './penguins-point-brush-x-filter';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { G2Spec, PLOT_CLASS_NAME } from '../../../src';
import { brush, dblclick } from './penguins-point-brush';

export function penguinsPointBrushFilterWithHistory(): G2Spec {
return {
type: 'point',
data: {
type: 'fetch',
value: 'data/penguins.csv',
},
axis: { x: { animate: false }, y: { animate: false } },
encode: {
color: 'species',
x: 'culmen_length_mm',
y: 'culmen_depth_mm',
},
state: {
inactive: { stroke: 'gray' },
},
interaction: {
brushFilter: {
history: true,
},
},
};
}

penguinsPointBrushFilterWithHistory.steps = ({ canvas }) => {
const { document } = canvas;
const plot = document.getElementsByClassName(PLOT_CLASS_NAME)[0];
return [
{
changeState: () => {
brush(plot, 100, 100, 300, 300);
},
},
{
changeState: () => {
brush(plot, 100, 100, 300, 300);
},
},
{
changeState: () => {
dblclick(plot);
},
},
];
};
Comment on lines +28 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The test for the history feature seems incomplete and could be improved:

  1. Missing Snapshot: The test steps include a dblclick to trigger the undo functionality, but the corresponding snapshot (step3.svg) is missing from the pull request. This is the most critical part of the test for this feature and should be included to verify the undo behavior.
  2. Test Case Improvement: Both brush actions use the same coordinates (100, 100, 300, 300). While this tests the history mechanism, using different coordinates for the second brush would make the visual change between step1 and step2 clearer, and would provide a more robust verification when step3 reverts to the state of step1.

1 change: 1 addition & 0 deletions site/docs/manual/core/interaction/brushFilter.en.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ It can also be configured at the View level. Interactions declared on the view w
| Property | Description | Type | Default | Required |
| -------- | ------------------------ | ------------- | ----------------- | -------- |
| reverse | Whether to reverse brush | boolean | false | |
| history | Whether to histroy brush | boolean | false | |
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There's a typo here. It should be history instead of histroy.

Suggested change
| history | Whether to histroy brush | boolean | false | |
| history | Whether to history brush | boolean | false | |

| mask | Style of brush area mask | [mask](#mask) | See [mask](#mask) | |

### mask
Expand Down
1 change: 1 addition & 0 deletions site/docs/manual/core/interaction/brushFilter.zh.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ chart.render();
| 属性 | 描述 | 类型 | 默认值 | 必选 |
| ------- | ------------------ | ------------- | ------------------ | ---- |
| reverse | brush 是否反转 | boolean | false | |
| history | brush 框选记录 | boolean | false | |
| mask | 框选区域的蒙版样式 | [mask](#mask) | 详见 [mask](#mask) | |

### mask
Expand Down
140 changes: 93 additions & 47 deletions src/interaction/brushFilter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,12 @@ export function brushFilter(
};
}

export function BrushFilter({ hideX = true, hideY = true, ...rest }) {
export function BrushFilter({
hideX = true,
hideY = true,
history = false,
...rest
}) {
return (target, viewInstances, emitter) => {
const { container, view, options: viewOptions, update, setState } = target;
const plotArea = selectPlotArea(container);
Expand All @@ -96,8 +101,36 @@ export function BrushFilter({ hideX = true, hideY = true, ...rest }) {
let filtered = false;
let filtering = false;
let newView = view;

const brushHistory = [];
const { scale, coordinate } = view;
const updateScale = (options, domainX, domainY) => {
const { marks } = options;
const newMarks = marks.map((mark) =>
deepMix(
{
// Hide label to keep smooth transition.
axis: {
...(hideX && { x: { transform: [{ type: 'hide' }] } }),
...(hideY && { y: { transform: [{ type: 'hide' }] } }),
},
},
mark,
{
// Set nice to false to avoid modify domain.
scale: {
x: { domain: domainX, nice: false },
y: { domain: domainY, nice: false },
},
},
),
);

return {
...viewOptions,
marks: newMarks,
clip: true, // Clip shapes out of plot area.
};
};
return brushFilter(plotArea, {
brushRegion: (x, y, x1, y1) => [x, y, x1, y1],
selection: (x, y, x1, y1) => {
Expand All @@ -112,62 +145,75 @@ export function BrushFilter({ hideX = true, hideY = true, ...rest }) {
// Update the domain of x and y scale to filter data.
const [domainX, domainY] = selection;

setState('brushFilter', (options) => {
const { marks } = options;
const newMarks = marks.map((mark) =>
deepMix(
{
// Hide label to keep smooth transition.
axis: {
...(hideX && { x: { transform: [{ type: 'hide' }] } }),
...(hideY && { y: { transform: [{ type: 'hide' }] } }),
},
},
mark,
{
// Set nice to false to avoid modify domain.
scale: {
x: { domain: domainX, nice: false },
y: { domain: domainY, nice: false },
},
},
),
);

return {
...viewOptions,
marks: newMarks,
clip: true, // Clip shapes out of plot area.
};
});

setState('brushFilter', (options) =>
updateScale(options, domainX, domainY),
);
if (history) {
brushHistory.push({
domain: selection,
view: newView,
});
}
// Emit event.
emitter.emit('brush:filter', {
...event,
data: { selection: [domainX, domainY] },
data: {
...(history ? { history: brushHistory } : {}),
selection: [domainX, domainY],
},
});

const newState = await update();
newView = newState.view;
filtering = false;
filtered = true;
},
reset: (event) => {
reset: async (event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

This function is now async, but its caller, the click handler within the outer brushFilter function, does not await the call to reset. This can lead to race conditions and unpredictable behavior. The click handler should be updated to be async and await this promise.

// in brushFilter(...)
  async function click(e) {
    if (isDblclick(e)) {
      e.nativeEvent = true;
      await reset(e);
    }
  }

Although this change is outside the diff, it's critical for the correctness of this feature.

if (filtering || !filtered) return;
event.nativeEvent = true;

// Emit event.
const { scale } = view;
const { x: scaleX, y: scaleY } = scale;
const domainX = scaleX.getOptions().domain;
const domainY = scaleY.getOptions().domain;
emitter.emit('brush:filter', {
...event,
data: { selection: [domainX, domainY] },
});
filtered = false;
newView = view;
setState('brushFilter');
update();
// Restore previous brush state
if (brushHistory.length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The logic if (brushHistory.length > 1) for handling history is a bit counter-intuitive. It means that undoing the very first brush action results in a full reset, rather than stepping back to the initial state. A user would likely expect to be able to undo each brush step by step.

A more conventional undo behavior would be to pop from history on every dblclick until the history is empty.

Consider refactoring the logic to handle undo consistently for all levels of history. For example:

if (history && brushHistory.length > 0) {
  brushHistory.pop();
  if (brushHistory.length > 0) {
    // Restore previous state
  } else {
    // Reset to initial state
  }
} else {
  // Reset to initial state (if no history)
}

This would provide a more predictable multi-level undo experience.

brushHistory.pop();

// Get the last brush state
const { domain, view: lastView } =
brushHistory[brushHistory.length - 1];

const [domainX, domainY] = domain;
newView = lastView;

// update scale
setState('brushFilter', (options) =>
updateScale(options, domainX, domainY),
);

emitter.emit('brush:filter', {
...event,
data: {
...(history ? { history: brushHistory } : {}),
selection: [domainX, domainY],
},
});

await update();
} else {
// Reset to initial state
brushHistory.length = 0;
emitter.emit('brush:filter', {
...event,
data: {
...(history ? { history: brushHistory } : {}),
selection: [
scale.x.getOptions().domain,
scale.y.getOptions().domain,
],
},
});
setState('brushFilter');
newView = view;
filtered = false;
await update();
}
},
extent: undefined,
emitter,
Expand Down
1 change: 1 addition & 0 deletions src/spec/interaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export type BrushAxisHighlightInteraction = {
export type BrushFilterInteraction = {
type?: 'brushFilter';
reverse?: boolean;
history?: boolean;
} & Record<`${'mask'}${any}`, any>;

export type BrushXFilterInteraction = {
Expand Down
Loading