diff --git a/newIDE/app/src/ObjectsList/EnumerateObjectFolderOrObject.js b/newIDE/app/src/ObjectsList/EnumerateObjectFolderOrObject.js index 6828737239..3181582f93 100644 --- a/newIDE/app/src/ObjectsList/EnumerateObjectFolderOrObject.js +++ b/newIDE/app/src/ObjectsList/EnumerateObjectFolderOrObject.js @@ -166,9 +166,10 @@ const itemMatchesSearchText = ( /** * Same case-insensitive substring match as TreeView's search filter. - * Folders that only appear as ancestors of a match, or that still contain a - * non-matching descendant, are not selectable: bulk ops would otherwise act - * on hidden children. + * Used by "Select all" while searching: folders that only appear as ancestors + * of a match, or that still contain a non-matching descendant, are skipped so + * that this implicit selection cannot act on hidden children. Explicitly + * clicked rows are never filtered: a visible row is always selectable. */ export const isSelectableWhileSearching = ( objectFolderOrObject: gdObjectFolderOrObject, @@ -203,25 +204,28 @@ export const enumerateAllChildrenInFolderMatchingSearch = ( }; /** - * After a Ctrl+click that removes a folder from the selection, also drop + * After a Ctrl+click that explicitly deselects a folder, also drop its * descendants that Select All (or a previous range) had added. Otherwise * nested objects stay selected inside a collapsed folder. + * + * `removedObjectFolderOrObjects` must be the items explicitly toggled off by + * the gesture (as reported by TreeView), NOT the difference between the + * previous and next selections: a plain click on the child of a selected + * folder also "removes" the folder from the selection, but the clicked child + * must obviously stay selected. */ export const dropDescendantsOfRemovedFolders = ( - previous: Array, + removedObjectFolderOrObjects: Array, next: Array ): Array => { - const nextPtrs = new Set(next.map(item => item.objectFolderOrObject.ptr)); - const removedFolders = previous.filter( - item => - item.objectFolderOrObject.isFolder() && - !nextPtrs.has(item.objectFolderOrObject.ptr) + const removedFolders = removedObjectFolderOrObjects.filter( + objectFolderOrObject => objectFolderOrObject.isFolder() ); if (removedFolders.length === 0) return next; return next.filter( item => !removedFolders.some(folder => - item.objectFolderOrObject.isADescendantOf(folder.objectFolderOrObject) + item.objectFolderOrObject.isADescendantOf(folder) ) ); }; diff --git a/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.js b/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.js index ce86488b5e..bb51314e25 100644 --- a/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.js +++ b/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.js @@ -14,6 +14,8 @@ import { import { t } from '@lingui/macro'; import { type I18n as I18nType } from '@lingui/core'; +const gd: libGDevelop = global.gd; + export const OBJECT_FOLDER_OR_OBJECTS_CLIPBOARD_KIND = 'ObjectFolderOrObjects'; // Kept for backward compatibility: read (but never written anymore) so that @@ -317,3 +319,70 @@ export const pasteObjectFolderOrObjectsFromClipboard = ({ return { createdObjects, topLevelObjectFolderOrObjects }; }; + +/** + * Paste the content of the clipboard at the given position and run the whole + * notification sequence shared by every paste entry point of the Objects + * panel: mark the project as modified, fire the object hooks (only when + * actual objects were created - pasting empty folders produces top-level + * items but no objects), and select the pasted items. + * Returns true when something was pasted. + */ +export const pasteObjectFolderOrObjectsAndNotify = ({ + project, + globalObjectsContainer, + objectsContainer, + global, + destinationFolder, + positionInFolder, + onObjectModified, + onObjectPasted, + onObjectCreated, + selectObjectFolderOrObjectsWithContext, +}: {| + project: gdProject, + globalObjectsContainer: gdObjectsContainer | null, + objectsContainer: gdObjectsContainer, + global: boolean, + destinationFolder: gdObjectFolderOrObject, + positionInFolder: number, + onObjectModified: (shouldForceUpdateList: boolean) => void, + onObjectPasted: ?(object: gdObject) => void, + onObjectCreated: ( + objects: Array, + isTheFirstOfItsTypeInProject: boolean + ) => void, + selectObjectFolderOrObjectsWithContext: ( + items: Array + ) => void, +|}): boolean => { + const isTheFirstOfItsTypeInProject = getObjectFolderOrObjectsClipboardObjectTypes().some( + objectType => !gd.UsedObjectTypeFinder.scanProject(project, objectType) + ); + + const pastedContent = pasteObjectFolderOrObjectsFromClipboard({ + project, + globalObjectsContainer, + objectsContainer, + global, + destinationFolder, + positionInFolder, + }); + if (!pastedContent) return false; + const { createdObjects, topLevelObjectFolderOrObjects } = pastedContent; + if (topLevelObjectFolderOrObjects.length === 0) return false; + + // onObjectModified(true) already calls forceUpdateList internally. + onObjectModified(true); + if (createdObjects.length > 0) { + if (onObjectPasted) onObjectPasted(createdObjects[0]); + onObjectCreated(createdObjects, isTheFirstOfItsTypeInProject); + } + selectObjectFolderOrObjectsWithContext( + topLevelObjectFolderOrObjects.map(objectFolderOrObject => ({ + objectFolderOrObject, + global, + })) + ); + return true; +}; diff --git a/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.spec.js b/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.spec.js index 504a213683..22a99c9366 100644 --- a/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.spec.js +++ b/newIDE/app/src/ObjectsList/ObjectFolderOrObjectsClipboard.spec.js @@ -175,20 +175,30 @@ describe('ObjectFolderOrObjectsClipboard', () => { const objectInFolder = subFolder.getChildAt(0); const sibling = rootFolder.getChildAt(0); - const previous = [ - { global: false, objectFolderOrObject: subFolder }, - { global: false, objectFolderOrObject: objectInFolder }, - { global: false, objectFolderOrObject: sibling }, - ]; const nextAfterDeselectingFolder = [ { global: false, objectFolderOrObject: objectInFolder }, { global: false, objectFolderOrObject: sibling }, ]; + // Ctrl+click deselected the folder: its still-selected child is dropped. expect( - dropDescendantsOfRemovedFolders(previous, nextAfterDeselectingFolder) + dropDescendantsOfRemovedFolders([subFolder], nextAfterDeselectingFolder) ).toEqual([{ global: false, objectFolderOrObject: sibling }]); + // A plain click on the child of a selected folder is not a toggle-off + // gesture (no removed items): the clicked child stays selected. + expect( + dropDescendantsOfRemovedFolders( + [], + [{ global: false, objectFolderOrObject: objectInFolder }] + ) + ).toEqual([{ global: false, objectFolderOrObject: objectInFolder }]); + + // Deselecting a plain object never drops anything. + expect( + dropDescendantsOfRemovedFolders([sibling], nextAfterDeselectingFolder) + ).toEqual(nextAfterDeselectingFolder); + project.delete(); }); diff --git a/newIDE/app/src/ObjectsList/ObjectFolderTreeViewItemContent.js b/newIDE/app/src/ObjectsList/ObjectFolderTreeViewItemContent.js index e8f5224fa5..35a7a817c4 100644 --- a/newIDE/app/src/ObjectsList/ObjectFolderTreeViewItemContent.js +++ b/newIDE/app/src/ObjectsList/ObjectFolderTreeViewItemContent.js @@ -16,8 +16,7 @@ import { writeObjectFolderOrObjectsToClipboard, hasObjectFolderOrObjectsInClipboard, getObjectFolderOrObjectsClipboardSummaryName, - getObjectFolderOrObjectsClipboardObjectTypes, - pasteObjectFolderOrObjectsFromClipboard, + pasteObjectFolderOrObjectsAndNotify, } from './ObjectFolderOrObjectsClipboard'; import { duplicateObjectFolderOrObjects } from './ObjectFolderOrObjectsDuplicate'; import { renderQuickCustomizationMenuItems } from '../QuickCustomization/QuickCustomizationMenuItems'; @@ -458,37 +457,22 @@ export class ObjectFolderTreeViewItemContent implements TreeViewItemContent { selectObjectFolderOrObjectsWithContext, } = this.props; - const isTheFirstOfItsTypeInProject = getObjectFolderOrObjectsClipboardObjectTypes().some( - objectType => !gd.UsedObjectTypeFinder.scanProject(project, objectType) - ); - - const pastedContent = pasteObjectFolderOrObjectsFromClipboard({ + const pasted = pasteObjectFolderOrObjectsAndNotify({ project, globalObjectsContainer, objectsContainer, global: this._isGlobal, destinationFolder: this.objectFolder, positionInFolder: this.objectFolder.getChildrenCount(), + onObjectModified, + onObjectPasted, + onObjectCreated, + selectObjectFolderOrObjectsWithContext, }); - if (!pastedContent) return; - const { createdObjects, topLevelObjectFolderOrObjects } = pastedContent; - if (topLevelObjectFolderOrObjects.length === 0) return; - - if (createdObjects.length > 0) { - onObjectCreated(createdObjects, isTheFirstOfItsTypeInProject); - } - onObjectModified(true); - if (onObjectPasted && createdObjects.length > 0) - onObjectPasted(createdObjects[0]); + if (!pasted) return; expandFolders([ { objectFolderOrObject: this.objectFolder, global: this._isGlobal }, ]); - selectObjectFolderOrObjectsWithContext( - topLevelObjectFolderOrObjects.map(pastedObjectFolderOrObject => ({ - objectFolderOrObject: pastedObjectFolderOrObject, - global: this._isGlobal, - })) - ); } duplicate(): void { diff --git a/newIDE/app/src/ObjectsList/ObjectTreeViewItemContent.js b/newIDE/app/src/ObjectsList/ObjectTreeViewItemContent.js index a025eff729..da05fe9d58 100644 --- a/newIDE/app/src/ObjectsList/ObjectTreeViewItemContent.js +++ b/newIDE/app/src/ObjectsList/ObjectTreeViewItemContent.js @@ -21,8 +21,7 @@ import { writeObjectFolderOrObjectsToClipboard, hasObjectFolderOrObjectsInClipboard, getObjectFolderOrObjectsClipboardSummaryName, - getObjectFolderOrObjectsClipboardObjectTypes, - pasteObjectFolderOrObjectsFromClipboard, + pasteObjectFolderOrObjectsAndNotify, } from './ObjectFolderOrObjectsClipboard'; import { type ObjectEditorTab } from '../ObjectEditor/ObjectEditorDialog'; import type { ObjectWithContext } from '../ObjectsList/EnumerateObjects'; @@ -293,9 +292,11 @@ export class ObjectTreeViewItemContent implements TreeViewItemContent { onClick(): void { // Selection itself is entirely handled by TreeView (single click, Ctrl/Cmd - // toggle, Shift range) through `onSelectItems`. Do not select here, as it - // would override that selection (in particular, it would collapse any - // multi-selection back to this single item). + // toggle, Shift range) through `onSelectItems` - including a plain click + // on the already-selected row, which is re-notified so the parent can + // bring the selection back to the front of the properties panel. Do not + // select here, as it would override that selection (in particular, it + // would collapse any multi-selection back to this single item). } rename(newName: string): void { @@ -647,45 +648,32 @@ export class ObjectTreeViewItemContent implements TreeViewItemContent { const objectFolderOrObject = this._getAliveObjectFolderOrObject(); if (!objectFolderOrObject) return; - const { project, globalObjectsContainer, objectsContainer } = this.props; const parentFolder = exceptionallyGuardAgainstDeadObject( objectFolderOrObject.getParent() ); if (!parentFolder) return; - const isTheFirstOfItsTypeInProject = getObjectFolderOrObjectsClipboardObjectTypes().some( - objectType => !gd.UsedObjectTypeFinder.scanProject(project, objectType) - ); - - const pastedContent = pasteObjectFolderOrObjectsFromClipboard({ + const { + project, + globalObjectsContainer, + objectsContainer, + onObjectPasted, + onObjectModified, + onObjectCreated, + selectObjectFolderOrObjectsWithContext, + } = this.props; + pasteObjectFolderOrObjectsAndNotify({ project, globalObjectsContainer, objectsContainer, global: this._isGlobal, destinationFolder: parentFolder, positionInFolder: parentFolder.getChildPosition(objectFolderOrObject) + 1, - }); - if (!pastedContent) return; - const { createdObjects, topLevelObjectFolderOrObjects } = pastedContent; - if (topLevelObjectFolderOrObjects.length === 0) return; - - const { - onObjectPasted, onObjectModified, + onObjectPasted, onObjectCreated, selectObjectFolderOrObjectsWithContext, - } = this.props; - onObjectModified(true); - if (createdObjects.length > 0) { - if (onObjectPasted) onObjectPasted(createdObjects[0]); - onObjectCreated(createdObjects, isTheFirstOfItsTypeInProject); - } - selectObjectFolderOrObjectsWithContext( - topLevelObjectFolderOrObjects.map(pastedObjectFolderOrObject => ({ - objectFolderOrObject: pastedObjectFolderOrObject, - global: this._isGlobal, - })) - ); + }); } duplicate(): void { diff --git a/newIDE/app/src/ObjectsList/UseBulkObjectOperations.js b/newIDE/app/src/ObjectsList/UseBulkObjectOperations.js index 17c69d405a..20672be03e 100644 --- a/newIDE/app/src/ObjectsList/UseBulkObjectOperations.js +++ b/newIDE/app/src/ObjectsList/UseBulkObjectOperations.js @@ -16,8 +16,7 @@ import { serializeObjectFolderOrObjectsForClipboard, writeObjectFolderOrObjectsToClipboard, hasObjectFolderOrObjectsInClipboard, - getObjectFolderOrObjectsClipboardObjectTypes, - pasteObjectFolderOrObjectsFromClipboard, + pasteObjectFolderOrObjectsAndNotify, getPasteMenuLabel, getUniqueFolderName, } from './ObjectFolderOrObjectsClipboard'; @@ -250,36 +249,18 @@ function useBulkObjectOperations({ ? objectFolderOrObject.getChildrenCount() : destinationFolder.getChildPosition(objectFolderOrObject) + 1; - const isTheFirstOfItsTypeInProject = getObjectFolderOrObjectsClipboardObjectTypes().some( - objectType => !gd.UsedObjectTypeFinder.scanProject(project, objectType) - ); - - const pastedContent = pasteObjectFolderOrObjectsFromClipboard({ + pasteObjectFolderOrObjectsAndNotify({ project, globalObjectsContainer, objectsContainer, global, destinationFolder, positionInFolder, + onObjectModified, + onObjectPasted, + onObjectCreated, + selectObjectFolderOrObjectsWithContext, }); - if (!pastedContent) return; - const { createdObjects, topLevelObjectFolderOrObjects } = pastedContent; - if (topLevelObjectFolderOrObjects.length === 0) return; - - // onObjectModified(true) already calls forceUpdateList internally. - onObjectModified(true); - // Only fire object hooks when actual objects were created; pasting - // empty folders produces topLevelObjectFolderOrObjects but no objects. - if (createdObjects.length > 0) { - if (onObjectPasted) onObjectPasted(createdObjects[0]); - onObjectCreated(createdObjects, isTheFirstOfItsTypeInProject); - } - selectObjectFolderOrObjectsWithContext( - topLevelObjectFolderOrObjects.map(pastedObjectFolderOrObject => ({ - objectFolderOrObject: pastedObjectFolderOrObject, - global, - })) - ); }, [ isListLocked, @@ -306,18 +287,14 @@ function useBulkObjectOperations({ ); if (candidates.length === 0) return; - // Filter out items that cannot be promoted, showing a warning per - // item rather than aborting the whole operation. + // Filter out items that cannot be promoted, then show a single warning + // listing all the name conflicts rather than one blocking dialog each. + const conflictingObjectNames = []; const objectItems = candidates.filter(item => { const objectName = item.objectFolderOrObject.getObject().getName(); if (!objectsContainer.hasObjectNamed(objectName)) return false; if (globalObjectsContainer.hasObjectNamed(objectName)) { - showWarningBox( - i18n._( - t`A global object with this name already exists. Please change the object name before setting it as a global object` - ), - { delayToNextTick: true } - ); + conflictingObjectNames.push(objectName); return false; } if (beforeSetAsGlobalObject && !beforeSetAsGlobalObject(objectName)) { @@ -325,6 +302,16 @@ function useBulkObjectOperations({ } return true; }); + if (conflictingObjectNames.length > 0) { + showWarningBox( + i18n._( + t`Global objects with these names already exist: ${conflictingObjectNames.join( + ', ' + )}. Please rename the objects before setting them as global objects.` + ), + { delayToNextTick: true } + ); + } if (objectItems.length === 0) return; const answer = Window.showConfirmDialog( diff --git a/newIDE/app/src/ObjectsList/index.js b/newIDE/app/src/ObjectsList/index.js index 65473414d0..463b1dd67f 100644 --- a/newIDE/app/src/ObjectsList/index.js +++ b/newIDE/app/src/ObjectsList/index.js @@ -30,14 +30,12 @@ import InAppTutorialContext from '../InAppTutorial/InAppTutorialContext'; import { getFoldersAscendanceWithoutRootFolder, getSelectionTopLevelItems, - isSelectableWhileSearching, dropDescendantsOfRemovedFolders, type ObjectFolderOrObjectWithContext, } from './EnumerateObjectFolderOrObject'; import { hasObjectFolderOrObjectsInClipboard, - getObjectFolderOrObjectsClipboardObjectTypes, - pasteObjectFolderOrObjectsFromClipboard, + pasteObjectFolderOrObjectsAndNotify, getPasteMenuLabel, } from './ObjectFolderOrObjectsClipboard'; import { useObjectsSelection } from './UseObjectsSelection'; @@ -1101,35 +1099,18 @@ const ObjectsList = React.forwardRef( (destinationFolder: gdObjectFolderOrObject, global: boolean) => { if (isListLocked) return; - const isTheFirstOfItsTypeInProject = getObjectFolderOrObjectsClipboardObjectTypes().some( - objectType => - !gd.UsedObjectTypeFinder.scanProject(project, objectType) - ); - - const pastedContent = pasteObjectFolderOrObjectsFromClipboard({ + pasteObjectFolderOrObjectsAndNotify({ project, globalObjectsContainer, objectsContainer, global, destinationFolder, positionInFolder: destinationFolder.getChildrenCount(), + onObjectModified, + onObjectPasted, + onObjectCreated, + selectObjectFolderOrObjectsWithContext, }); - if (!pastedContent) return; - const { createdObjects, topLevelObjectFolderOrObjects } = pastedContent; - if (topLevelObjectFolderOrObjects.length === 0) return; - - // onObjectModified(true) already calls forceUpdateList internally. - onObjectModified(true); - if (createdObjects.length > 0) { - if (onObjectPasted) onObjectPasted(createdObjects[0]); - onObjectCreated(createdObjects, isTheFirstOfItsTypeInProject); - } - selectObjectFolderOrObjectsWithContext( - topLevelObjectFolderOrObjects.map(pastedObjectFolderOrObject => ({ - objectFolderOrObject: pastedObjectFolderOrObject, - global, - })) - ); }, [ isListLocked, @@ -1964,7 +1945,7 @@ const ObjectsList = React.forwardRef( onClickItem={onClickItem} onCollapseItem={onCollapseItem} selectedItems={selectedItems} - onSelectItems={items => { + onSelectItems={(items, removedItems) => { if (!items || items.length === 0) { selectObjectFolderOrObjectsWithContext([]); return; @@ -1992,28 +1973,18 @@ const ObjectsList = React.forwardRef( const sameSectionItems = objectFolderOrObjectsToSelect.filter( item => item.global === global ); - let searchableItems = sameSectionItems; - if (searchText) { - const matchingItems = sameSectionItems.filter(item => - isSelectableWhileSearching( - item.objectFolderOrObject, - searchText - ) - ); - // A click/range that only hit context-only folders - // must not clear the current selection. - if ( - matchingItems.length === 0 && - sameSectionItems.length > 0 - ) { - return; - } - searchableItems = matchingItems; - } + // When a folder is explicitly deselected (Ctrl+click), + // also drop its descendants that a previous Select All + // or range had added to the selection. + const removedObjectFolderOrObjects = ( + removedItems || [] + ) + .map(item => item.content.getObjectFolderOrObject()) + .filter(Boolean); selectObjectFolderOrObjectsWithContext( dropDescendantsOfRemovedFolders( - selectedObjectFolderOrObjectsWithContext, - searchableItems + removedObjectFolderOrObjects, + sameSectionItems ) ); }} diff --git a/newIDE/app/src/SceneEditor/index.js b/newIDE/app/src/SceneEditor/index.js index 65157dac07..bbc8187f3a 100644 --- a/newIDE/app/src/SceneEditor/index.js +++ b/newIDE/app/src/SceneEditor/index.js @@ -1677,19 +1677,23 @@ export default class SceneEditor extends React.Component { if (objects.length === 0) { return; } - const object = objects[0]; - const infoBarDetails = onObjectAdded({ - object, - layersContainer: this.props.layersContainer, - globalObjectsContainer: this.props.globalObjectsContainer, - objectsContainer: this.props.objectsContainer, - }); - if (infoBarDetails) { - this.setState({ - additionalWorkInfoBar: infoBarDetails, - showAdditionalWorkInfoBar: true, + // Run the per-object-type additional work for every created object (for + // instance, a lighting layer is created for a light object): bulk paste + // and duplicate can create several objects at once. + objects.forEach(object => { + const infoBarDetails = onObjectAdded({ + object, + layersContainer: this.props.layersContainer, + globalObjectsContainer: this.props.globalObjectsContainer, + objectsContainer: this.props.objectsContainer, }); - } + if (infoBarDetails) { + this.setState({ + additionalWorkInfoBar: infoBarDetails, + showAdditionalWorkInfoBar: true, + }); + } + }); if (this.props.unsavedChanges) this.props.unsavedChanges.triggerUnsavedChanges(); diff --git a/newIDE/app/src/UI/TreeView/UseTreeViewSelection.js b/newIDE/app/src/UI/TreeView/UseTreeViewSelection.js index 2741bd5f8e..c4a99db419 100644 --- a/newIDE/app/src/UI/TreeView/UseTreeViewSelection.js +++ b/newIDE/app/src/UI/TreeView/UseTreeViewSelection.js @@ -31,6 +31,10 @@ type ComputeArgs = {| export type ComputeTreeViewSelectionResult = {| newSelection: Array | null, + // Items explicitly deselected by this gesture (Ctrl/Cmd+click toggle-off). + // Callers can use this to also drop related items (e.g. the descendants of + // a deselected folder) - which cannot be inferred from the selections alone. + removedItems: Array, selectionAnchorId: ?string, shiftSelectionBase: Array, navigationFocusId: string, @@ -61,6 +65,7 @@ export function computeTreeViewSelection({ const newSelection = [node.item]; return { newSelection, + removedItems: [], selectionAnchorId: node.id, shiftSelectionBase: newSelection, navigationFocusId: node.id, @@ -82,6 +87,7 @@ export function computeTreeViewSelection({ ); return { newSelection: [...baseItemsOutsideRange, ...rangeItems], + removedItems: [], selectionAnchorId, shiftSelectionBase: [...shiftSelectionBase], navigationFocusId: node.id, @@ -89,23 +95,20 @@ export function computeTreeViewSelection({ } let newSelection; + let removedItems: Array = []; if (multiSelect) { const selectedItemIds = selectedItems.map(getItemId); if (node.selected) { if (exclusive) { - if (selectedItems.length === 1) { - return { - newSelection: null, - selectionAnchorId, - shiftSelectionBase: [...shiftSelectionBase], - navigationFocusId: node.id, - }; - } + // Also when the node is already the only selected item: the parent + // is notified again so it can, for instance, bring the selection + // back to the front of a properties panel. newSelection = [node.item]; } else { newSelection = selectedItems.filter( (item, index) => selectedItemIds[index] !== node.id ); + removedItems = [node.item]; } } else { if (exclusive) newSelection = [node.item]; @@ -115,6 +118,7 @@ export function computeTreeViewSelection({ if (node.selected && selectedItems.length === 1) { return { newSelection: null, + removedItems: [], selectionAnchorId, shiftSelectionBase: [...shiftSelectionBase], navigationFocusId: node.id, @@ -125,6 +129,7 @@ export function computeTreeViewSelection({ return { newSelection, + removedItems, selectionAnchorId: node.id, shiftSelectionBase: newSelection, navigationFocusId: node.id, @@ -154,7 +159,7 @@ function useTreeViewSelection({ multiSelect: boolean, selectedItems: $ReadOnlyArray, flattenedData: $ReadOnlyArray>, - onSelectItems: (items: Array) => void, + onSelectItems: (items: Array, removedItems?: Array) => void, getItemId: (item: Item) => string, |}): {| onSelect: (SelectArgs) => void, @@ -163,19 +168,40 @@ function useTreeViewSelection({ const selectionAnchorIdRef = React.useRef(null); const shiftSelectionBaseRef = React.useRef>([]); const navigationFocusIdRef = React.useRef(null); + // Set right before `onSelect` notifies the parent, and consumed by the + // effect below to tell user gestures apart from programmatic selection + // changes (paste, duplicate, "move to folder"...). + const selectionComesFromUserGestureRef = React.useRef(false); - // When the selection is cleared externally (e.g. Deselect All), stale - // range-select refs must be reset so a later Shift+click doesn't extend - // from a ghost anchor. Keyboard focus is kept so arrows continue from the - // last interacted row. React.useEffect( () => { + const comesFromUserGesture = selectionComesFromUserGestureRef.current; + selectionComesFromUserGestureRef.current = false; if (selectedItems.length === 0) { + // When the selection is cleared (e.g. Deselect All), stale + // range-select refs must be reset so a later Shift+click doesn't + // extend from a ghost anchor. Keyboard focus is kept so arrows + // continue from the last interacted row. + selectionAnchorIdRef.current = null; + shiftSelectionBaseRef.current = []; + return; + } + if (comesFromUserGesture) return; + // The selection was changed programmatically (paste, duplicate, "move + // to folder"...) and no longer contains the last interacted row: + // keyboard navigation and Shift ranges must restart from the new + // selection, not from that now unrelated row. + const selectedIds = new Set(selectedItems.map(getItemId)); + if ( + navigationFocusIdRef.current && + !selectedIds.has(navigationFocusIdRef.current) + ) { + navigationFocusIdRef.current = null; selectionAnchorIdRef.current = null; shiftSelectionBaseRef.current = []; } }, - [selectedItems] + [selectedItems, getItemId] ); const onSelect = React.useCallback( @@ -194,7 +220,10 @@ function useTreeViewSelection({ navigationFocusIdRef.current = result.navigationFocusId; selectionAnchorIdRef.current = result.selectionAnchorId; shiftSelectionBaseRef.current = result.shiftSelectionBase; - if (result.newSelection !== null) onSelectItems(result.newSelection); + if (result.newSelection !== null) { + selectionComesFromUserGestureRef.current = true; + onSelectItems(result.newSelection, result.removedItems); + } }, [multiSelect, selectedItems, flattenedData, onSelectItems, getItemId] ); diff --git a/newIDE/app/src/UI/TreeView/UseTreeViewSelection.spec.js b/newIDE/app/src/UI/TreeView/UseTreeViewSelection.spec.js index 0491053bcf..ed64e808cc 100644 --- a/newIDE/app/src/UI/TreeView/UseTreeViewSelection.spec.js +++ b/newIDE/app/src/UI/TreeView/UseTreeViewSelection.spec.js @@ -69,7 +69,7 @@ describe('computeTreeViewSelection', () => { expect(result.navigationFocusId).toBe('d'); }); - test('ctrl+click on a selected item toggles it off and keeps keyboard focus on it', () => { + test('ctrl+click on a selected item toggles it off, reports it as removed and keeps keyboard focus on it', () => { const result = computeTreeViewSelection({ multiSelect: true, selectedItems: [item('a'), item('d')], @@ -82,6 +82,7 @@ describe('computeTreeViewSelection', () => { expect(result.newSelection && result.newSelection.map(getItemId)).toEqual([ 'a', ]); + expect(result.removedItems.map(getItemId)).toEqual(['d']); expect(result.navigationFocusId).toBe('d'); }); @@ -157,7 +158,9 @@ describe('computeTreeViewSelection', () => { ]); }); - test('exclusive click on the only selected item is a no-op for the selection', () => { + test('exclusive click on the only selected item keeps the selection but still notifies', () => { + // The parent must be notified again so it can, for instance, bring the + // selection back to the front of a properties panel. const result = computeTreeViewSelection({ multiSelect: true, selectedItems: [item('b')], @@ -168,7 +171,30 @@ describe('computeTreeViewSelection', () => { node: { ...node('b'), selected: true }, exclusive: true, }); - expect(result.newSelection).toBe(null); + expect(result.newSelection && result.newSelection.map(getItemId)).toEqual([ + 'b', + ]); + expect(result.removedItems).toEqual([]); expect(result.navigationFocusId).toBe('b'); }); + + test('exclusive click collapses a multi-selection to the clicked item without reporting removals', () => { + // Collapsing a multi-selection with a plain click is not a toggle-off + // gesture: the other items must not be reported as removed, otherwise + // clicking the child of a selected folder would drop the child too. + const result = computeTreeViewSelection({ + multiSelect: true, + selectedItems: [item('a'), item('b'), item('c')], + flattenedData, + getItemId, + selectionAnchorId: 'a', + shiftSelectionBase: [item('a'), item('b'), item('c')], + node: { ...node('b'), selected: true }, + exclusive: true, + }); + expect(result.newSelection && result.newSelection.map(getItemId)).toEqual([ + 'b', + ]); + expect(result.removedItems).toEqual([]); + }); }); diff --git a/newIDE/app/src/UI/TreeView/index.js b/newIDE/app/src/UI/TreeView/index.js index 3abdbbbfe1..07c83c353f 100644 --- a/newIDE/app/src/UI/TreeView/index.js +++ b/newIDE/app/src/UI/TreeView/index.js @@ -180,7 +180,9 @@ type Props = {| searchText?: string, selectedItems: $ReadOnlyArray, onClickItem?: Item => void, - onSelectItems: (Item[]) => void, + // `removedItems` lists the items explicitly deselected by the gesture + // (Ctrl/Cmd+click toggle-off), so callers can drop related items too. + onSelectItems: (Item[], removedItems?: Array) => void, multiSelect: boolean, onRenameItem: (Item, newName: string) => void, onMoveSelectionToItem: ( diff --git a/newIDE/app/src/stories/componentStories/LayoutEditor/ObjectsList.stories.js b/newIDE/app/src/stories/componentStories/LayoutEditor/ObjectsList.stories.js index ec39632f71..381a64b7ac 100644 --- a/newIDE/app/src/stories/componentStories/LayoutEditor/ObjectsList.stories.js +++ b/newIDE/app/src/stories/componentStories/LayoutEditor/ObjectsList.stories.js @@ -49,8 +49,9 @@ const AssetStoreProviders = ({ children }: {| children: React.Node |}) => ( ); -// Shared props used by all stories. -const sharedProps = { +// Shared props used by all stories. Must be built at render time (not at +// module load): `testProject` is only usable once GDevelop.js is initialized. +const getSharedProps = () => ({ getThumbnail: () => 'res/unknown32.png', project: testProject.project, layout: testProject.testLayout, @@ -81,14 +82,14 @@ const sharedProps = { hotReloadPreviewButtonProps: fakeHotReloadPreviewButtonProps, onWillInstallExtension: action('extension will be installed'), onExtensionInstalled: action('onExtensionInstalled'), -}; +}); export const Default = (): React.Node => (
{}} isListLocked={false} @@ -119,7 +120,7 @@ export const WithMultiSelection = (): React.Node => {
{ ); }; +// --------------------------------------------------------------------------- +// A stateful story for the visual tests (see newIDE/visual-tests): the +// selection is managed like SceneEditor does, on a small container with a +// known content, and exposed on `window.objectsListManipulations` so a test +// can check that what the list displays is what the app holds. +// --------------------------------------------------------------------------- + +let playgroundContainers: {| + objectsContainer: gdObjectsContainer, + globalObjectsContainer: gdObjectsContainer, +|} | null = null; +const getOrCreatePlaygroundContainers = () => { + if (playgroundContainers) return playgroundContainers; + const gd = global.gd; + const project = testProject.project; + const objectsContainer = new gd.ObjectsContainer(gd.ObjectsContainer.Unknown); + const globalObjectsContainer = new gd.ObjectsContainer( + gd.ObjectsContainer.Unknown + ); + const rootFolder = objectsContainer.getRootFolder(); + objectsContainer.insertNewObjectInFolder( + project, + 'Sprite', + 'Player', + rootFolder, + 0 + ); + objectsContainer.insertNewObjectInFolder( + project, + 'Sprite', + 'Enemy1', + rootFolder, + 1 + ); + const enemiesFolder = rootFolder.insertNewFolder('Enemies', 2); + objectsContainer.insertNewObjectInFolder( + project, + 'Sprite', + 'EnemyBoss', + enemiesFolder, + 0 + ); + objectsContainer.insertNewObjectInFolder( + project, + 'Sprite', + 'Wall', + enemiesFolder, + 1 + ); + objectsContainer.insertNewObjectInFolder( + project, + 'Sprite', + 'Background', + rootFolder, + 3 + ); + globalObjectsContainer.insertNewObjectInFolder( + project, + 'Sprite', + 'GlobalPlayer', + globalObjectsContainer.getRootFolder(), + 0 + ); + playgroundContainers = { objectsContainer, globalObjectsContainer }; + return playgroundContainers; +}; + +const readAllNamesInFolder = ( + folder: gdObjectFolderOrObject +): Array => { + const names = []; + for (let i = 0; i < folder.getChildrenCount(); i++) { + const child = folder.getChildAt(i); + if (child.isFolder()) { + names.push(child.getFolderName(), ...readAllNamesInFolder(child)); + } else { + names.push(child.getObject().getName()); + } + } + return names; +}; + +type SelectedItems = Array<{| + objectFolderOrObject: gdObjectFolderOrObject, + global: boolean, +|}>; + +const SelectionPlaygroundStory = () => { + const { + objectsContainer, + globalObjectsContainer, + } = getOrCreatePlaygroundContainers(); + const [selection, setSelection] = React.useState([]); + const selectionRef = React.useRef(selection); + const selectionNotificationsCountRef = React.useRef(0); + + const onObjectFolderOrObjectsWithContextSelected = React.useCallback( + (items: SelectedItems = []) => { + selectionNotificationsCountRef.current++; + // Like SceneEditor: the selection stays within a single section. + const sameSectionItems: SelectedItems = + items.length === 0 + ? [] + : items.filter(item => item.global === items[0].global); + selectionRef.current = sameSectionItems; + setSelection(sameSectionItems); + }, + [] + ); + + React.useEffect( + () => { + window.objectsListManipulations = { + readState: () => ({ + sceneNames: readAllNamesInFolder(objectsContainer.getRootFolder()), + globalNames: readAllNamesInFolder( + globalObjectsContainer.getRootFolder() + ), + selectionNames: selectionRef.current.map(item => + item.objectFolderOrObject.isFolder() + ? item.objectFolderOrObject.getFolderName() + : item.objectFolderOrObject.getObject().getName() + ), + selectionNotificationsCount: selectionNotificationsCountRef.current, + }), + }; + }, + [objectsContainer, globalObjectsContainer] + ); + + return ( + + +
+ +
+ + + ); +}; + +export const SelectionPlayground = (): React.Node => ( + +); + export const WithSerializedObjectView = (): React.Node => (
{}} isListLocked={false} @@ -156,7 +311,7 @@ export const Locked = (): React.Node => (
{}} isListLocked={true} diff --git a/newIDE/visual-tests/helpers/ObjectsList.js b/newIDE/visual-tests/helpers/ObjectsList.js index fd2d812c8b..1687cd69c0 100644 --- a/newIDE/visual-tests/helpers/ObjectsList.js +++ b/newIDE/visual-tests/helpers/ObjectsList.js @@ -1,14 +1,276 @@ // @ts-check /** - * The objects list of a scene, and the dialog editing one object. Used by the - * tests running on the real app to reach the editor they want to manipulate. + * The objects list of a scene: how to find its rows, read what it displays + * (including which rows are shown as selected) and manipulate the selection. + * Used by the Storybook tests of the multi-selection, and by the tests running + * on the real app to reach the editor they want to manipulate. */ -const { click, exists, waitFor, wait } = require('../lib/PageDriver'); +const { + wait, + click, + exists, + waitFor, + rectOf, + typeInInput, +} = require('../lib/PageDriver'); const OBJECT_EDITOR_DIALOG = { selector: '#object-editor-dialog' }; +// ------------------------------------------------------- installed in the page + +/** + * Reads the rows of the objects list from their data attributes, and what the + * story holds (its objects and its selection) from + * `window.objectsListManipulations` when it exposes it. + * + * Targets of this editor are `{ objectsList: { name, part } }` with part one + * of: row (default), chevron (the expand/collapse arrow of a folder). + */ +const installObjectsListPageHelpers = function() { + const { addTargetResolver } = window.gdVisualTests; + + const getRows = () => + Array.from( + document.querySelectorAll('[data-object-name], [data-folder-name]') + ).map(element => ({ + element, + name: + element.getAttribute('data-object-name') || + element.getAttribute('data-folder-name'), + isFolder: element.hasAttribute('data-folder-name'), + isGlobal: element.getAttribute('data-global') === 'true', + selected: element.getAttribute('aria-selected') === 'true', + })); + + addTargetResolver('objectsList', target => { + const row = getRows().find(row => row.name === target.name); + if (!row) return null; + // The expand/collapse arrow of a folder is its first (icon) button. + if (target.part === 'chevron') return row.element.querySelector('button'); + return row.element; + }); + addTargetResolver( + 'objectsListSearch', + () => document.querySelector('input[placeholder]') || null + ); + + const readState = () => + window.objectsListManipulations + ? window.objectsListManipulations.readState() + : null; + + const describe = () => { + const state = readState(); + return { + rows: getRows().map(({ name, isFolder, isGlobal, selected }) => ({ + name, + isFolder, + isGlobal, + selected, + })), + selectionNames: state ? state.selectionNames : null, + selectionNotificationsCount: state + ? state.selectionNotificationsCount + : null, + ...window.gdVisualTests.describeOverlays(), + }; + }; + + /** Compare what the list displays with what the app holds. */ + const check = () => { + const described = describe(); + const problems = []; + if (!described.rows.length) { + problems.push('the objects list is not displayed anymore'); + return { problems, described }; + } + const state = readState(); + if (!state) return { problems, described }; + + const allNames = [...state.sceneNames, ...state.globalNames]; + const displayedSelection = []; + described.rows.forEach(row => { + if (!allNames.includes(row.name)) + problems.push( + `the row "${row.name}" is displayed but no object or folder ` + + `has this name` + ); + if (row.selected) displayedSelection.push(row.name); + }); + // The rows shown as selected must be the selection the app holds. Only + // displayed rows can be compared: a selected row inside a collapsed + // folder is simply not displayed. + displayedSelection.forEach(name => { + if (!state.selectionNames.includes(name)) + problems.push( + `the row "${name}" is shown as selected but is not in the selection` + ); + }); + state.selectionNames.forEach(name => { + const row = described.rows.find(row => row.name === name); + if (row && !row.selected) + problems.push( + `"${name}" is in the selection but its row is not shown as selected` + ); + }); + return { problems, described }; + }; + + window.gdVisualTests.objectsList = { describe, check }; +}; + +// -------------------------------------------------------------- manipulations + +const rowTarget = (name, part) => ({ objectsList: { name, part } }); + +/** A real mouse click (with an optionally held key), so that the row handlers + * see the modifiers and the focus moves into the tree (for the keyboard). */ +const realClick = async (page, target, modifierKey) => { + const rect = await rectOf(page, target, true); + if (!rect) return false; + if (modifierKey) await page.keyboard.down(modifierKey); + await page.mouse.click(rect.x, rect.y); + if (modifierKey) await page.keyboard.up(modifierKey); + return true; +}; + +const focusTree = page => + page.evaluate(() => { + const tree = document.querySelector('#objects-list [tabindex="0"]'); + if (!tree) return false; + tree.focus(); + return true; + }); + +/** The precise selection (and notification) a manipulation must end on. */ +const expectationFromArgs = (state, args) => { + if (!args.expectedSelection) return null; + const expectation = { selectedNames: args.expectedSelection }; + if (args.expectNewNotification) + expectation.minSelectionNotificationsCount = + state.selectionNotificationsCount + 1; + return expectation; +}; + +const objectsListActions = { + clickRow: { + describe: args => `click the row "${args.name}"`, + expect: expectationFromArgs, + run: async (page, args) => { + if (!(await realClick(page, rowTarget(args.name)))) + return `the row "${args.name}" is not visible`; + await wait(400); + }, + }, + + ctrlClickRow: { + describe: args => `Ctrl+click the row "${args.name}"`, + expect: expectationFromArgs, + run: async (page, args) => { + if (!(await realClick(page, rowTarget(args.name), 'Control'))) + return `the row "${args.name}" is not visible`; + await wait(400); + }, + }, + + toggleFolder: { + describe: args => `expand or collapse the folder "${args.name}"`, + run: async (page, args) => { + if (!(await click(page, rowTarget(args.name, 'chevron')))) + return `the folder "${args.name}" has no expand arrow`; + await wait(400); + }, + }, + + pressKey: { + describe: args => `press ${args.key} in the list`, + expect: expectationFromArgs, + run: async (page, args) => { + if (!(await focusTree(page))) return 'the tree cannot be focused'; + await page.keyboard.press(args.key); + await wait(400); + }, + }, + + selectAll: { + describe: () => 'press Ctrl+A in the list', + expect: expectationFromArgs, + run: async page => { + if (!(await focusTree(page))) return 'the tree cannot be focused'; + await page.keyboard.down('Control'); + await page.keyboard.press('a'); + await page.keyboard.up('Control'); + await wait(400); + }, + }, + + setSearchText: { + describe: args => `search for "${args.text}"`, + expect: (state, args) => + args.expectedVisibleRows ? { visibleRowNames: args.expectedVisibleRows } : null, + run: async (page, args) => { + if (!(await typeInInput(page, { objectsListSearch: {} }, args.text))) + return 'there is no search field'; + await wait(600); + }, + }, +}; + +// ----------------------------------------------- what the runner checks with + +const describe = page => + page.evaluate(() => window.gdVisualTests.objectsList.describe()); + +const describeEffect = (snapshotBefore, snapshotAfter) => { + const changes = []; + const selectionOf = snapshot => (snapshot.selectionNames || []).join(', '); + if (selectionOf(snapshotBefore) !== selectionOf(snapshotAfter)) + changes.push(`selection: [${selectionOf(snapshotAfter)}]`); + if (snapshotBefore.rows.length !== snapshotAfter.rows.length) + changes.push( + `${snapshotBefore.rows.length} → ${snapshotAfter.rows.length} rows` + ); + if ( + snapshotBefore.selectionNotificationsCount !== + snapshotAfter.selectionNotificationsCount + ) + changes.push('the app was notified'); + return changes.length ? changes.join(', ') : 'nothing changed'; +}; + +/** Check the precise outcome a manipulation declared with `expect`. */ +const checkExpectation = (expectation, snapshotAfter) => { + const problems = []; + if ( + expectation.selectedNames && + snapshotAfter.selectionNames && + expectation.selectedNames.join('|') !== snapshotAfter.selectionNames.join('|') + ) + problems.push( + `the selection is [${snapshotAfter.selectionNames.join(', ')}] but ` + + `[${expectation.selectedNames.join(', ')}] was expected` + ); + if ( + expectation.minSelectionNotificationsCount !== undefined && + snapshotAfter.selectionNotificationsCount < + expectation.minSelectionNotificationsCount + ) + problems.push('the app was not notified of the selection again'); + if (expectation.visibleRowNames) { + const visible = snapshotAfter.rows.map(row => row.name); + if (expectation.visibleRowNames.join('|') !== visible.join('|')) + problems.push( + `the rows displayed are [${visible.join(', ')}] but ` + + `[${expectation.visibleRowNames.join(', ')}] were expected` + ); + } + return problems; +}; + +// ------------------------------------- used by the tests on the real app too + /** Wait for the scene to be opened and return the names of its objects. */ const waitForTheScene = async (page, timeoutInMs = 180000) => { await page.waitForSelector('[data-object-name]', { timeout: timeoutInMs }); @@ -49,7 +311,19 @@ module.exports = { paths: [ 'newIDE/app/src/ObjectsList/', 'newIDE/app/src/ObjectEditor/ObjectEditorDialog.js', + 'newIDE/app/src/UI/TreeView/', + 'newIDE/app/src/stories/componentStories/LayoutEditor/ObjectsList.stories.js', ], + installPageHelpers: installObjectsListPageHelpers, + actions: objectsListActions, + describe, + check: page => page.evaluate(() => window.gdVisualTests.objectsList.check()), + describeEffect, + checkExpectation, + summarize: async page => { + const described = await describe(page); + return `${described.rows.length} rows displayed`; + }, OBJECT_EDITOR_DIALOG, waitForTheScene, openObjectEditor, diff --git a/newIDE/visual-tests/storybook-tests/objects-list.js b/newIDE/visual-tests/storybook-tests/objects-list.js new file mode 100644 index 0000000000..ea4bb15ba2 --- /dev/null +++ b/newIDE/visual-tests/storybook-tests/objects-list.js @@ -0,0 +1,102 @@ +// @ts-check + +/** + * Multi-selection in the objects list, on a story holding the selection like + * SceneEditor does. The story contains, on top of a "GlobalPlayer" global + * object: Player, Enemy1, a folder "Enemies" (containing EnemyBoss and Wall) + * and Background. + * + * After every manipulation, the helper checks that the rows shown as selected + * are exactly the selection the app holds. + */ + +const objectsList = require('../helpers/ObjectsList'); + +const STORY = 'layouteditor-objectslist--selection-playground'; + +module.exports = [ + { + name: 'objects-list/click-inside-a-selected-folder', + description: + 'Clicking (or arrow-navigating to) the child of a selected folder ' + + 'selects the child - it must not clear the selection - and re-clicking ' + + 'the selected row notifies the app again (for the properties panel).', + helper: objectsList, + story: STORY, + steps: [ + ['clickRow', { name: 'Enemies', expectedSelection: ['Enemies'] }], + ['toggleFolder', { name: 'Enemies' }], + ['clickRow', { name: 'EnemyBoss', expectedSelection: ['EnemyBoss'] }], + ['clickRow', { name: 'Enemies', expectedSelection: ['Enemies'] }], + ['pressKey', { key: 'ArrowDown', expectedSelection: ['EnemyBoss'] }], + [ + 'clickRow', + { + name: 'EnemyBoss', + expectedSelection: ['EnemyBoss'], + expectNewNotification: true, + }, + ], + ], + }, + { + name: 'objects-list/ctrl-click-multi-selection', + description: + 'Ctrl+click toggles rows in and out of the selection; deselecting a ' + + 'folder also drops its selected descendants, and the selection stays ' + + 'within one section (scene or global objects).', + helper: objectsList, + story: STORY, + steps: [ + ['clickRow', { name: 'Player', expectedSelection: ['Player'] }], + [ + 'ctrlClickRow', + { name: 'Enemy1', expectedSelection: ['Player', 'Enemy1'] }, + ], + ['toggleFolder', { name: 'Enemies' }], + [ + 'ctrlClickRow', + { name: 'Enemies', expectedSelection: ['Player', 'Enemy1', 'Enemies'] }, + ], + [ + 'ctrlClickRow', + { + name: 'EnemyBoss', + expectedSelection: ['Player', 'Enemy1', 'Enemies', 'EnemyBoss'], + }, + ], + // Deselecting the folder must also deselect EnemyBoss inside it. + [ + 'ctrlClickRow', + { name: 'Enemies', expectedSelection: ['Player', 'Enemy1'] }, + ], + ['ctrlClickRow', { name: 'Enemy1', expectedSelection: ['Player'] }], + // A global object cannot join a scene objects selection. + ['ctrlClickRow', { name: 'GlobalPlayer', expectedSelection: ['Player'] }], + ], + }, + { + name: 'objects-list/selection-while-searching', + description: + 'Every visible row stays selectable while searching, and Ctrl+A only ' + + 'selects the items matching the search.', + helper: objectsList, + story: STORY, + steps: [ + [ + 'setSearchText', + { + text: 'enemy', + expectedVisibleRows: ['Enemy1', 'Enemies', 'EnemyBoss'], + }, + ], + // "Enemies" matches but contains the non-matching "Wall": its visible + // row must still be selectable. + ['clickRow', { name: 'Enemies', expectedSelection: ['Enemies'] }], + ['clickRow', { name: 'EnemyBoss', expectedSelection: ['EnemyBoss'] }], + // Select All is implicit: it must skip "Enemies" (its hidden child + // "Wall" does not match) and anything not displayed. + ['selectAll', { expectedSelection: ['Enemy1', 'EnemyBoss'] }], + ], + }, +];