New variables list improvements

* Fix drop down styling on light theme for selected rows
* Make sure the toolbar is fixed on every dialog the dialog is used in
* When adding a variable, scroll to the new row and focus name
* When searching, display children of collections to enable the user path "Search by collection name > Find child of interest > change child value".
  * If one changes a variable name that justifies its display during the search, the row does not disappear as soon as one is done with editing the name
This commit is contained in:
AlexandreS
2022-05-20 15:33:09 +02:00
committed by GitHub
parent e75d49ea51
commit eeea1b99f4
12 changed files with 395 additions and 64 deletions
+14
View File
@@ -15,6 +15,20 @@
"disableOptimisticBPs": true,
"cwd": "${workspaceFolder}/GDevelop.js"
},
{
"type": "node",
"request": "launch",
"name": "newIDE/app Jest tests (current file)",
"program": "${workspaceFolder}/newIDE/app/node_modules/.bin/react-scripts",
"args": [
"test", "--env=node",
"${fileBasenameNoExtension}"
],
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen",
"disableOptimisticBPs": true,
"cwd": "${workspaceFolder}/newIDE/app"
},
{
"type": "node",
"request": "launch",
@@ -8,7 +8,7 @@ import {
type ParameterFieldProps,
type ParameterFieldInterface,
} from './ParameterFieldCommons';
import SelectField from '../../UI/SelectField';
import SelectField, { type SelectFieldInterface } from '../../UI/SelectField';
import SelectOption from '../../UI/SelectOption';
import { TextFieldWithButtonLayout } from '../../UI/Layout';
import RaisedButtonWithSplitMenu from '../../UI/RaisedButtonWithSplitMenu';
@@ -57,9 +57,10 @@ export default React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
const isOnline = useOnlineStatus();
const leaderboards = useFetchLeaderboards();
const [isAdminOpen, setIsAdminOpen] = React.useState(false);
const inputFieldRef = React.useRef<?(GenericExpressionField | SelectField)>(
null
);
const inputFieldRef = React.useRef<?(
| GenericExpressionField
| SelectFieldInterface
)>(null);
React.useImperativeHandle(ref, () => ({
focus: () => {
if (inputFieldRef.current) {
@@ -4,11 +4,11 @@ import { t } from '@lingui/macro';
import { type ParameterFieldProps } from './ParameterFieldCommons';
import { type ParameterInlineRendererProps } from './ParameterInlineRenderer.flow';
import React, { Component } from 'react';
import SelectField from '../../UI/SelectField';
import SelectField, { type SelectFieldInterface } from '../../UI/SelectField';
import SelectOption from '../../UI/SelectOption';
export default class MouseField extends Component<ParameterFieldProps> {
_field: ?SelectField;
_field: ?SelectFieldInterface;
focus() {
if (this._field) this._field.focus();
@@ -4,7 +4,7 @@ import { t } from '@lingui/macro';
import React, { Component } from 'react';
import { type ParameterInlineRendererProps } from './ParameterInlineRenderer.flow';
import { type ParameterFieldProps } from './ParameterFieldCommons';
import SelectField from '../../UI/SelectField';
import SelectField, { type SelectFieldInterface } from '../../UI/SelectField';
import SelectOption from '../../UI/SelectOption';
const operatorLabels = {
@@ -23,7 +23,7 @@ const mapTypeToOperators = {
};
export default class OperatorField extends Component<ParameterFieldProps> {
_field: ?SelectField;
_field: ?SelectFieldInterface;
focus() {
if (this._field && this._field.focus) this._field.focus();
}
@@ -4,7 +4,7 @@ import { t } from '@lingui/macro';
import React, { Component } from 'react';
import { type ParameterInlineRendererProps } from './ParameterInlineRenderer.flow';
import { type ParameterFieldProps } from './ParameterFieldCommons';
import SelectField from '../../UI/SelectField';
import SelectField, { type SelectFieldInterface } from '../../UI/SelectField';
import SelectOption from '../../UI/SelectOption';
const operatorLabels = {
@@ -25,7 +25,7 @@ const mapTypeToOperators: { [string]: Array<string> } = {
};
export default class RelationalOperatorField extends Component<ParameterFieldProps> {
_field: ?SelectField;
_field: ?SelectFieldInterface;
focus() {
if (this._field && this._field.focus) this._field.focus();
}
+15 -9
View File
@@ -41,6 +41,8 @@ type Props = {|
hintText?: MessageDescriptor,
|};
export type SelectFieldInterface = {| focus: () => void |};
const INVALID_VALUE = '';
const stopPropagation = event => event.stopPropagation();
@@ -48,15 +50,18 @@ const stopPropagation = event => event.stopPropagation();
* A select field based on Material-UI select field.
* To be used with `SelectOption`.
*/
export default class SelectField extends React.Component<Props, {||}> {
_input = React.createRef<HTMLInputElement>();
const SelectField = React.forwardRef<Props, SelectFieldInterface>(
(props, ref) => {
const inputRef = React.useRef<?HTMLInputElement>(null);
focus() {
if (this._input.current) this._input.current.focus();
}
const focus = () => {
if (inputRef.current) inputRef.current.focus();
};
React.useImperativeHandle(ref, () => ({
focus,
}));
render() {
const { props } = this;
const onChange = props.onChange || undefined;
// Dig into children props to see if the current value is valid or not.
@@ -108,7 +113,7 @@ export default class SelectField extends React.Component<Props, {||}> {
native: true,
}}
style={props.style}
inputRef={this._input}
inputRef={inputRef}
>
{!hasValidValue ? (
<option value={INVALID_VALUE} disabled>
@@ -123,4 +128,5 @@ export default class SelectField extends React.Component<Props, {||}> {
</I18n>
);
}
}
);
export default SelectField;
+23 -13
View File
@@ -2,6 +2,7 @@
import * as React from 'react';
import { I18n } from '@lingui/react';
import { type MessageDescriptor } from '../Utils/i18n/MessageDescriptor.flow';
import { useTheme } from '@material-ui/core/styles';
// We support a subset of the props supported by Material-UI v0.x MenuItem
// They should be self descriptive - refer to Material UI docs otherwise.
@@ -14,16 +15,25 @@ type Props = {|
/**
* A native select option to be used with `SelectField`.
*/
export default class SelectOption extends React.Component<Props, {||}> {
render() {
return (
<I18n>
{({ i18n }) => (
<option value={this.props.value} disabled={this.props.disabled}>
{i18n._(this.props.primaryText)}
</option>
)}
</I18n>
);
}
}
const SelectOption = (props: Props) => {
const muiTheme = useTheme();
return (
<I18n>
{({ i18n }) => (
<option
value={props.value}
disabled={props.disabled}
style={{
color: muiTheme.palette.text.primary,
backgroundColor: muiTheme.palette.background.paper,
}}
>
{i18n._(props.primaryText)}
</option>
)}
</I18n>
);
};
export default SelectOption;
+3 -3
View File
@@ -47,7 +47,7 @@ export const insertInVariablesContainer = (
name: string,
serializedVariable: ?any,
index: ?number
): string => {
): { name: string, variable: gdVariable } => {
const newName = newNameGenerator(
name,
name => variablesContainer.has(name),
@@ -59,13 +59,13 @@ export const insertInVariablesContainer = (
} else {
newVariable.setString('');
}
variablesContainer.insert(
const variable = variablesContainer.insert(
newName,
newVariable,
index || variablesContainer.count()
);
newVariable.delete();
return newName;
return { name: newName, variable };
};
export const insertInVariableChildrenArray = (
@@ -1,6 +1,7 @@
// @flow
import { mapFor } from '../Utils/MapFor';
import { normalizeString } from '../Utils/Search';
const gd: libGDevelop = global.gd;
type MovementType =
@@ -273,3 +274,108 @@ export const getMovementTypeWithinVariablesContainer = (
return null;
};
export const generateListOfNodesMatchingSearchInVariable = ({
variable,
variableName,
nodeId,
searchText,
acc,
}: {|
variable: gdVariable,
variableName: string,
nodeId: string,
searchText: string,
acc: Array<string>,
|}): Array<string> => {
let newAcc;
let childrenNodes;
switch (variable.getType()) {
case gd.Variable.Boolean:
if (normalizeString(variableName).includes(searchText)) {
return [nodeId];
}
return [];
case gd.Variable.String:
if (
normalizeString(variableName).includes(searchText) ||
normalizeString(variable.getString()).includes(searchText)
) {
return [nodeId];
}
return [];
case gd.Variable.Number:
if (
normalizeString(variableName).includes(searchText) ||
variable
.getValue()
.toString()
.includes(searchText)
) {
return [nodeId];
}
return [];
case gd.Variable.Array:
newAcc = [...acc];
if (normalizeString(variableName).includes(searchText)) {
newAcc.push(nodeId);
}
childrenNodes = mapFor(0, variable.getChildrenCount(), index => {
const childVariable = variable.getAtIndex(index);
return generateListOfNodesMatchingSearchInVariable({
variable: childVariable,
variableName: '',
nodeId: `${nodeId}${separator}${index}`,
searchText: searchText,
acc: newAcc,
});
}).flat();
return [...newAcc, ...childrenNodes];
case gd.Variable.Structure:
newAcc = [...acc];
if (normalizeString(variableName).includes(searchText)) {
newAcc.push(nodeId);
}
childrenNodes = variable
.getAllChildrenNames()
.toJSArray()
.map(childName => {
const childVariable = variable.getChild(childName);
return Array.from(
new Set(
generateListOfNodesMatchingSearchInVariable({
variable: childVariable,
variableName: childName,
nodeId: `${nodeId}${separator}${childName}`,
searchText: searchText,
acc,
})
)
);
})
.flat();
return [...newAcc, ...childrenNodes];
default:
return [];
}
};
export const generateListOfNodesMatchingSearchInVariablesContainer = (
variablesContainer: gdVariablesContainer,
searchText: string,
prefix?: string
): Array<string> => {
return mapFor(0, variablesContainer.count(), index => {
const variable = variablesContainer.getAt(index);
const name = variablesContainer.getNameAt(index);
return generateListOfNodesMatchingSearchInVariable({
variable,
variableName: name,
nodeId: prefix ? `${prefix}${name}` : name,
searchText,
acc: [],
});
}).flat();
};
@@ -1,5 +1,7 @@
// @flow
import {
generateListOfNodesMatchingSearchInVariable,
generateListOfNodesMatchingSearchInVariablesContainer,
getExpandedNodeIdsFromVariablesContainer,
getVariableContextFromNodeId,
separator,
@@ -26,13 +28,13 @@ describe('VariableToTreeNodeHandling', () => {
├── parent
│   ├── stringChild
│   └── arrayChild (folded)
│      ├── firstArrayChild
│      └── secondArrayChild
│      ├── firstArrayChild 35
│      └── secondArrayChild false
└── parent2 (folded)
   ├── boolChild
   ├── boolChild true
   └── structureChild
      ├── firstStructureChild
      └── secondStructureChild
      ├── firstStructureChild 'Danger'
      └── secondStructureChild 'secondStructureChild'
*/
parent = new gd.Variable();
parent.castTo('structure');
@@ -64,7 +66,7 @@ describe('VariableToTreeNodeHandling', () => {
structureChild.castTo('structure');
structureChild.setFolded(false);
firstStructureChild = new gd.Variable();
firstStructureChild.setString('firstStructureChild');
firstStructureChild.setString('Danger');
structureChild.insertChild('firstStructureChild', firstStructureChild);
secondStructureChild = new gd.Variable();
secondStructureChild.setString('secondStructureChild');
@@ -173,6 +175,112 @@ describe('VariableToTreeNodeHandling', () => {
});
});
describe('generateListOfNodesMatchingSearchInVariable', () => {
test('First variable should be included if name matches', () => {
expect(
generateListOfNodesMatchingSearchInVariable({
variable: parent,
variableName: 'parent',
nodeId: 'parent',
searchText: 'parent',
acc: [],
})
).toEqual(['parent']);
expect(
generateListOfNodesMatchingSearchInVariable({
variable: parent2,
variableName: 'parent2',
nodeId: 'parent2',
searchText: 'parent',
acc: [],
})
).toEqual(['parent2']);
});
test('Leaf variable in array should be included if value matches', () => {
expect(
generateListOfNodesMatchingSearchInVariable({
variable: parent,
variableName: 'parent',
nodeId: 'parent',
searchText: '35',
acc: [],
})
).toEqual([`parent${separator}arrayChild${separator}0`]);
});
test('Leaf variable in structure should be included if value matches', () => {
expect(
generateListOfNodesMatchingSearchInVariable({
variable: parent2,
variableName: 'parent2',
nodeId: 'parent2',
searchText: 'danger',
acc: [],
})
).toEqual([
`parent2${separator}structureChild${separator}firstStructureChild`,
]);
});
test('All branches should be included if each variable matches by the name and/or the value', () => {
expect(
generateListOfNodesMatchingSearchInVariable({
variable: parent2,
variableName: 'parent2',
nodeId: 'parent2',
searchText: 'structure',
acc: [],
})
).toEqual([
`parent2${separator}structureChild`,
`parent2${separator}structureChild${separator}firstStructureChild`,
`parent2${separator}structureChild${separator}secondStructureChild`,
]);
});
});
describe('generateListOfNodesMatchingSearchInVariablesContainer', () => {
test('First variable should be included if name matches', () => {
expect(
generateListOfNodesMatchingSearchInVariablesContainer(
variablesContainer,
'parent'
)
).toEqual(['parent', 'parent2']);
});
test('Leaf variable in array should be included if value matches', () => {
expect(
generateListOfNodesMatchingSearchInVariablesContainer(
variablesContainer,
'35'
)
).toEqual([`parent${separator}arrayChild${separator}0`]);
});
test('Leaf variable in structure should be included if value matches', () => {
expect(
generateListOfNodesMatchingSearchInVariablesContainer(
variablesContainer,
'danger'
)
).toEqual([
`parent2${separator}structureChild${separator}firstStructureChild`,
]);
});
test('All branches should be included if each variable matches by the name and/or the value', () => {
expect(
generateListOfNodesMatchingSearchInVariablesContainer(
variablesContainer,
'child'
)
).toEqual([
`parent${separator}arrayChild`,
`parent${separator}stringChild`,
`parent2${separator}boolChild`,
`parent2${separator}structureChild`,
`parent2${separator}structureChild${separator}firstStructureChild`,
`parent2${separator}structureChild${separator}secondStructureChild`,
]);
});
});
afterEach(() => {
variablesContainer.delete();
parent.delete();
+106 -20
View File
@@ -25,7 +25,9 @@ import ScrollView from '../UI/ScrollView';
import GDevelopThemeContext from '../UI/Theme/ThemeContext';
import { ResponsiveLineStackLayout } from '../UI/Layout';
import KeyboardShortcuts from '../UI/KeyboardShortcuts';
import SemiControlledAutoComplete from '../UI/SemiControlledAutoComplete';
import SemiControlledAutoComplete, {
type SemiControlledAutoCompleteInterface,
} from '../UI/SemiControlledAutoComplete';
import useForceUpdate from '../Utils/UseForceUpdate';
import { mapFor } from '../Utils/MapFor';
@@ -45,7 +47,6 @@ import {
saveToHistory,
} from '../Utils/History';
import {
hasChildThatContainsStringInNameOrValue,
hasVariablesContainerSubChildren,
insertInVariableChildren,
insertInVariableChildrenArray,
@@ -54,6 +55,7 @@ import {
} from '../Utils/VariablesUtils';
import {
foldNodesVariables,
generateListOfNodesMatchingSearchInVariablesContainer,
getDirectParentNodeId,
getDirectParentVariable,
getExpandedNodeIdsFromVariablesContainer,
@@ -161,14 +163,72 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
[onComputeAllVariableNames]
);
const [selectedNodes, setSelectedNodes] = React.useState<Array<string>>([]);
const [searchMatchingNodes, setSearchMatchingNodes] = React.useState<
Array<string>
>([]);
const [containerWidth, setContainerWidth] = React.useState<?number>(null);
const [nameErrors, setNameErrors] = React.useState<{ [number]: React.Node }>(
{}
);
const topLevelVariableNameInputRefs = React.useRef<{
[number]: SemiControlledAutoCompleteInterface,
}>({});
const [variablePtrToFocus, setVariablePtrToFocus] = React.useState<?number>(
null
);
const gdevelopTheme = React.useContext(GDevelopThemeContext);
const draggedNodeId = React.useRef<?string>(null);
const forceUpdate = useForceUpdate();
const DragSourceAndDropTarget = React.useMemo(
() => makeDragSourceAndDropTarget('variable-editor'),
[]
);
const triggerSearch = React.useCallback(
() => {
let matchingInheritedNodes = [];
const matchingNodes = generateListOfNodesMatchingSearchInVariablesContainer(
props.variablesContainer,
normalizeString(searchText)
);
if (props.inheritedVariablesContainer) {
matchingInheritedNodes = generateListOfNodesMatchingSearchInVariablesContainer(
props.inheritedVariablesContainer,
normalizeString(searchText),
inheritedPrefix
);
}
setSearchMatchingNodes([...matchingNodes, ...matchingInheritedNodes]);
},
[props.inheritedVariablesContainer, props.variablesContainer, searchText]
);
React.useEffect(
() => {
if (!!searchText) {
triggerSearch();
} else {
setSearchMatchingNodes([]);
}
},
[searchText, triggerSearch]
);
React.useEffect(
() => {
if (variablePtrToFocus) {
const inputRef =
topLevelVariableNameInputRefs.current[variablePtrToFocus];
if (inputRef) {
inputRef.focus();
setVariablePtrToFocus(null);
}
}
},
[variablePtrToFocus]
);
const shouldHideExpandIcons =
!hasVariablesContainerSubChildren(props.variablesContainer) &&
(props.inheritedVariablesContainer
@@ -319,7 +379,7 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
if (pasteAtTopLevel) {
if (!name) return;
const newName = insertInVariablesContainer(
const { name: newName } = insertInVariablesContainer(
props.variablesContainer,
name,
serializedVariable
@@ -340,7 +400,7 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
);
if (!targetParentVariable) {
if (!name) return;
const newName = insertInVariablesContainer(
const { name: newName } = insertInVariablesContainer(
props.variablesContainer,
name,
serializedVariable,
@@ -435,6 +495,15 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
setSelectedNodes(
updateListOfNodesFollowingChangeName(selectedNodes, oldNodeId, newName)
);
if (!!searchText) {
setSearchMatchingNodes(
updateListOfNodesFollowingChangeName(
searchMatchingNodes,
oldNodeId,
newName
)
);
}
};
const updateExpandedAndSelectedNodesFollowingNodeMove = (
@@ -451,13 +520,12 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
...inheritedExpandedNodes,
...getExpandedNodeIdsFromVariablesContainer(props.variablesContainer),
]);
if (!!searchText) {
triggerSearch();
forceUpdate();
}
};
const DragSourceAndDropTarget = React.useMemo(
() => makeDragSourceAndDropTarget('variable-editor'),
[]
);
const canDrop = (nodeId: string): boolean => {
if (nodeId.startsWith(inheritedPrefix)) return false;
const { current } = draggedNodeId;
@@ -713,7 +781,7 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
selectedNodes.some(node => node.startsWith(inheritedPrefix));
if (addAtTopLevel) {
const newName = insertInVariablesContainer(
const { name: newName, variable } = insertInVariablesContainer(
props.variablesContainer,
'Variable',
null,
@@ -721,6 +789,7 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
);
_saveToHistory();
setSelectedNodes([newName]);
setVariablePtrToFocus(variable.ptr);
return;
}
@@ -737,7 +806,7 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
} else {
position = props.variablesContainer.getPosition(oldestAncestry.name) + 1;
}
const newName = insertInVariablesContainer(
const { name: newName, variable } = insertInVariablesContainer(
props.variablesContainer,
'Variable',
null,
@@ -745,6 +814,7 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
);
_saveToHistory();
setSelectedNodes([newName]);
setVariablePtrToFocus(variable.ptr);
};
const renderVariableAndChildrenRows = (
@@ -801,13 +871,22 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
props.inheritedVariablesContainer &&
props.inheritedVariablesContainer.has(name);
const normalizedSearchText = normalizeString(searchText);
if (
!!searchText &&
!normalizeString(name).includes(normalizedSearchText) &&
!hasChildThatContainsStringInNameOrValue(variable, normalizedSearchText)
) {
return null;
if (!!searchText) {
if (
!(
searchMatchingNodes.includes(nodeId) ||
searchMatchingNodes.includes(parentNodeId) ||
searchMatchingNodes.some(matchingNodeId =>
matchingNodeId.startsWith(nodeId)
)
)
) {
// Display node if one of these is true:
// - node is in the list of nodes matching search
// - parent node is in the list of nodes matching search (to be able to edit direct children of searched structure)
// - node is an ancestry of a node in the list of nodes matching search
return null;
}
}
const valueInputStyle = {};
@@ -878,6 +957,13 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
{shouldWrap ? null : <Spacer />}
<SemiControlledAutoComplete
fullWidth
ref={element => {
if (depth === 0 && element) {
topLevelVariableNameInputRefs.current[
variable.ptr
] = element;
}
}}
dataSource={isTopLevel ? undefinedVariableNames : []}
margin="none"
key="name"
@@ -1356,11 +1442,11 @@ const VariablesList = ({ onComputeAllVariableNames, ...props }: Props) => {
{({ contentRect, measureRef }) => (
<div
ref={measureRef}
style={{ flex: 1, display: 'flex' }}
style={{ flex: 1, display: 'flex', minHeight: 0 }}
onKeyDown={keyboardShortcuts.onKeyDown}
onKeyUp={keyboardShortcuts.onKeyUp}
>
<Column expand>
<Column expand useFullHeight>
{isNarrow ? null : toolbar}
{props.variablesContainer.count() === 0 &&
(!props.inheritedVariablesContainer ||
+3 -3
View File
@@ -276,9 +276,9 @@ export const makeTestProject = (gd /*: libGDevelop */) /*: TestProject */ => {
1
);
variable.castTo('structure');
variable.getChild('InstanceChild1').setValue(564);
variable.getChild('InstanceChild2').setString('Guttentag');
variable.getChild('InstanceChild3').setBool(true);
variable.getChild('InstanceChild1').setValue(1995);
variable.getChild('InstanceChild2').setString('Hallo');
variable.getChild('InstanceChild3').setBool(false);
const arrayVariable = variable.getChild('InstanceChild4');
arrayVariable.castTo('array');
arrayVariable.pushNew().setString('Bonjour');