Skip to content

Commit aeb4058

Browse files
laa-odoolaa-odoo
committed
[IMP] formula assistant: Adapt UI for repeatable arguments
Following previous commits, the formula assistant is no longer useful for repeatable arguments. We've lost the information telling us when the first repeatable group is required or not. This commit fixes this. closes #7027 Task: 5082027 Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
1 parent a74bbfb commit aeb4058

File tree

12 files changed

+405
-345
lines changed

12 files changed

+405
-345
lines changed

‎packages/o-spreadsheet-engine/src/formulas/compiler.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ function compileTokensOrThrow(tokens: Token[]): CompiledFormula {
134134
const argToFocus = argTargeting(functionDefinition, args.length);
135135

136136
for (let i = 0; i < args.length; i++) {
137-
const argDefinition = functionDefinition.args[argToFocus(i) ?? -1];
137+
const argDefinition = functionDefinition.args[argToFocus(i).index ?? -1];
138138
const currentArg = args[i];
139139
const argTypes = argDefinition.type || [];
140140

‎packages/o-spreadsheet-engine/src/formulas/formula_formatter.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ function astToDoc(ast: AST): Doc {
248248
const docs: Doc[] = [];
249249
let i = 0;
250250
while (i < ast.args.length) {
251-
const isRepeating = functionDescription.args[getArgToFocus(i) || -1]?.repeating;
251+
const isRepeating = functionDescription.args[getArgToFocus(i).index ?? -1]?.repeating;
252252
if (isRepeating) {
253253
const repeatingArgSeries = ast.args.slice(i, i + functionDescription.nbrArgRepeating);
254254
const docsSeries = repeatingArgSeries.map((arg) => astToDoc(arg));

‎packages/o-spreadsheet-engine/src/functions/arguments.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,10 @@ export function addMetaInfoFromArg(
128128
return descr;
129129
}
130130

131-
type ArgToFocus = (argPosition: number) => number | undefined;
131+
type ArgToFocus = (argPosition: number) => {
132+
index: number | undefined;
133+
repeatingGroupIndex?: number;
134+
};
132135
const cacheArgTargeting: Record<string, Record<number, ArgToFocus>> = {};
133136

134137
/**
@@ -228,7 +231,10 @@ export function _argTargeting(
228231
functionDescription: FunctionDescription,
229232
nbrArgSupplied: number
230233
): ArgToFocus {
231-
const valueIndexToArgPosition: Record<number, number> = {};
234+
const valueIndexToArgPosition: Record<
235+
number,
236+
{ index: number | undefined; repeatingGroupIndex?: number }
237+
> = {};
232238
const groupsOfOptionalRepeatingValues = functionDescription.nbrArgRepeating
233239
? Math.floor(
234240
(nbrArgSupplied - functionDescription.minArgRequired) / functionDescription.nbrArgRepeating
@@ -247,7 +253,7 @@ export function _argTargeting(
247253

248254
if ((arg.optional || arg.default) && !arg.repeating) {
249255
if (countValueOptional < nbrValueOptional) {
250-
valueIndexToArgPosition[countValueSupplied] = i;
256+
valueIndexToArgPosition[countValueSupplied] = { index: i };
251257
countValueSupplied++;
252258
}
253259
countValueOptional++;
@@ -261,7 +267,7 @@ export function _argTargeting(
261267
// --> the index i will be incremented by the number of repeating values at the end of the loop
262268
for (let j = 0; j < groupsOfOptionalRepeatingValues + groupOfMandatoryRepeatingValues; j++) {
263269
for (let k = 0; k < functionDescription.nbrArgRepeating; k++) {
264-
valueIndexToArgPosition[countValueSupplied] = i + k;
270+
valueIndexToArgPosition[countValueSupplied] = { index: i + k, repeatingGroupIndex: j };
265271
countValueSupplied++;
266272
}
267273
}
@@ -270,7 +276,7 @@ export function _argTargeting(
270276
}
271277

272278
// End case: it's a required argument
273-
valueIndexToArgPosition[countValueSupplied] = i;
279+
valueIndexToArgPosition[countValueSupplied] = { index: i };
274280
countValueSupplied++;
275281
}
276282

‎packages/o-spreadsheet-engine/src/functions/create_compute_function.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ export function createComputeFunction(
1919
const getArgToFocus = argTargeting(descr, args.length);
2020
//#region Compute vectorisation limits
2121
for (let i = 0; i < args.length; i++) {
22-
const argIndex = getArgToFocus(i) ?? -1;
22+
const argIndex = getArgToFocus(i).index ?? -1;
2323
const argDefinition = descr.args[argIndex];
2424
const arg = args[i];
2525
if (!isMatrix(arg) && argDefinition.acceptMatrixOnly) {
@@ -44,7 +44,7 @@ export function createComputeFunction(
4444
for (let i = 0; i < args.length; i++) {
4545
const arg = args[i];
4646
const getArgToFocus = argTargeting(descr, args.length);
47-
const argDefinition = descr.args[getArgToFocus(i) ?? i];
47+
const argDefinition = descr.args[getArgToFocus(i).index ?? i];
4848

4949
// Early exit if the argument is an error and the function does not accept errors
5050
// We only check scalar arguments, not matrix arguments for performance reasons.

‎src/components/composer/composer/composer.ts‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ interface FunctionDescriptionState {
7272
showDescription: boolean;
7373
functionDescription: FunctionDescription;
7474
argsToFocus: number[];
75+
repeatingArgGroupIndex: number | undefined;
7576
}
7677

7778
export class Composer extends Component<CellComposerProps, SpreadsheetChildEnv> {
@@ -116,6 +117,7 @@ export class Composer extends Component<CellComposerProps, SpreadsheetChildEnv>
116117
showDescription: false,
117118
functionDescription: {} as FunctionDescription,
118119
argsToFocus: [],
120+
repeatingArgGroupIndex: 0,
119121
});
120122
assistant = useState({
121123
forcedClosed: false,
@@ -761,10 +763,40 @@ export class Composer extends Component<CellComposerProps, SpreadsheetChildEnv>
761763
);
762764

763765
this.functionDescriptionState.showDescription = true;
766+
this.functionDescriptionState.repeatingArgGroupIndex = this.getRepeatingArgGroupIndex(
767+
description,
768+
nbrArgSupplied,
769+
argPosition
770+
);
764771
}
765772
}
766773
}
767774

775+
private getRepeatingArgGroupIndex(
776+
description: FunctionDescription,
777+
nbrArgSupplied: number,
778+
argPosition: number
779+
): number | undefined {
780+
const { minArgRequired, maxArgPossible, nbrArgRepeating } = description;
781+
782+
if (!nbrArgRepeating) {
783+
return undefined;
784+
}
785+
786+
const groupsOfOptionalRepeatingValues = nbrArgRepeating
787+
? Math.ceil((nbrArgSupplied - minArgRequired) / nbrArgRepeating)
788+
: 0;
789+
790+
const nbrArgSuppliedRoundedToGroupOfRepeating =
791+
groupsOfOptionalRepeatingValues * nbrArgRepeating + minArgRequired;
792+
return (
793+
argTargeting(
794+
description,
795+
Math.max(Math.min(maxArgPossible, nbrArgSuppliedRoundedToGroupOfRepeating), minArgRequired)
796+
)(argPosition).repeatingGroupIndex ?? 0
797+
);
798+
}
799+
768800
/**
769801
* Compute the arguments to focus depending on the current value position.
770802
*
@@ -788,7 +820,7 @@ export class Composer extends Component<CellComposerProps, SpreadsheetChildEnv>
788820
const focusedArg = argTargeting(
789821
description,
790822
Math.max(Math.min(maxArgPossible, nbrArgSupplied), minArgRequired)
791-
)(argPosition);
823+
)(argPosition)?.index;
792824
return focusedArg !== undefined ? [focusedArg] : [];
793825
}
794826

@@ -803,7 +835,7 @@ export class Composer extends Component<CellComposerProps, SpreadsheetChildEnv>
803835

804836
const argsToFocus: number[] = [];
805837
for (let i = minArgsNumberPossibility; i <= maxArgsNumberPossibility; i++) {
806-
const focusedArg = argTargeting(description, i)(argPosition);
838+
const focusedArg = argTargeting(description, i)(argPosition)?.index;
807839
if (focusedArg !== undefined) {
808840
argsToFocus.push(focusedArg);
809841
}

‎src/components/composer/composer/composer.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
t-if="functionDescriptionState.showDescription"
6868
functionDescription="functionDescriptionState.functionDescription"
6969
argsToFocus="functionDescriptionState.argsToFocus"
70+
repeatingArgGroupIndex="functionDescriptionState.repeatingArgGroupIndex"
7071
/>
7172
<div
7273
t-if="functionDescriptionState.showDescription and autoCompleteProposals.length"

‎src/components/composer/formula_assistant/formula_assistant.css‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,9 @@
1111
font-size: 85%;
1212
}
1313
.o-formula-assistant-focus {
14-
div:first-child,
15-
span {
16-
color: var(--os-composer-assistant-color);
17-
text-shadow: 0px 0px 1px var(--os-composer-assistant-color);
18-
}
19-
div:last-child {
14+
color: var(--os-composer-assistant-color);
15+
text-shadow: 0px 0px 1px var(--os-composer-assistant-color);
16+
& + div {
2017
color: black;
2118
}
2219
}

‎src/components/composer/formula_assistant/formula_assistant.ts‎

Lines changed: 73 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@ import { Collapse } from "../../side_panel/components/collapse/collapse";
66
interface Props {
77
functionDescription: FunctionDescription;
88
argsToFocus: number[];
9+
repeatingArgGroupIndex: number | undefined;
910
}
1011

1112
export class FunctionDescriptionProvider extends Component<Props, SpreadsheetChildEnv> {
1213
static template = "o-spreadsheet-FunctionDescriptionProvider";
1314
static props = {
1415
functionDescription: Object,
1516
argsToFocus: Array,
17+
repeatingArgGroupIndex: { type: Number, optional: true },
1618
};
1719
static components = { Collapse };
1820

@@ -28,7 +30,76 @@ export class FunctionDescriptionProvider extends Component<Props, SpreadsheetChi
2830
return this.props;
2931
}
3032

31-
get formulaArgSeparator() {
32-
return this.env.model.getters.getLocale().formulaArgSeparator + " ";
33+
get formulaHeaderContent(): { content: string; focused?: boolean }[] {
34+
const { functionDescription, repeatingArgGroupIndex, argsToFocus } = this.props;
35+
const argSeparator = this.env.model.getters.getLocale().formulaArgSeparator + " ";
36+
37+
const result: { content: string; focused?: boolean }[] = [
38+
{ content: functionDescription.name + " ( " },
39+
];
40+
41+
for (let i = 0; i < functionDescription.args.length; i++) {
42+
const arg = functionDescription.args[i];
43+
const isRepeating = arg.repeating;
44+
45+
if (i > 0) {
46+
result.push({ content: argSeparator });
47+
}
48+
49+
if (isRepeating) {
50+
// treat all repeating args in one go
51+
const displayBrackets = arg.optional || (repeatingArgGroupIndex ?? 0) > 0;
52+
const repeatingArgNames = functionDescription.args
53+
.slice(i, i + functionDescription.nbrArgRepeating)
54+
.map((arg) => arg.name);
55+
56+
if (repeatingArgGroupIndex) {
57+
result.push({ content: "... " + argSeparator });
58+
}
59+
if (displayBrackets) {
60+
result.push({ content: "[" });
61+
}
62+
for (let idx = 0; idx < repeatingArgNames.length; idx++) {
63+
const name = repeatingArgNames[idx];
64+
const argIndex = i + idx;
65+
const focused = argsToFocus.includes(argIndex);
66+
result.push({ content: name + ((repeatingArgGroupIndex ?? 0) + 1), focused });
67+
// Add separator after each element except the last
68+
if (idx < repeatingArgNames.length - 1) {
69+
result.push({ content: argSeparator });
70+
}
71+
}
72+
if (displayBrackets) {
73+
result.push({ content: "]" });
74+
}
75+
result.push({ content: argSeparator + "[" });
76+
for (let idx = 0; idx < repeatingArgNames.length; idx++) {
77+
const name = repeatingArgNames[idx];
78+
result.push({ content: name + ((repeatingArgGroupIndex ?? 0) + 2) });
79+
// Add separator after each element except the last
80+
if (idx < repeatingArgNames.length - 1) {
81+
result.push({ content: argSeparator });
82+
}
83+
}
84+
result.push({ content: "]" + argSeparator + "... " });
85+
86+
// Skip the processed repeating args
87+
i += functionDescription.nbrArgRepeating - 1;
88+
} else {
89+
const displayBrackets = arg.optional || arg.default;
90+
const focused = argsToFocus.includes(i);
91+
if (displayBrackets) {
92+
result.push({ content: "[" });
93+
}
94+
result.push({ content: arg.name, focused });
95+
if (displayBrackets) {
96+
result.push({ content: "]" });
97+
}
98+
}
99+
}
100+
101+
result.push({ content: " )" });
102+
103+
return result;
33104
}
34105
}

‎src/components/composer/formula_assistant/formula_assistant.xml‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,12 @@
55
<div class="o-formula-assistant" t-if="context.functionDescription.name">
66
<div class="o-formula-assistant-head d-flex flex-row justify-content-between">
77
<div>
8-
<span t-esc="context.functionDescription.name"/>
9-
(
10-
<t t-foreach="context.functionDescription.args" t-as="arg" t-key="arg.name">
11-
<span t-if="arg_index > '0'" t-esc="formulaArgSeparator"/>
8+
<t t-foreach="formulaHeaderContent" t-as="part" t-key="part_index">
129
<span
13-
t-att-class="{ 'o-formula-assistant-focus': context.argsToFocus.includes(arg_index) }">
14-
<span>
15-
<span t-if="arg.optional || arg.repeating || arg.default">[</span>
16-
<span t-esc="arg.name"/>
17-
<span t-if="arg.repeating">, ...</span>
18-
<span t-if="arg.optional || arg.repeating || arg.default">]</span>
19-
</span>
20-
</span>
10+
t-esc="part.content"
11+
t-att-class="part.focused ? 'o-formula-assistant-focus' : ''"
12+
/>
2113
</t>
22-
)
2314
</div>
2415
<div
2516
class="collapsor px-2 d-flex align-items-center rounded"
@@ -37,22 +28,29 @@
3728
<div t-esc="context.functionDescription.description"/>
3829
</div>
3930

31+
<t
32+
t-set="firstRepeatableGroupOptional"
33+
t-value="context.functionDescription.args.some(arg => arg.repeating and arg.optional)"
34+
/>
35+
4036
<t t-foreach="context.functionDescription.args" t-as="arg" t-key="arg.name">
4137
<div
4238
class="o-formula-assistant-arg p-3 pt-0 display-flex flex-column"
43-
t-att-class="{
44-
'o-formula-assistant-gray': context.argsToFocus.length > 0,
45-
'o-formula-assistant-focus': context.argsToFocus.includes(arg_index),
46-
}">
47-
<div>
48-
<span t-esc="arg.name"/>
39+
t-att-class="{'o-formula-assistant-gray': context.argsToFocus.length > 0}">
40+
<div
41+
t-att-class="{'o-formula-assistant-focus': context.argsToFocus.includes(arg_index)}">
42+
<span t-if="arg.repeating">
43+
<span t-esc="arg.name + (context.repeatingArgGroupIndex + 1)"/>
44+
</span>
45+
<span t-else="">
46+
<span t-esc="arg.name"/>
47+
</span>
4948
<span
50-
t-if="arg.optional || arg.repeating || arg.default ">&#xA0;- [optional]&#xA0;</span>
49+
t-if="arg.optional || arg.default || (arg.repeating and (firstRepeatableGroupOptional or context.repeatingArgGroupIndex > 0))">&#xA0;- [optional]&#xA0;</span>
5150
<span t-if="arg.default">
5251
<span>default:&#xA0;</span>
5352
<t t-esc="arg.defaultValue"/>
5453
</span>
55-
<span t-if="arg.repeating">repeatable</span>
5654
</div>
5755
<div class="o-formula-assistant-arg-description" t-esc="arg.description"/>
5856
</div>

0 commit comments

Comments
 (0)