Rework the variables editor so that it's more performant and comfortable to use (#5690)

- The editor is now way faster to open when lots of variables are present. It's also way faster when editing and when lots of variables are present.
- Variable names that have forbidden characters are automatically fixed, not breaking your workflow.
- Deletion of multiple variables in an array was fixed. 
- Also fix adding a new variable sometimes overriding a variable inherited from the object
- Allow editing text variables in a separate window (more comfortable and faster)
This commit is contained in:
Florian Rival
2023-09-27 16:30:46 +02:00
committed by GitHub
parent 9f4b12ce8a
commit d4cb4fafd3
20 changed files with 1551 additions and 1138 deletions

View File

@@ -1,7 +1,7 @@
// @flow
import * as React from 'react';
import CommandManager, { CommandManagerInterface } from './CommandManager';
import useRefInit from './UseRefInitHook';
import useValueWithInit from '../Utils/UseRefInitHook';
const CommandsContext = React.createContext<CommandManagerInterface>(
new CommandManager()
@@ -12,7 +12,9 @@ type Props = {
};
export const CommandsContextProvider = (props: Props) => {
const commandManager = useRefInit<CommandManager>(() => new CommandManager());
const commandManager = useValueWithInit<CommandManager>(
() => new CommandManager()
);
return (
<CommandsContext.Provider value={commandManager}>

View File

@@ -7,7 +7,7 @@ import {
} from './CommandManager';
import { type CommandName } from './CommandsList';
import CommandsContext from './CommandsContext';
import useRefInit from './UseRefInitHook';
import useValueWithInit from '../Utils/UseRefInitHook';
class ScopedCommandManager implements CommandManagerInterface {
_commands: { [CommandName]: Command };
@@ -72,7 +72,7 @@ type Props = {|
const CommandsContextScopedProvider = (props: Props) => {
const centralManager = React.useContext(CommandsContext);
const scopedManager = useRefInit(
const scopedManager = useValueWithInit(
() => new ScopedCommandManager(centralManager)
);

View File

@@ -1,15 +0,0 @@
// @flow
import { useRef } from 'react';
const useRefInit = <T>(init: () => T): T => {
const instanceRef = useRef(null);
let instance = instanceRef.current;
if (instance !== null) return instance;
// Lazily create the ref object
let newInstance = init();
instanceRef.current = newInstance;
return newInstance;
};
export default useRefInit;

View File

@@ -377,7 +377,7 @@ const InstancePropertiesEditor = ({
</Column>
{object ? (
<VariablesList
commitChangesOnBlur={false}
directlyStoreValueChangesWhileEditing
inheritedVariablesContainer={object.getVariables()}
variablesContainer={instance.getVariables()}
size="small"

View File

@@ -291,7 +291,6 @@ const InnerDialog = (props: InnerDialogProps) => {
</Line>
)}
<VariablesList
commitChangesOnBlur
variablesContainer={props.object.getVariables()}
emptyPlaceholderTitle={
<Trans>Add your first object variable</Trans>

View File

@@ -23,6 +23,7 @@ type ErrorBoundaryScope =
| 'mainframe'
| 'list-search-result'
| 'box-search-result'
| 'variables-list'
| 'app';
const errorHandler = (

View File

@@ -0,0 +1,55 @@
/**
* CSS classes for a text field, inspired from Material UI, but lightweight
* and faster to render.
*/
.gd-simple-text-field {
position: relative;
width: 100%;
}
.gd-simple-text-field input {
font-family: var(--gdevelop-modern-font-family);
font-size: 14px;
outline: none;
border: none;
padding: 0;
background-image: none;
background-color: transparent;
-webkit-box-shadow: none;
-moz-box-shadow: none;
box-shadow: none;
color: inherit;
width: 100%;
}
.gd-simple-text-field.gd-disabled input {
color: var(--theme-text-disabled-color);
}
.gd-simple-text-field:before {
bottom: -1px;
left: 0;
right: 0;
content: '\00a0';
border-bottom: 1px solid var(--theme-text-default-color);
display: block;
position: absolute;
pointer-events: none;
}
.gd-simple-text-field:hover::before {
border-width: 2px;
}
.gd-simple-text-field:focus-within::before {
border-width: 2px;
}
.gd-simple-text-field.gd-disabled:before {
border-bottom-style: dotted;
border-color: var(--theme-text-disabled-color);
}
.gd-simple-text-field.gd-disabled:hover::before {
border-width: 1px;
}
.gd-simple-text-field.gd-disabled:focus-within::before {
border-width: 1px;
}

View File

@@ -0,0 +1,136 @@
// @flow
import * as React from 'react';
import { shouldValidate } from './KeyboardShortcuts/InteractionKeys';
import './SimpleTextField.css';
type SimpleTextFieldProps = {|
disabled: boolean,
type: 'number' | 'text',
onChange: (newValue: string, context: any) => void,
value: string,
id: string,
additionalContext?: any,
italic?: boolean,
/**
* Only to be used in the exceptional case where any change
* must be immediately communicated to the parent.
*/
directlyStoreValueChangesWhileEditing?: boolean,
|};
type FocusOptions = {|
selectAll?: boolean,
caretPosition?: number,
|};
export type SimpleTextFieldInterface = {|
focus: (options: ?FocusOptions) => void,
forceSetSelection: (selectionStart: number, selectionEnd: number) => void,
getCaretPosition: () => number,
|};
const styles = {
italic: {
fontStyle: 'italic',
},
};
const stopPropagation = e => e.stopPropagation();
/**
* A text field, inspired from Material UI, but lightweight
* and faster to render (2 DOM elements, uncontrolled, pure CSS styling).
*/
export const SimpleTextField = React.memo<
SimpleTextFieldProps,
SimpleTextFieldInterface
>(
React.forwardRef<SimpleTextFieldProps, SimpleTextFieldInterface>(
(props, ref) => {
const inputRef = React.useRef<?HTMLInputElement>(null);
React.useEffect(
() => {
// If the value passed changed, update the input. Otherwise,
// keep the input uncontrolled.
if (inputRef.current) inputRef.current.value = props.value;
},
[props.value]
);
const focus = React.useCallback((options: ?FocusOptions) => {
const input = inputRef.current;
if (input) {
input.focus();
if (options && options.selectAll) {
input.select();
}
if (options && Number.isInteger(options.caretPosition)) {
const position = Number(options.caretPosition);
input.setSelectionRange(position, position);
}
}
}, []);
const forceSetSelection = React.useCallback(
(selectionStart: number, selectionEnd: number) => {
if (inputRef.current) {
inputRef.current.selectionStart = selectionStart;
inputRef.current.selectionEnd = selectionEnd;
}
},
[]
);
const getCaretPosition = React.useCallback(() => {
if (inputRef.current) return inputRef.current.selectionStart;
return 0;
}, []);
React.useImperativeHandle(ref, () => ({
focus,
forceSetSelection,
getCaretPosition,
}));
return (
<div
className={`gd-simple-text-field ${
props.disabled ? 'gd-disabled' : ''
}`}
>
<input
disabled={props.disabled}
ref={inputRef}
type={props.type}
defaultValue={props.value}
onClick={stopPropagation}
onDoubleClick={stopPropagation}
onBlur={e => {
props.onChange(e.currentTarget.value, props.additionalContext);
}}
onChange={
props.directlyStoreValueChangesWhileEditing
? e => {
props.onChange(
e.currentTarget.value,
props.additionalContext
);
}
: undefined
}
onKeyUp={e => {
if (shouldValidate(e)) {
props.onChange(e.currentTarget.value, props.additionalContext);
}
}}
style={props.italic ? styles.italic : undefined}
/>
</div>
);
}
)
);

View File

@@ -13,6 +13,7 @@ export function getRootClassNames(theme: string) {
eventsSheetRootClassName: theme,
tableRootClassName: theme,
markdownRootClassName: theme,
uiRootClassName: theme,
};
}

View File

@@ -0,0 +1,29 @@
// @flow
import { useRef } from 'react';
const useValueWithInit = <T>(init: () => T): T => {
const instanceRef = useRef<T | null>(null);
let instance = instanceRef.current;
if (instance !== null) return instance;
// Lazily create the ref object.
let newInstance = init();
instanceRef.current = newInstance;
return newInstance;
};
export default useValueWithInit;
export const useRefWithInit = <T>(init: () => T): {| current: T |} => {
const instanceRef = useRef<T | null>(null);
let instance = instanceRef.current;
// $FlowFixMe - we have the guarantee that T can't be null.
if (instance !== null) return instanceRef;
// Lazily create the ref object.
let newInstance = init();
instanceRef.current = newInstance;
// $FlowFixMe - we have the guarantee that T can't be null.
return instanceRef;
};

View File

@@ -45,12 +45,18 @@ export const hasChildThatContainsStringInNameOrValue = (
export const insertInVariablesContainer = (
variablesContainer: gdVariablesContainer,
name: string,
serializedVariable: ?any,
index: ?number
serializedVariable: any | null,
index: number,
inheritedVariablesContainer: ?gdVariablesContainer
): { name: string, variable: gdVariable } => {
const newName = newNameGenerator(
name,
name => variablesContainer.has(name),
name => {
return (
variablesContainer.has(name) ||
(!!inheritedVariablesContainer && inheritedVariablesContainer.has(name))
);
},
serializedVariable ? 'CopyOf' : undefined
);
const newVariable = new gd.Variable();
@@ -60,11 +66,7 @@ export const insertInVariablesContainer = (
} else {
newVariable.setString('');
}
const variable = variablesContainer.insert(
newName,
newVariable,
index || variablesContainer.count()
);
const variable = variablesContainer.insert(newName, newVariable, index);
newVariable.delete();
return { name: newName, variable };
};

View File

@@ -0,0 +1,48 @@
// @flow
import * as React from 'react';
import { Trans } from '@lingui/macro';
import Dialog, { DialogPrimaryButton } from '../UI/Dialog';
import SemiControlledTextField from '../UI/SemiControlledTextField';
type Props = {|
initialValue: string,
onClose: (newValue: string) => void,
|};
export const MultilineVariableEditorDialog = ({
initialValue,
onClose,
}: Props) => {
const [value, setValue] = React.useState(initialValue);
return (
<Dialog
open
title={null}
flexColumnBody
noMobileFullScreen
actions={[
<DialogPrimaryButton
key="ok"
label={<Trans>Ok</Trans>}
primary
onClick={() => onClose(value)}
/>,
]}
maxWidth="md"
onRequestClose={() => onClose(value)}
onApply={() => onClose(value)}
>
<SemiControlledTextField
autoFocus="desktopAndMobileDevices"
multiline
fullWidth
floatingLabelText={<Trans>Initial text of the variable</Trans>}
value={value}
onChange={setValue}
rows={5}
rowsMax={10}
/>
</Dialog>
);
};

View File

@@ -102,43 +102,6 @@ export const getVariableContextFromNodeId = (
};
};
export const getExpandedNodeIdsFromVariables = (
variables: { name: string, variable: gdVariable }[],
accumulator: string[],
parentNodeId: string = ''
): string[] => {
let newAccumulator = [];
for (const { name, variable } of variables) {
const nodeId = parentNodeId ? `${parentNodeId}${separator}${name}` : name;
if (!variable.isFolded() && variable.getChildrenCount() > 0) {
newAccumulator.push(nodeId);
}
if (variable.getType() === gd.Variable.Array) {
const children = mapFor(0, variable.getChildrenCount(), index => ({
name: index.toString(),
variable: variable.getAtIndex(index),
}));
newAccumulator = [
...newAccumulator,
...getExpandedNodeIdsFromVariables(children, newAccumulator, nodeId),
];
} else if (variable.getType() === gd.Variable.Structure) {
const children = variable
.getAllChildrenNames()
.toJSArray()
.map((childName, index) => ({
variable: variable.getChild(childName),
name: childName,
}));
newAccumulator = [
...newAccumulator,
...getExpandedNodeIdsFromVariables(children, newAccumulator, nodeId),
];
}
}
return newAccumulator;
};
export const updateListOfNodesFollowingChangeName = (
list: string[],
oldNodeId: string,
@@ -164,38 +127,6 @@ export const updateListOfNodesFollowingChangeName = (
return newList;
};
export const getExpandedNodeIdsFromVariablesContainer = (
variablesContainer: gdVariablesContainer,
isInherited: boolean = false
): string[] => {
const variables = [];
for (let index = 0; index < variablesContainer.count(); index += 1) {
variables.push({
name: `${
isInherited ? inheritedPrefix : ''
}${variablesContainer.getNameAt(index)}`,
variable: variablesContainer.getAt(index),
});
}
return getExpandedNodeIdsFromVariables(variables, []);
};
export const foldNodesVariables = (
variablesContainer: gdVariablesContainer,
nodes: string[],
fold: boolean
) => {
nodes.forEach(nodeId => {
const { variable } = getVariableContextFromNodeId(
nodeId,
variablesContainer
);
if (variable) {
variable.setFolded(fold);
}
});
};
export const getMovementTypeWithinVariablesContainer = (
draggedVariableContext: VariableContext,
targetVariableContext: VariableContext

View File

@@ -2,7 +2,6 @@
import {
generateListOfNodesMatchingSearchInVariable,
generateListOfNodesMatchingSearchInVariablesContainer,
getExpandedNodeIdsFromVariablesContainer,
getVariableContextFromNodeId,
separator,
updateListOfNodesFollowingChangeName,
@@ -129,14 +128,6 @@ describe('VariableToTreeNodeHandling', () => {
});
});
describe('getExpandedNodeIdsFromVariablesContainer', () => {
test('List of unfolded nodes are returned', () => {
expect(
getExpandedNodeIdsFromVariablesContainer(variablesContainer)
).toEqual(['parent', `parent2${separator}structureChild`]);
});
});
describe('updateListOfNodesFollowingChangeName', () => {
test('Concerned variable node id is modified in the list', () => {
expect(

View File

@@ -15,9 +15,10 @@ const gd = global.gd;
type Props = {|
variableType: Variable_Type,
onChange: (newVariableType: string) => void,
onChange: (newVariableType: string, nodeId: string) => void,
nodeId: string,
isHighlighted?: boolean,
disabled?: boolean,
readOnlyWithIcon?: boolean,
id?: string,
|};
@@ -80,7 +81,7 @@ const getVariableTypeToString = () => {
return variableTypeToString;
};
const VariableTypeSelector = (props: Props) => {
const VariableTypeSelector = React.memo<Props>((props: Props) => {
const gdevelopTheme = React.useContext(GDevelopThemeContext);
const Icon = getVariableTypeToIcon()[props.variableType];
@@ -94,26 +95,33 @@ const VariableTypeSelector = (props: Props) => {
: undefined
}
/>
<Spacer />
<SelectField
value={props.variableType}
margin="none"
stopPropagationOnClick
onChange={event =>
props.onChange(getVariableTypeToString()[event.target.value])
}
inputStyle={
props.isHighlighted
? { color: gdevelopTheme.listItem.selectedTextColor }
: undefined
}
disabled={props.disabled}
id={props.id}
>
{getOptions()}
</SelectField>
{!props.readOnlyWithIcon && (
<>
<Spacer />
<SelectField
value={props.variableType}
margin="none"
stopPropagationOnClick
onChange={event =>
props.onChange(
getVariableTypeToString()[event.target.value],
props.nodeId
)
}
inputStyle={{
fontSize: 14,
color: props.isHighlighted
? gdevelopTheme.listItem.selectedTextColor
: undefined,
}}
id={props.id}
>
{getOptions()}
</SelectField>
</>
)}
</Line>
);
};
});
export default VariableTypeSelector;

View File

@@ -168,7 +168,6 @@ const VariablesEditorDialog = ({
</Line>
)}
<VariablesList
commitChangesOnBlur
variablesContainer={variablesContainer}
inheritedVariablesContainer={inheritedVariablesContainer}
emptyPlaceholderTitle={emptyPlaceholderTitle}

File diff suppressed because it is too large Load Diff

View File

@@ -1,6 +1,11 @@
// @flow
import React from 'react';
type FocusOptions = {|
identifier: number,
caretPosition?: ?number,
|};
const useRefocusField = (fieldRefs: {|
current: {|
[identifier: number]: {|
@@ -8,23 +13,23 @@ const useRefocusField = (fieldRefs: {|
|},
|},
|}) => {
const [fieldToFocus, setFieldToFocus] = React.useState<?{
identifier: number,
caretPosition?: ?number,
}>(null);
const fieldToFocus = React.useRef<FocusOptions | null>(null);
React.useEffect(
() => {
if (fieldToFocus) {
const fieldRef = fieldRefs.current[fieldToFocus.identifier];
if (fieldRef) {
fieldRef.focus({ caretPosition: fieldToFocus.caretPosition });
setFieldToFocus(null);
}
const setFieldToFocus = React.useCallback((options: FocusOptions) => {
fieldToFocus.current = options;
}, []);
React.useLayoutEffect(() => {
if (fieldToFocus.current) {
const fieldRef = fieldRefs.current[fieldToFocus.current.identifier];
if (fieldRef) {
fieldRef.focus({ caretPosition: fieldToFocus.current.caretPosition });
}
},
[fieldToFocus, fieldRefs]
);
}
fieldToFocus.current = null;
});
return setFieldToFocus;
};

View File

@@ -0,0 +1,50 @@
// @flow
import * as React from 'react';
import { action } from '@storybook/addon-actions';
import muiDecorator from '../ThemeDecorator';
import { SimpleTextField } from '../../UI/SimpleTextField';
import { ColumnStackLayout } from '../../UI/Layout';
import paperDecorator from '../PaperDecorator';
export default {
title: 'UI Building Blocks/SimpleTextField',
component: SimpleTextField,
decorators: [paperDecorator, muiDecorator],
};
export const Default = () => (
<ColumnStackLayout>
<SimpleTextField
disabled={false}
type="text"
id="some-id-1"
value={'Test 123'}
onChange={action('onChange')}
/>
<SimpleTextField
disabled={false}
type="number"
id="some-id-2"
value={'456.123'}
onChange={action('onChange')}
/>
<SimpleTextField
disabled={false}
type="text"
id="some-id-3"
italic
value={'Test 456'}
onChange={action('onChange')}
/>
<SimpleTextField
disabled={true}
type="text"
id="some-id-3"
italic
value={'Test 456'}
onChange={action('onChange')}
/>
</ColumnStackLayout>
);

View File

@@ -13,7 +13,6 @@ export const Default = () => (
<DragAndDropContextProvider>
<FixedHeightFlexContainer height={600}>
<VariablesList
commitChangesOnBlur
variablesContainer={testProject.testLayout.getVariables()}
emptyPlaceholderDescription="Variables help you store data"
emptyPlaceholderTitle="Variables"
@@ -32,7 +31,6 @@ export const InstanceWithObjectVariables = () => (
<DragAndDropContextProvider>
<FixedHeightFlexContainer height={600}>
<VariablesList
commitChangesOnBlur
variablesContainer={testProject.testSpriteObjectInstance.getVariables()}
emptyPlaceholderDescription="Variables help you store data"
emptyPlaceholderTitle="Variables"