Skip to content

Conversation

LucasLefevre
Copy link
Collaborator

Steps to reproduce:

  • type a long formula, e.g. =SUM(11111111, 22222222, 33333333, 44444444, 55555555, 66666666, 77777777, 88888888)
  • in the top bar composer, click somewhere that won't be on the first line when prettified (in the fours)

=> the cursor is badly positioned on the prettified formula

Task: 5095470

Description:

description of this task, what is implemented and why it is implemented that way.

Task: TASK_ID

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Sep 17, 2025

Pull request status dashboard

@LucasLefevre LucasLefevre force-pushed the 19.0-prettified-cursor-lul branch from 05565e2 to 0658513 Compare September 17, 2025 14:03
Comment on lines -154 to -157
const content = text || this.getComposerContent(this.getters.getActivePosition());
const validSelection = this.isSelectionValid(content.length, selection.start, selection.end);
if (!validSelection) {
return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks like a safeguard against a programming error somewhere else, calling this with wrong values.
Keeping this check would mean making it more complex for ... not a lot...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the mostly pretty fix I've written...

Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧇 👋

Comment on lines +220 to +222
if (char !== "\n" && char !== "\t") {
originalIndex++;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check the prettifier feature too closely, but are we sure that there are no tabs/newlines in the formula before the prettification ? Otherwise your fix wo'nt work

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure that there are no tabs/newlines in the formula before the prettification

yes, because we explicitly display it in a single line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's quite fragile and confusing to understand, I admit. because the cursor is linked to the one-liner that is displayed in the composer, but the content comes from somewhere else (the cell)

@LucasLefevre LucasLefevre force-pushed the 19.0-prettified-cursor-lul branch from 0658513 to 0cf12d7 Compare October 8, 2025 10:44
Steps to reproduce:
- type a long formula, e.g. =SUM(11111111, 22222222, 33333333, 44444444, 55555555, 66666666, 77777777, 88888888)
- in the top bar composer, click somewhere that won't be on the first line
  when prettified (in the fours)

=> the cursor is badly positioned on the prettified formula

Task: 5095470
@LucasLefevre LucasLefevre force-pushed the 19.0-prettified-cursor-lul branch from 0cf12d7 to d534336 Compare October 8, 2025 10:44
@LucasLefevre
Copy link
Collaborator Author

@rrahir fixed :)

@rrahir
Copy link
Collaborator

rrahir commented Oct 8, 2025

thanks :)

robodoo r+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants