Skip to content

[JEWEL-819] Fix markdown delete row indexes #3051

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

obask
Copy link
Collaborator

@obask obask commented May 18, 2025

There are two separate changes:

1. Minimal Fix for inserts after Leading Whitespace in JEWEL-819:

The first change addresses a bug caused by leading whitespace before the first block. Previously, this whitespace was ignored, resulting in an incomplete string being selected for the updated text.

The fix allows the index of the first edited block to start from -1. This effectively treats leading whitespace as space between blocks, ensuring correct selection. A regression test has been added to verify this behavior.

2. Normalization of End Range Indexing:

The second change applies a similar idea to how end range indexes are interpreted. End indexes are now always positioned after the last character of the corresponding block(or an edited space between blocks). This means they may point just beyond the last element in a list, improving consistency when referencing “after block” positions.

This update relies on the source span length to compute a precise end index position(assuming these are correct).

New tests have been added to cover several edge cases:

  • Inserting a line between paragraphs,

  • Inserting a character at the end of a paragraph, Between two paragraphs, At the start of the second paragraph.

It was caused by incorectly selecting a range from 0 to 0 of
edited blocks which resulted in ignoring chars before the first block.

Allowing edit computation to start from -1 removes this problem.
@obask obask self-assigned this May 18, 2025
@obask obask requested review from rock3r and AlexVanGogen May 18, 2025 18:38
@obask
Copy link
Collaborator Author

obask commented May 18, 2025

@rock3r
Copy link
Collaborator

rock3r commented May 18, 2025

Any chance you could add a short recap of the fix in the PR description?

@obask obask force-pushed the fix-mardown-delete-row-indexes branch from 964fc2c to 71ca1c2 Compare May 19, 2025 09:41
@obask
Copy link
Collaborator Author

obask commented May 19, 2025

run the pre-push run config and push again

It made some changes to the UI package. I assume it's ok to include these into my change?

@rock3r
Copy link
Collaborator

rock3r commented May 19, 2025

That's fine. It's probably something that was supposed to be fixed on master, but slipped through because the TeamCity CI does not run Gradle checks yet (see JEWEL-800).

@obask obask force-pushed the fix-mardown-delete-row-indexes branch 2 times, most recently from ec2ed30 to 0f44adb Compare May 19, 2025 09:58
@obask obask force-pushed the fix-mardown-delete-row-indexes branch from 0f44adb to e31cde9 Compare May 19, 2025 10:18
Copy link
Collaborator

@rock3r rock3r left a comment

Choose a reason for hiding this comment

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

LGTM for what I can understand of the logic, and seems to work fine when tested, but leaving to @AlexVanGogen to confirm

Oleg Baskakov added 2 commits May 19, 2025 11:37
...such as inserting a line between paragraphs.

Also use more uniform indexes to represent after
block position even if it points to the element of
a list following the last.
@obask obask force-pushed the fix-mardown-delete-row-indexes branch from e31cde9 to c3f69d9 Compare May 19, 2025 10:39
Copy link
Contributor

@AlexVanGogen AlexVanGogen left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@rock3r
Copy link
Collaborator

rock3r commented May 29, 2025

Ready to merge

@rock3r rock3r added the Jewel label May 29, 2025
@AlexVanGogen
Copy link
Contributor

Pushed manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants