Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Formatting the whole document makes a big mess #2437

Closed
kaloyan-raev opened this issue Sep 13, 2016 · 3 comments
Closed

Formatting the whole document makes a big mess #2437

kaloyan-raev opened this issue Sep 13, 2016 · 3 comments
Assignees
@evidolob
Labels
kind/bug Outline of a bug - must adhere to the bug report template. sprint/current

Comments

@kaloyan-raev
Copy link
Contributor

kaloyan-raev commented Sep 13, 2016 •
edited
Loading

Formatting a JSON document of average size makes a big mess. See the animated gif.

I have a local build of Che with the latest content from the language-server branch.

I see similar result with PHP files using a PHP language server. So, it seems to be a generic formatting issue when using language servers, i.e. related to how #1788 is implemented.

formatting-mess

Reproduction Steps:

  1. Create a workspace with project from this GitHub repo: https://github.com/kaloyan-raev/AstroSplash.git
  2. Open the composer.json file
  3. Invoke Edit > Format from the main menu

Expected behavior:

The JSON file is nicely formatted.

Observed behavior:

The document is completely broken.

Che version: latest content from the language-server branch
OS and version: Fedora 24
Docker version: 1.10.3
Che install: local build, started with the bin/che.sh script

Additional information:

  • Problem started happening recently, didn't happen in an older version of Che: [No]
  • Problem can be reliably reproduced, doesn't happen randomly: [Yes]
@ddementieva
Copy link
Contributor

@kaloyan-raev Bug confirmed.

@ddementieva ddementieva added the kind/bug Outline of a bug - must adhere to the bug report template. label Sep 14, 2016
@kaloyan-raev kaloyan-raev mentioned this issue Sep 26, 2016
Closed
34 tasks
@kaloyan-raev
Copy link
Contributor Author

I did some debugging. The text edits returned by the language server are all calculated based on the document before any modification is applied.

Che applies the edits to the document one by one. If the inserted text has a different length than the replaced text then the ranges of the remaining edits become invalid and they must be shifted appropriately.

I am working on a PR to fix this issue.

kaloyan-raev added a commit to kaloyan-raev/che that referenced this issue Oct 5, 2016
@kaloyan-raev
Fixes eclipse-che#2437: Shift remaining ranges when applying multi-ed…
21159c2 
…it formatting

A formatting request to a language server may result in a list of
multiple text edits. The ranges of all these text edits are based on the
yet unmodified document. Che applies the edits to the document one by
one. If the inserted text has a different length than the replaced text
then the ranges of the remaining edits become invalid and they must be
shifted appropriately.

This patch shifts the ranges as necessary. To achieve this, it first
converts the ranges based on lines and characters to linear ranges,
which are easier for shifting.

Signed-off-by: Kaloyan Raev <kaloyan.r@zend.com>
@kaloyan-raev kaloyan-raev mentioned this issue Oct 5, 2016
Merged
1 task
@kaloyan-raev
Copy link
Contributor Author

Pushed PR #2719 with the fix.

JPinkney pushed a commit to JPinkney/che that referenced this issue Aug 17, 2017
@kaloyan-raev
Fixes eclipse-che#2437: Apply multi-edit formatting without messing u…
0051c43 
…p the document (eclipse-che#2719)

* Fixes eclipse-che#2437: Shift remaining ranges when applying multi-edit formatting

A formatting request to a language server may result in a list of
multiple text edits. The ranges of all these text edits are based on the
yet unmodified document. Che applies the edits to the document one by
one. If the inserted text has a different length than the replaced text
then the ranges of the remaining edits become invalid and they must be
shifted appropriately.

This patch shifts the ranges as necessary. To achieve this, it first
converts the ranges based on lines and characters to linear ranges,
which are easier for shifting.

Signed-off-by: Kaloyan Raev <kaloyan.r@zend.com>

* Simplify the shifting logic

Signed-off-by: Kaloyan Raev <kaloyan.r@zend.com>

* Reworked: just applying the text edits backwards is enough
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@evidolob evidolob

Labels
kind/bug Outline of a bug - must adhere to the bug report template. sprint/current
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@kaloyan-raev @evidolob @vparfonov @ddementieva