Skip to content

Commit

Permalink
[REV] pivot: select pivot cells in composer
Browse files Browse the repository at this point in the history
This reverts commit 6c4a8e7.

It is quite annoying to build a formula by referring a pivot value and getting
the full pivot formula rather than its cell address.

So a good behavior IMO would be:

If you copy a value from a pivot and paste it somewhere else, you would get
the full formula (i.e. =PIVOT(1,$$$,$$$,$$$))
If you select a value from a pivot while building a formula within the composer,
you would get its address (i.e. =SUM(1+C3+4)) and not
(=SUM(1+(PIVOT(1,$$$,$$$,$$$))+4)

Additional notes:

With the current behavior (full formula inserted) and if you actually want the
reference, it's very annoying: you have to delete the full formula, carefully
check the reference of the cell you want to target, then type the reference by
hand. The other way around is slightly easier: either build the formula using
the auto-complete assistant, or copy-paste the cell somewhere else and
reference that cell with the fixed formula.

There are pros & cons with both behaviors, and they are incompatible.
I say: if you use a dynamic pivot, we let you embrace dynamic all the way,
otherwise just use a fixed formula pivot. Reverting full formula insertion is
also less code and less surprising (not a different behavior just for pivots)

closes #5197

Task: 4189098
Signed-off-by: Rémi Rahir (rar) <[email protected]>
  • Loading branch information
LucasLefevre committed Dec 3, 2024
1 parent e84a864 commit 88aa2a7
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 330 deletions.
59 changes: 8 additions & 51 deletions src/components/composer/composer/abstract_composer_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ import {
splitReference,
zoneToDimension,
} from "../../../helpers/index";
import { canonicalizeNumberContent, localizeFormula } from "../../../helpers/locale";
import { createPivotFormula } from "../../../helpers/pivot/pivot_helpers";
import { canonicalizeNumberContent } from "../../../helpers/locale";
import { cycleFixedReference } from "../../../helpers/reference_type";
import {
AutoCompleteProvider,
Expand Down Expand Up @@ -384,22 +383,8 @@ export abstract class AbstractComposerStore extends SpreadsheetStore {
private replaceSelectedRange(zone: Zone | UnboundedZone) {
const ref = this.getZoneReference(zone);
const currentToken = this.tokenAtCursor;

let replaceStart = this.selectionStart;
if (currentToken?.type === "REFERENCE") {
replaceStart = currentToken.start;
} else if (currentToken?.type === "RIGHT_PAREN") {
// match left parenthesis
const leftParenthesisIndex = this.currentTokens.findIndex(
(token) => token.type === "LEFT_PAREN" && token.parenIndex === currentToken.parenIndex
);
const functionToken = this.currentTokens[leftParenthesisIndex - 1];
if (functionToken === undefined) {
return;
}
replaceStart = functionToken.start;
}
this.replaceText(ref, replaceStart, this.selectionEnd);
const start = currentToken?.type === "REFERENCE" ? currentToken.start : this.selectionStart;
this.replaceText(ref, start, this.selectionEnd);
}

/**
Expand Down Expand Up @@ -439,17 +424,6 @@ export abstract class AbstractComposerStore extends SpreadsheetStore {
private getZoneReference(zone: Zone | UnboundedZone): string {
const inputSheetId = this.sheetId;
const sheetId = this.getters.getActiveSheetId();
if (zone.top === zone.bottom && zone.left === zone.right) {
const position = { sheetId, col: zone.left, row: zone.top };
const pivotId = this.getters.getPivotIdFromPosition(position);
const pivotCell = this.getters.getPivotCellFromPosition(position);
const cell = this.getters.getCell(position);
if (pivotId && pivotCell.type !== "EMPTY" && !cell?.isFormula) {
const formulaPivotId = this.getters.getPivotFormulaId(pivotId);
const formula = createPivotFormula(formulaPivotId, pivotCell);
return localizeFormula(formula, this.getters.getLocale()).slice(1); // strip leading =
}
}
const range = this.getters.getRangeFromZone(sheetId, zone);
return this.getters.getSelectionRangeString(range, inputSheetId);
}
Expand Down Expand Up @@ -527,38 +501,21 @@ export abstract class AbstractComposerStore extends SpreadsheetStore {
const colorIndex = this.colorIndexByRange[rangeString];
return colors[colorIndex % colors.length];
};
const highlights: Highlight[] = [];
for (const range of this.getReferencedRanges()) {
return this.getReferencedRanges().map((range) => {
const rangeString = this.getters.getRangeString(range, editionSheetId);
const { numberOfRows, numberOfCols } = zoneToDimension(range.zone);
const zone =
numberOfRows * numberOfCols === 1
? this.getters.expandZone(range.sheetId, range.zone)
: range.zone;
highlights.push({

return {
zone,
color: rangeColor(rangeString),
sheetId: range.sheetId,
interactive: true,
});
}
const activeSheetId = this.getters.getActiveSheetId();
const selectionZone = this.model.selection.getAnchor().zone;
const isSelectionHightlighted = highlights.find(
(highlight) => highlight.sheetId === activeSheetId && isEqual(highlight.zone, selectionZone)
);
if (this.editionMode === "selecting" && !isSelectionHightlighted) {
highlights.push({
zone: selectionZone,
color: "#445566",
sheetId: activeSheetId,
dashed: true,
interactive: false,
noFill: true,
thinLine: true,
});
}
return highlights;
};
});
}

/**
Expand Down
7 changes: 1 addition & 6 deletions src/selection_stream/selection_stream_processor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
Zone,
} from "../types";
import { SelectionEvent, SelectionEventOptions } from "../types/event_stream";
import { CellPosition, Dimension, HeaderIndex, Immutable } from "./../types/misc";
import { CellPosition, Dimension, HeaderIndex } from "./../types/misc";
import { EventStream, StreamCallbacks } from "./event_stream";

type Delta = [number, number];
Expand All @@ -39,7 +39,6 @@ type StatefulStream<Event, State> = {
* Allows to select cells in the grid and update the selection
*/
interface SelectionProcessor {
getAnchor(): Immutable<AnchorZone>;
selectZone(anchor: AnchorZone, options?: SelectionEventOptions): DispatchResult;
selectCell(col: number, row: number): DispatchResult;
moveAnchorCell(direction: Direction, step: SelectionStep): DispatchResult;
Expand Down Expand Up @@ -125,10 +124,6 @@ export class SelectionStreamProcessorImpl implements SelectionStreamProcessor {
this.stream.getBackToDefault();
}

getAnchor() {
return this.anchor;
}

private modifyAnchor(
anchor: AnchorZone,
mode: SelectionEvent["mode"],
Expand Down
Loading

0 comments on commit 88aa2a7

Please sign in to comment.