PrivateCoffee/tldraw
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

closing in on group selection bugs

Browse source
This commit is contained in:
Steve Ruiz 2021-09-03 10:45:36 +01:00
parent 0b93e5a041
commit 9461935aca
7 changed files with 374 additions and 87 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
packages/core/src/components/shape/shape.tsx
View file

@ -29,6 +29,7 @@ export const Shape = React.memo(
transform={transform}
{...events}
>
<text>{shape.childIndex}</text>
{isEditing && utils.isEditableText ? (
<EditingTextShape
shape={shape}

4
packages/core/src/hooks/useShapeTree.tsx
View file

@ -83,7 +83,9 @@ export function useShapeTree<T extends TLShape, M extends Record<string, unknown
width: maxY - minY,
}
// Filter shapes that are in view
// Filter shapes that are in view, and that are the direct child of
// the page. Other shapes are not visible, or will be rendered as
// the children of groups.
const shapesToRender = Object.values(page.shapes).filter((shape) => {
if (shape.parentId !== page.id) return false

173
packages/tldraw/src/state/command/group/group.command.spec.ts
View file

@ -122,6 +122,12 @@ describe('Group command', () => {
*/
it('creates a new group on the page', () => {
/*
When the selected shapes are the children of another group, and so
long as the children do not represent ALL of the group's children,
then a new group should be created that is a child of the parent group.
*/
tlstate.resetDocument().createShapes(
{
id: 'rect1',
@ -200,49 +206,144 @@ describe('Group command', () => {
expect(tlstate.getShape<GroupShape>('newGroupB').children).toStrictEqual(['rect1', 'rect3'])
})
/*
When the selected shapes are the children of another group, and so
long as the children do not represent ALL of the group's children,
then a new group should be created that is a child of the parent group.
*/
it.todo('does not group shapes if shapes are all the groups children')
/*
it('does not group shapes if shapes are all the groups children', () => {
/*
If the selected shapes represent ALL of the children of the a
group, then no effect should occur.
*/
tlstate.resetDocument().createShapes(
{
id: 'rect1',
type: TLDrawShapeType.Rectangle,
childIndex: 1,
},
{
id: 'rect2',
type: TLDrawShapeType.Rectangle,
childIndex: 2,
},
{
id: 'rect3',
type: TLDrawShapeType.Rectangle,
childIndex: 3,
}
)
it.todo('creates the new group as a child of the parent group')
/*
The new group should be a child of the parent group.
*/
it('moves the selected layers to the new group', () => {
/*
The new group should have the selected children. The old parents
should no longer have the selected shapes among their children.
All of the selected shapes should be assigned the new parent.
*/
tlstate.group(['rect1', 'rect2', 'rect3'], 'newGroupA')
tlstate.group(['rect1', 'rect2', 'rect3'], 'newGroupB')
expect(tlstate.getShape<GroupShape>('newGroupB')).toBeUndefined()
})
it.todo('deletes any groups that no longer have children')
/*
If the selected groups included the children of another group, then
that group should be destroyed. Other rules around deleted
shapes should here apply: bindings connected to the group
should be deleted, etc.
it('deletes any groups that no longer have children', () => {
/*
If the selected groups included the children of another group
in addition to other shapes then that group should be destroyed.
Other rules around deleted shapes should here apply: bindings
connected to the group should be deleted, etc.
*/
tlstate.resetDocument().createShapes(
{
id: 'rect1',
type: TLDrawShapeType.Rectangle,
childIndex: 1,
},
{
id: 'rect2',
type: TLDrawShapeType.Rectangle,
childIndex: 2,
},
{
id: 'rect3',
type: TLDrawShapeType.Rectangle,
childIndex: 3,
}
)
tlstate.group(['rect1', 'rect2'], 'newGroupA')
tlstate.group(['rect1', 'rect2', 'rect3'], 'newGroupB')
expect(tlstate.getShape<GroupShape>('newGroupA')).toBeUndefined()
expect(tlstate.getShape<GroupShape>('newGroupB').children).toStrictEqual([
'rect1',
'rect2',
'rect3',
])
})
it('marges selected groups that no longer have children', () => {
/*
If the user is creating a group while having selected other
groups, then the selected groups should be destroyed and a new
group created with the selected shapes and the group(s)' children.
*/
tlstate.resetDocument().createShapes(
{
id: 'rect1',
type: TLDrawShapeType.Rectangle,
childIndex: 1,
},
{
id: 'rect2',
type: TLDrawShapeType.Rectangle,
childIndex: 2,
},
{
id: 'rect3',
type: TLDrawShapeType.Rectangle,
childIndex: 3,
}
)
tlstate.group(['rect1', 'rect2'], 'newGroupA')
tlstate.group(['newGroupA', 'rect3'], 'newGroupB')
expect(tlstate.getShape<GroupShape>('newGroupA')).toBeUndefined()
expect(tlstate.getShape<GroupShape>('newGroupB').children).toStrictEqual([
'rect1',
'rect2',
'rect3',
])
tlstate.undo()
expect(tlstate.getShape<GroupShape>('newGroupB')).toBeUndefined()
expect(tlstate.getShape<GroupShape>('newGroupA')).toBeDefined()
expect(tlstate.getShape<GroupShape>('newGroupA').children).toStrictEqual(['rect1', 'rect2'])
tlstate.redo()
expect(tlstate.getShape<GroupShape>('newGroupA')).toBeUndefined()
expect(tlstate.getShape<GroupShape>('newGroupB')).toBeDefined()
expect(tlstate.getShape<GroupShape>('newGroupB').children).toStrictEqual([
'rect1',
'rect2',
'rect3',
])
})
it.todo('preserves the child index order')
/*
The layers should be in the same order as the original layers as
they would have appeared on a layers tree (lowest child index
first, parent inclusive).
*/
it.todo('preserves the child index order')
/* --------------------- Nesting -------------------- */
// it.todo('creates the new group as a child of the parent group')
/*
The new group should be a child of the parent group.
*/
// it.todo('moves the selected layers to the new group')
/*
The new group should have the selected children. The old parents
should no longer have the selected shapes among their children.
All of the selected shapes should be assigned the new parent.
*/
})
describe('when grouping shapes with different parents', () => {
/*
// describe('when grouping shapes with different parents', () => {
/*
When two shapes with different parents are grouped, the new parent
group should have the same parent as the shape nearest to the top
of the layer tree. The new group's child index should be that
@ -291,10 +392,20 @@ describe('Group command', () => {
the top.
*/
it.todo('creates a group in the correct place')
/*
// it.todo('creates a group in the correct place')
/*
The new group should be a child of the nearest shape to the top
of the tree.
*/
})
/*
If the selected groups included the children of another group, then
that group should be destroyed. Other rules around deleted
shapes should here apply: bindings connected to the group
should be deleted, etc.
*/
// it.todo('deletes any groups that no longer have children')
// })
})

46
packages/tldraw/src/state/command/group/group.command.ts
View file

@ -17,17 +17,32 @@ export function group(
const { currentPageId } = data.appState
const initialShapes = ids.map((id) => TLDR.getShape(data, id, currentPageId))
const idsToGroup = [...ids]
const shapesToGroup: TLDrawShape[] = []
const deletedGroupIds: string[] = []
const otherEffectedGroups: TLDrawShape[] = []
// Collect all of the shapes to group (and their ids)
for (const id of ids) {
const shape = TLDR.getShape(data, id, currentPageId)
if (shape.children === undefined) {
shapesToGroup.push(shape)
} else {
otherEffectedGroups.push(shape)
idsToGroup.push(...shape.children)
shapesToGroup.push(...shape.children.map((id) => TLDR.getShape(data, id, currentPageId)))
}
}
// 1. Can we create this group?
// Do the shapes have the same parent?
if (initialShapes.every((shape) => shape.parentId === initialShapes[0].parentId)) {
if (shapesToGroup.every((shape) => shape.parentId === shapesToGroup[0].parentId)) {
// Is the common parent a shape (not the page)?
if (initialShapes[0].parentId !== currentPageId) {
const commonParent = TLDR.getShape(data, initialShapes[0].parentId, currentPageId)
if (shapesToGroup[0].parentId !== currentPageId) {
const commonParent = TLDR.getShape(data, shapesToGroup[0].parentId, currentPageId)
// Are all of the common parent's shapes selected?
if (commonParent.children?.length === ids.length) {
if (commonParent.children?.length === idsToGroup.length) {
// Don't create a group if that group would be the same as the
// existing group.
return
@ -40,11 +55,11 @@ export function group(
// A map of shapes to their index in flattendShapes
const shapeIndexMap = Object.fromEntries(
initialShapes.map((shape) => [shape.id, flattenedShapes.indexOf(shape)])
shapesToGroup.map((shape) => [shape.id, flattenedShapes.indexOf(shape)])
)
// An array of shapes in order by their index in flattendShapes
const sortedShapes = initialShapes.sort((a, b) => shapeIndexMap[a.id] - shapeIndexMap[b.id])
const sortedShapes = shapesToGroup.sort((a, b) => shapeIndexMap[a.id] - shapeIndexMap[b.id])
// The parentId is always the current page
const groupParentId = currentPageId // sortedShapes[0].parentId
@ -57,7 +72,7 @@ export function group(
).childIndex
// The shape's point is the min point of its childrens' common bounds
const groupBounds = Utils.getCommonBounds(initialShapes.map((shape) => TLDR.getBounds(shape)))
const groupBounds = Utils.getCommonBounds(shapesToGroup.map((shape) => TLDR.getBounds(shape)))
// Create the group
beforeShapes[groupId] = undefined
@ -71,9 +86,6 @@ export function group(
children: sortedShapes.map((shape) => shape.id),
})
// Collect parents (other groups) that will have lost children
const otherEffectedGroups: TLDrawShape[] = []
// Reparent shapes to the new group
sortedShapes.forEach((shape, index) => {
// If the shape is part of a different group, mark the parent shape for cleanup
@ -95,9 +107,6 @@ export function group(
}
})
// These are the ids of deleted groups
const deletedShapeIds: string[] = []
// Clean up effected parents
while (otherEffectedGroups.length > 0) {
const shape = otherEffectedGroups.pop()
@ -105,7 +114,7 @@ export function group(
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const nextChildren = (beforeShapes[shape.id]?.children || shape.children)!.filter(
(childId) => childId && !(ids.includes(childId) || deletedShapeIds.includes(childId))
(childId) => childId && !(idsToGroup.includes(childId) || deletedGroupIds.includes(childId))
)
// If the parent has no children, remove it
@ -114,8 +123,9 @@ export function group(
afterShapes[shape.id] = undefined
// And if that parent is part of a different group, mark it for cleanup
// (This is necessary only when we implement nested groups.)
if (shape.parentId !== currentPageId) {
deletedShapeIds.push(shape.id)
deletedGroupIds.push(shape.id)
otherEffectedGroups.push(TLDR.getShape(data, shape.parentId, currentPageId))
}
} else {
@ -131,7 +141,7 @@ export function group(
}
}
// TODO: This code is copied from delete.command, create a shared helper
// TODO: This code is copied from delete.command. Create a shared helper!
const page = TLDR.getPage(data, currentPageId)
@ -163,7 +173,7 @@ export function group(
// Unless we're currently deleting the shape, remove the
// binding reference from the after patch
if (!deletedShapeIds.includes(id)) {
if (!deletedGroupIds.includes(id)) {
afterShapes[id] = {
...afterShapes[id],
handles: {

111
packages/tldraw/src/state/tlstate.spec.ts
View file

@ -75,7 +75,6 @@ describe('TLDrawState', () => {
tlstate.loadDocument(mockDocument).deselectAll()
tlu.clickShape('rect1')
tlu.clickShape('rect2', { shiftKey: true })
expect(tlstate.selectedIds).toStrictEqual(['rect1', 'rect2'])
tlu.clickShape('rect2', { shiftKey: true })
expect(tlstate.selectedIds).toStrictEqual(['rect1'])
expect(tlstate.appState.status.current).toBe('idle')
@ -108,13 +107,6 @@ describe('TLDrawState', () => {
expect(tlstate.appState.status.current).toBe('idle')
})
it('does not select on meta-shift-click', () => {
tlstate.loadDocument(mockDocument).deselectAll()
tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true })
expect(tlstate.selectedIds).toStrictEqual([])
expect(tlstate.appState.status.current).toBe('idle')
})
it.todo('deletes shapes if cancelled during creating')
it.todo('deletes shapes on undo after creating')
@ -232,8 +224,6 @@ describe('TLDrawState', () => {
describe('Mutates bound shapes', () => {
const tlstate = new TLDrawState()
tlstate
.createShapes(
{
id: 'rect',
@ -269,4 +259,105 @@ describe('TLDrawState', () => {
expect(tlstate.getShape('arrow').style.color).toBe(ColorStyle.Red)
expect(tlstate.getShape('rect').style.color).toBe(ColorStyle.Red)
})
describe('when selecting shapes in a group', () => {
it('selects the group when a grouped shape is clicked', () => {
const tlstate = new TLDrawState()
.loadDocument(mockDocument)
.group(['rect1', 'rect2'], 'groupA')
const tlu = new TLStateUtils(tlstate)
tlu.clickShape('rect1')
expect(tlstate.selectedGroupId).toBeUndefined()
expect(tlstate.selectedIds).toStrictEqual(['groupA'])
})
it('selects the grouped shape when double clicked', () => {
const tlstate = new TLDrawState()
.loadDocument(mockDocument)
.group(['rect1', 'rect2'], 'groupA')
const tlu = new TLStateUtils(tlstate)
tlu.doubleClickShape('rect1')
expect(tlstate.selectedGroupId).toStrictEqual('groupA')
expect(tlstate.selectedIds).toStrictEqual(['rect1'])
})
it('clears the selectedGroupId when selecting a different shape', () => {
const tlstate = new TLDrawState()
.loadDocument(mockDocument)
.group(['rect1', 'rect2'], 'groupA')
const tlu = new TLStateUtils(tlstate)
tlu.doubleClickShape('rect1')
tlu.clickShape('rect3')
expect(tlstate.selectedGroupId).toBeUndefined()
expect(tlstate.selectedIds).toStrictEqual(['rect3'])
})
it('selects a grouped shape when meta-shift-clicked', () => {
const tlstate = new TLDrawState()
.loadDocument(mockDocument)
.group(['rect1', 'rect2'], 'groupA')
.deselectAll()
const tlu = new TLStateUtils(tlstate)
tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true })
expect(tlstate.selectedIds).toStrictEqual(['rect1'])
tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true })
expect(tlstate.selectedIds).toStrictEqual([])
})
it('selects a hovered shape from the selected group when meta-shift-clicked', () => {
const tlstate = new TLDrawState()
.loadDocument(mockDocument)
.group(['rect1', 'rect2'], 'groupA')
const tlu = new TLStateUtils(tlstate)
tlu.hoverShape('rect1')
tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true })
expect(tlstate.selectedIds).toStrictEqual(['rect1'])
tlu.clickShape('rect1', { ctrlKey: true, shiftKey: true })
expect(tlstate.selectedIds).toStrictEqual([])
})
})
describe('when creating shapes', () => {
it('Creates shapes with the correct child index', () => {
const tlstate = new TLDrawState()
.createShapes(
{
id: 'rect1',
type: TLDrawShapeType.Rectangle,
childIndex: 1,
},
{
id: 'rect2',
type: TLDrawShapeType.Rectangle,
childIndex: 2,
},
{
id: 'rect3',
type: TLDrawShapeType.Rectangle,
childIndex: 3,
}
)
.selectTool(TLDrawShapeType.Rectangle)
.createActiveToolShape([0, 0], 'rect4')
expect(tlstate.getShape('rect4').childIndex).toBe(4)
tlstate
.group(['rect2', 'rect3', 'rect4'], 'groupA')
.selectTool(TLDrawShapeType.Rectangle)
.createActiveToolShape([0, 0], 'rect5')
expect(tlstate.getShape('groupA').childIndex).toBe(2)
expect(tlstate.getShape('rect5').childIndex).toBe(3)
})
})
})

114
packages/tldraw/src/state/tlstate.ts
View file

@ -113,6 +113,8 @@ export class TLDrawState extends StateManager<Data> {
isCreating = false
selectedGroupId?: string
constructor(id = Utils.uniqueId()) {
super(initialData, id, 2, (prev, next, prevVersion) => {
const state = { ...prev }
@ -1765,18 +1767,9 @@ export class TLDrawState extends StateManager<Data> {
brushUpdater.clear()
break
}
case TLDrawStatus.Translating: {
this.cancelSession()
break
}
case TLDrawStatus.Transforming: {
this.cancelSession()
break
}
case TLDrawStatus.Rotating: {
this.cancelSession()
break
}
case TLDrawStatus.Translating:
case TLDrawStatus.Transforming:
case TLDrawStatus.Rotating:
case TLDrawStatus.Creating: {
this.cancelSession()
break
@ -1870,8 +1863,7 @@ export class TLDrawState extends StateManager<Data> {
return this
}
createActiveToolShape = (point: number[]): this => {
const id = Utils.uniqueId()
createActiveToolShape = (point: number[], id = Utils.uniqueId()): this => {
const pagePoint = Vec.round(this.getPagePoint(point))
if (this.appState.activeTool === 'select') return this
@ -1883,7 +1875,11 @@ export class TLDrawState extends StateManager<Data> {
const shapes = this.getShapes()
const childIndex =
shapes.length === 0 ? 1 : shapes.sort((a, b) => b.childIndex - a.childIndex)[0].childIndex + 1
shapes.length === 0
? 1
: shapes
.filter((shape) => shape.parentId === this.currentPageId)
.sort((a, b) => b.childIndex - a.childIndex)[0].childIndex + 1
this.patchState(
{
@ -2141,7 +2137,7 @@ export class TLDrawState extends StateManager<Data> {
} else if (this.selectedIds.includes(info.target)) {
// If we're holding shift...
if (info.shiftKey) {
// Unless we just shift-selected the shape, remove it from the selected shapes
// unless we just shift-selected the shape, remove it from the selected shapes
if (this.pointedId !== info.target) {
this.select(...this.selectedIds.filter((id) => id !== info.target))
}
@ -2259,19 +2255,59 @@ export class TLDrawState extends StateManager<Data> {
switch (this.appState.activeTool) {
case 'select': {
if (info.metaKey) {
// While holding command key, start a brush session
this.startBrushSession(this.getPagePoint(info.point))
if (info.shiftKey) {
// While holding command and shift, select or deselect
// the shape, ignoring any group that may contain it
this.pointedId = info.target
if (this.selectedIds.includes(info.target)) {
// Deselect if selected
this.select(...this.selectedIds.filter((id) => id !== info.target))
} else {
// Otherwise, push select the shape
this.select(...this.selectedIds, info.target)
}
} else {
// While holding just command key, start a brush session
this.startBrushSession(this.getPagePoint(info.point))
}
return
}
if (!this.selectedIds.includes(info.target)) {
this.pointedId = info.target
// Set the pointed ID to the shape that was clicked.
// If we've clicked on a shape that is inside of a group,
// then select the group rather than the shape.
let shapeIdToSelect: string
const { parentId } = this.getShape(info.target)
// If the shape is not selected; then if the user is pressing shift,
// If the pointed shape is a child of the page, select the
// target shape and clear the selected group id.
if (parentId === this.currentPageId) {
shapeIdToSelect = info.target
this.selectedGroupId = undefined
} else {
// If the parent is some other group...
if (parentId === this.selectedGroupId) {
// If that group is the selected group, then select
// the target shape.
shapeIdToSelect = info.target
} else {
// Otherwise, select the group and clear the selected
// group id.
shapeIdToSelect = parentId
this.selectedGroupId = undefined
}
}
if (!this.selectedIds.includes(shapeIdToSelect)) {
// Set the pointed ID to the shape that was clicked.
this.pointedId = shapeIdToSelect
// If the shape is not selected: then if the user is pressing shift,
// add the shape to the current selection; otherwise, set the shape as
// the only selected shape.
this.select(...(info.shiftKey ? [...this.selectedIds, info.target] : [info.target]))
this.select(
...(info.shiftKey ? [...this.selectedIds, shapeIdToSelect] : [shapeIdToSelect])
)
}
this.setStatus(TLDrawStatus.PointingBounds)
@ -2281,7 +2317,30 @@ export class TLDrawState extends StateManager<Data> {
break
}
case TLDrawStatus.PointingBounds: {
this.pointedId = info.target
// If we've clicked on a shape that is inside of a group,
// then select the group rather than the shape.
if (info.metaKey && info.shiftKey) {
const targetId = this.pageState.hoveredId
if (targetId) {
this.pointedId = targetId
const shape = this.getShape(targetId)
if (this.selectedIds.includes(targetId)) {
this.select(...this.selectedIds.filter((id) => id !== targetId))
} else {
if (this.selectedIds.includes(shape.parentId)) {
this.select(targetId)
} else {
this.select(...this.selectedIds, targetId)
}
}
return
}
}
const { parentId } = this.getShape(info.target)
this.pointedId = parentId === this.currentPageId ? info.target : parentId
break
}
}
@ -2293,10 +2352,15 @@ export class TLDrawState extends StateManager<Data> {
onDoubleClickShape: TLPointerEventHandler = (info) => {
switch (this.appState.status.current) {
case TLDrawStatus.Idle:
case TLDrawStatus.PointingBounds: {
switch (this.appState.activeTool) {
case 'select': {
this.startTextSession(info.target)
const shape = this.getShape(info.target)
if (shape.parentId !== this.currentPageId) {
this.selectedGroupId = shape.parentId
}
this.select(info.target)
break
}
}

12
packages/tldraw/src/test/state-utils.tsx
View file

@ -17,6 +17,11 @@ export class TLStateUtils {
this.tlstate = tlstate
}
hoverShape = (id: string, options: PointerOptions = {}) => {
const { tlstate } = this
tlstate.onHoverShape(inputs.pointerDown(this.getPoint(options), id), {} as React.PointerEvent)
}
pointCanvas = (options: PointerOptions = {}) => {
this.tlstate.onPointCanvas(
inputs.pointerDown(this.getPoint(options), 'canvas'),
@ -62,7 +67,7 @@ export class TLStateUtils {
stopPointing = (target = 'canvas', options: PointerOptions = {}) => {
this.tlstate.onPointerUp(
inputs.pointerDown(this.getPoint(options), target),
inputs.pointerUp(this.getPoint(options), target),
{} as React.PointerEvent
)
return this
@ -75,6 +80,9 @@ export class TLStateUtils {
}
clickShape = (id: string, options: PointerOptions = {}) => {
if (this.tlstate.selectedIds.includes(id)) {
this.pointBounds(options)
}
this.pointShape(id, options)
this.stopPointing(id, options)
return this
@ -82,7 +90,7 @@ export class TLStateUtils {
clickBounds = (options: PointerOptions = {}) => {
this.pointBounds(options)
this.stopPointing()
this.stopPointing('bounds', options)
return this
}