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

revert(): rm active selection ref #9561

Merged
merged 6 commits into from
Jan 9, 2024
Merged
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
2 changes: 2 additions & 0 deletions .codesandbox/templates/next/components/Canvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export const Canvas = React.forwardRef<
ref.current = canvas;
}

// it is crucial `onLoad` is a dependency of this effect
// to ensure the canvas is disposed and re-created if it changes
onLoad?.(canvas);

return () => {
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- refactor(): rm active selection ref [#9561](https://github.com/fabricjs/fabric.js/pull/9561)
- feat(Next.js sandbox): simpler canvas hook [#9577](https://github.com/fabricjs/fabric.js/pull/9577)
- fix(): fix modify polygon points with zero sized polygons ( particular case of axis oriented lines ) [#9575](https://github.com/fabricjs/fabric.js/pull/9575)
- fix(Polyline, Polygon): Fix wrong pathOffset for polyline with the normal bounding box calculation. [#9460](https://github.com/fabricjs/fabric.js/pull/9460)
Expand Down
2 changes: 1 addition & 1 deletion src/ClassRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@
export const SVG = 'svg';

export class ClassRegistry {
declare [JSON]: Map<string, any>;

Check warning on line 19 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
declare [SVG]: Map<string, any>;

Check warning on line 20 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type

constructor() {
this[JSON] = new Map();
this[SVG] = new Map();
}

getClass(classType: string): any {
getClass<T>(classType: string): T {
const constructor = this[JSON].get(classType);
if (!constructor) {
throw new FabricError(`No class registered for ${classType}`);
Expand All @@ -32,7 +32,7 @@
return constructor;
}

setClass(classConstructor: any, classType?: string) {

Check warning on line 35 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
if (classType) {
this[JSON].set(classType, classConstructor);
} else {
Expand All @@ -43,11 +43,11 @@
}
}

getSVGClass(SVGTagName: string): any {

Check warning on line 46 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
return this[SVG].get(SVGTagName);
}

setSVGClass(classConstructor: any, SVGTagName?: string) {

Check warning on line 50 in src/ClassRegistry.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
this[SVG].set(
SVGTagName ?? classConstructor.type.toLowerCase(),
classConstructor
Expand Down
54 changes: 33 additions & 21 deletions src/canvas/Canvas.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { classRegistry } from '../ClassRegistry';
import { NONE } from '../constants';
import type {
CanvasEvents,
Expand All @@ -8,12 +9,14 @@ import type {
Transform,
} from '../EventTypeDefs';
import { Point } from '../Point';
import type { ActiveSelection } from '../shapes/ActiveSelection';
import type { Group } from '../shapes/Group';
import type { IText } from '../shapes/IText/IText';
import type { FabricObject } from '../shapes/Object/FabricObject';
import { isTouchEvent, stopEvent } from '../util/dom_event';
import { getDocumentFromElement, getWindowFromElement } from '../util/dom_misc';
import { sendPointToPlane } from '../util/misc/planeChange';
import { isActiveSelection } from '../util/typeAssertions';
import type { CanvasOptions, TCanvasOptions } from './CanvasOptions';
import { SelectableCanvas } from './SelectableCanvas';
import { TextEditingManager } from './TextEditingManager';
Expand Down Expand Up @@ -1387,10 +1390,9 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
return;
}
let hoverCursor = target.hoverCursor || this.hoverCursor;
const activeSelection =
this._activeObject === this._activeSelection
? this._activeObject
: null,
const activeSelection = isActiveSelection(this._activeObject)
? this._activeObject
: null,
// only show proper corner when group selection is not active
corner =
(!activeSelection || target.group !== activeSelection) &&
Expand Down Expand Up @@ -1431,8 +1433,7 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
*/
protected handleMultiSelection(e: TPointerEvent, target?: FabricObject) {
const activeObject = this._activeObject;
const activeSelection = this._activeSelection;
const isAS = activeObject === activeSelection;
const isAS = isActiveSelection(activeObject);
if (
// check if an active object exists on canvas and if the user is pressing the `selectionKey` while canvas supports multi selection.
!!activeObject &&
Expand All @@ -1455,9 +1456,8 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
!activeObject.getActiveControl()
) {
if (isAS) {
const prevActiveObjects =
activeSelection.getObjects() as FabricObject[];
if (target === activeSelection) {
const prevActiveObjects = activeObject.getObjects();
if (target === activeObject) {
const pointer = this.getViewportPoint(e);
target =
// first search active objects for a target to remove
Expand All @@ -1470,32 +1470,43 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
return false;
}
}
if (target.group === activeSelection) {
if (target.group === activeObject) {
// `target` is part of active selection => remove it
activeSelection.remove(target);
activeObject.remove(target);
this._hoveredTarget = target;
this._hoveredTargets = [...this.targets];
if (activeSelection.size() === 1) {
// if after removing an object we are left with one only...
if (activeObject.size() === 1) {
// activate last remaining object
this._setActiveObject(activeSelection.item(0) as FabricObject, e);
// deselecting the active selection will remove the remaining object from it
this._setActiveObject(activeObject.item(0), e);
}
} else {
// `target` isn't part of active selection => add it
activeSelection.multiSelectAdd(target);
this._hoveredTarget = activeSelection;
// `target` isn't part of active selection => add it
activeObject.multiSelectAdd(target);
this._hoveredTarget = activeObject;
this._hoveredTargets = [...this.targets];
}
this._fireSelectionEvents(prevActiveObjects, e);
} else {
(activeObject as IText).exitEditing &&
(activeObject as IText).exitEditing();
// add the active object and the target to the active selection and set it as the active object
activeSelection.multiSelectAdd(activeObject, target);
this._hoveredTarget = activeSelection;
const klass =
classRegistry.getClass<typeof ActiveSelection>('ActiveSelection');
const newActiveSelection = new klass([], {
/**
* it is crucial to pass the canvas ref before calling {@link ActiveSelection#multiSelectAdd}
* since it uses {@link FabricObject#isInFrontOf} which relies on the canvas ref
*/
canvas: this,
});
newActiveSelection.multiSelectAdd(activeObject, target);
this._hoveredTarget = newActiveSelection;
// ISSUE 4115: should we consider subTargets here?
// this._hoveredTargets = [];
// this._hoveredTargets = this.targets.concat();
this._setActiveObject(activeSelection, e);
this._setActiveObject(newActiveSelection, e);
this._fireSelectionEvents([activeObject], e);
}
return true;
Expand Down Expand Up @@ -1549,8 +1560,9 @@ export class Canvas extends SelectableCanvas implements CanvasOptions {
this.setActiveObject(objects[0], e);
} else if (objects.length > 1) {
// add to active selection and make it the active object
this._activeSelection.add(...objects);
this.setActiveObject(this._activeSelection, e);
const klass =
classRegistry.getClass<typeof ActiveSelection>('ActiveSelection');
this.setActiveObject(new klass(objects, { canvas: this }), e);
}

// cleanup
Expand Down
5 changes: 1 addition & 4 deletions src/canvas/CanvasOptions.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { ModifierKey, TOptionalModifierKey } from '../EventTypeDefs';
import type { ActiveSelection } from '../shapes/ActiveSelection';
import type { TOptions } from '../typedefs';
import type { StaticCanvasOptions } from './StaticCanvasOptions';

Expand Down Expand Up @@ -260,9 +259,7 @@ export interface CanvasOptions
preserveObjectStacking: boolean;
}

export type TCanvasOptions = TOptions<
CanvasOptions & { activeSelection: ActiveSelection }
>;
export type TCanvasOptions = TOptions<CanvasOptions>;

export const canvasDefaults: TOptions<CanvasOptions> = {
uniformScaling: true,
Expand Down
74 changes: 24 additions & 50 deletions src/canvas/SelectableCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import type {
} from '../EventTypeDefs';
import {
addTransformToObject,
resetObjectTransform,
saveObjectTransform,
} from '../util/misc/objectTransforms';
import type { TCanvasSizeOptions } from './StaticCanvas';
Expand All @@ -31,13 +30,13 @@ import type { IText } from '../shapes/IText/IText';
import type { BaseBrush } from '../brushes/BaseBrush';
import { pick } from '../util/misc/pick';
import { sendPointToPlane } from '../util/misc/planeChange';
import { ActiveSelection } from '../shapes/ActiveSelection';
import { cos, createCanvasElement, sin } from '../util';
import { CanvasDOMManager } from './DOMManagers/CanvasDOMManager';
import { BOTTOM, CENTER, LEFT, RIGHT, TOP } from '../constants';
import type { CanvasOptions, TCanvasOptions } from './CanvasOptions';
import type { CanvasOptions } from './CanvasOptions';
import { canvasDefaults } from './CanvasOptions';
import { Intersection } from '../Intersection';
import { isActiveSelection } from '../util/typeAssertions';

/**
* Canvas class
Expand Down Expand Up @@ -294,17 +293,6 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
protected declare _isCurrentlyDrawing: boolean;
declare freeDrawingBrush?: BaseBrush;
declare _activeObject?: FabricObject;
protected _activeSelection: ActiveSelection;

constructor(
el?: string | HTMLCanvasElement,
{ activeSelection = new ActiveSelection(), ...options }: TCanvasOptions = {}
) {
super(el, options);
this._activeSelection = activeSelection;
this._activeSelection.set('canvas', this);
this._activeSelection.setCoords();
}

protected initElements(el?: string | HTMLCanvasElement) {
this.elements = new CanvasDOMManager(el, {
Expand Down Expand Up @@ -1029,27 +1017,17 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
return this._activeObject;
}

/**
* Returns instance's active selection
*/
getActiveSelection() {
return this._activeSelection;
}

/**
* Returns an array with the current selected objects
* @return {FabricObject[]} active objects array
*/
getActiveObjects(): FabricObject[] {
const active = this._activeObject;
if (active) {
if (active === this._activeSelection) {
return [...(active as ActiveSelection)._objects];
} else {
return [active];
}
}
return [];
return isActiveSelection(active)
? active.getObjects()
: active
? [active]
: [];
}

/**
Expand Down Expand Up @@ -1134,20 +1112,22 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
* @return {Boolean} true if the object has been selected
*/
_setActiveObject(object: FabricObject, e?: TPointerEvent) {
if (this._activeObject === object) {
const prevActiveObject = this._activeObject;
if (prevActiveObject === object) {
return false;
}
// after calling this._discardActiveObject, this,_activeObject could be undefined
if (!this._discardActiveObject(e, object) && this._activeObject) {
// refused to deselect
return false;
}
if (object.onSelect({ e })) {
return false;
}

this._activeObject = object;

if (object instanceof ActiveSelection && this._activeSelection !== object) {
this._activeSelection = object;
if (isActiveSelection(object) && prevActiveObject !== object) {
object.set('canvas', this);
object.setCoords();
}
Expand All @@ -1173,11 +1153,6 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
if (obj.onDeselect({ e, object })) {
return false;
}
// clear active selection
if (obj === this._activeSelection) {
this._activeSelection.removeAll();
resetObjectTransform(this._activeSelection);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I removed this line but still the layout manager is present and does the same.
Let's see what will happen when I remove it

}
if (this._currentTransform && this._currentTransform.target === obj) {
// @ts-expect-error this method exists in the subclass - should be moved or declared as abstract
this.endCurrentTransform(e);
Expand Down Expand Up @@ -1227,11 +1202,13 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
*/
destroy() {
// dispose of active selection
const activeSelection = this._activeSelection;
activeSelection.removeAll();
// @ts-expect-error disposing
this._activeSelection = undefined;
activeSelection.dispose();
const activeObject = this._activeObject;
if (isActiveSelection(activeObject)) {
activeObject.removeAll();
activeObject.dispose();
}

delete this._activeObject;

super.destroy();

Expand Down Expand Up @@ -1271,7 +1248,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
/**
* @private
*/
_toObject(
protected _toObject(
instance: FabricObject,
methodName: 'toObject' | 'toDatalessObject',
propertiesToInclude: string[]
Expand All @@ -1293,14 +1270,11 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
* @param {FabricObject} [instance] the object to transform (gets mutated)
* @returns the original values of instance which were changed
*/
_realizeGroupTransformOnObject(
private _realizeGroupTransformOnObject(
instance: FabricObject
): Partial<typeof instance> {
if (
instance.group &&
instance.group === this._activeSelection &&
this._activeObject === instance.group
) {
const { group } = instance;
if (group && isActiveSelection(group) && this._activeObject === group) {
const layoutProps = [
'angle',
'flipX',
Expand All @@ -1313,7 +1287,7 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents>
TOP,
] as (keyof typeof instance)[];
const originalValues = pick<typeof instance>(instance, layoutProps);
addTransformToObject(instance, this._activeObject.calcOwnMatrix());
addTransformToObject(instance, group.calcOwnMatrix());
return originalValues;
} else {
return {};
Expand Down
2 changes: 1 addition & 1 deletion src/canvas/StaticCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -859,7 +859,7 @@ export class StaticCanvas<
/**
* @private
*/
_toObject(
protected _toObject(
instance: FabricObject,
methodName: TValidToObjectMethod,
propertiesToInclude?: string[]
Expand Down
4 changes: 3 additions & 1 deletion src/filters/Composed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ export class Composed extends BaseFilter {
) {
return Promise.all(
((object.subFilters || []) as BaseFilter[]).map((filter) =>
classRegistry.getClass(filter.type).fromObject(filter, options)
classRegistry
.getClass<typeof BaseFilter>(filter.type)
.fromObject(filter, options)
)
).then(
(enlivedFilters) => new this({ subFilters: enlivedFilters }) as BaseFilter
Expand Down
Loading
Loading