Skip to content

Conversation

danthe1st
Copy link
Contributor

@danthe1st danthe1st commented Oct 9, 2025

Because this part of the codebase isn't frequently modified, I'll try to describe this in a way that is reasonable to understand without being too familiar with these parts.

Description of the change

ProjectionViewer/ProjectionDocument allows hiding ranges/regions within the document.
When a region is removed/hidden, it keeps an empty range which is used when e.g. typing text at that position (if there is no visible region left, it should still be possible to type new text).
Currently, this range is put at the end of the removed range causing typed letters to be inserted at the end of the last removed range.
This PR changes this behavior to put the zero-length range at the beginning of the "removed" range if the entire range is "removed" (from being visible) and the removed range ends at the end of the file (so if you remove the last part of the file and the part before the removed range is removed already).

The offsetInMaster != 0 && offsetInMaster + lengthInMaster == getMasterDocument().getLength() part of the condition is my attempt of not changing any behavior that is not wanted/keeping impact as low as possible:

  • If the beginning of the file (or the whole file most likely) is hidden with a single call, it keeps the old behavior (it leaves the position for typing at the end of the range)
  • It is only applicable when the end of the file is removed because someone hiding the last part of the file shouldn't want the user to continue typing at the end of the file.

Reasoning

With my previous change in #3074, ProjectionViewer#setVisibleRegion is based on projection regions if projections are enabled (to allow using these two features together). This caused a regression (#3380) which resulted in setting an empty "visible region" to cause entered text appear at the end (failing SegmentedModeTest#testSegmentation).

Other considerations

This doesn't fix the issue that (assuming projections are enabled for the editor) setting a visible region still allows editing text at the end of the file.

For example, if projections are enabled for the editor and setVisibleRegion is used to only show the X class, it is possible to type after the end of the Y class by moving the cursor to the very end:

class X {

}

class Y {

}

The code responsible for that is in the else block but I wasn't (yet) able to make that work.

If this change is considered too risky/invasive (by changing behavior for people relying on removing the end region - though I have no idea why someone would rely on that), I can understand still reverting #3074 instead of merging this PR.

Also note that I cannot run the tests locally (I think because of the JUnit 6 issue).

Fixes #3380

Copy link
Contributor

github-actions bot commented Oct 9, 2025

Test Results

 3 018 files  ±0   3 018 suites  ±0   2h 31m 24s ⏱️ + 6m 13s
 8 229 tests +3   7 980 ✅ + 4  249 💤 ±0  0 ❌  - 1 
23 607 runs  +9  22 813 ✅ +10  794 💤 ±0  0 ❌  - 1 

Results for commit a327306. ± Comparison against base commit 4f771f0.

♻️ This comment has been updated with latest results.

@iloveeclipse
Copy link
Member

Also note that I cannot run the tests locally (I think because of the JUnit 6 issue).

This should be now fixed on master, so I've rebased the branch.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a text insertion behavior in ProjectionDocument when removing ranges at the end of file. When a region is removed/hidden and results in an empty range at EOF, the empty range is now positioned at the start of the removed range instead of at the end, ensuring text typed at that position appears in the correct location.

Key changes:

  • Modified the fragment offset positioning logic when removing master document ranges
  • Added condition to detect when removed range ends at EOF and reposition empty fragments accordingly

@danthe1st danthe1st force-pushed the collapse-all branch 2 times, most recently from 14e6f5f to 5712747 Compare October 10, 2025 14:50
@danthe1st
Copy link
Contributor Author

I also added additional tests for the behavior in this PR.

@danthe1st danthe1st force-pushed the collapse-all branch 3 times, most recently from fe231df to f4b89fe Compare October 13, 2025 15:50
@danthe1st
Copy link
Contributor Author

danthe1st commented Oct 13, 2025

@iloveeclipse ping as the PR is ready for re-review so the test stops failing btw

@iloveeclipse
Copy link
Member

ping as the PR is ready for re-review so the test stops failing btw

Could you please check eclipse-xtext/xtext#3531 ?

@danthe1st
Copy link
Contributor Author

Given the amount of issues it caused, it may be better to revert #3074.
Nevertheless, I guess I can try to look at the xtext issue if I have time (though I have no experience with it at all).

@iloveeclipse
Copy link
Member

Given the amount of issues it caused, it may be better to revert #3074. Nevertheless, I guess I can try to look at the xtext issue if I have time (though I have no experience with it at all).

4.38 M2 is planned for October 24. It would be nice to have a fix PR a day before that or a revert PR, if there will be no fix.

See also comment from @cdietrich regarding the paerticular problem with Xtext: eclipse-xtext/xtext#3531 (comment)

@danthe1st danthe1st force-pushed the collapse-all branch 2 times, most recently from 56c86aa to 7e8ec0d Compare October 15, 2025 16:52
@danthe1st
Copy link
Contributor Author

danthe1st commented Oct 15, 2025

I have updated the code in an attempt to fix eclipse-xtext/xtext#3531.

I changed fVisibleRegionDuringProjection to track all changes to visible regions to ensure enableProjection calls setVisibleRegion with the actual region set before (or doesn't call it if setVisibleRegion wasn't called before). This requires the UpdateDocumentListener to be always registered (technically it would be possible to only register it when a visible region is set but I didn't do that here) which matches the idea I mentioned in eclipse-xtext/xtext#3531 (comment).

The change to overlapsWithNonVisibleRegions is probably best explained with an example. Let's say that [] describes a folding region and <> represents the visible region:

[    <text
    text
>    ] text

In this case, it should still be possible to collapse the part in [] folded because the [ and < are in the same line (and the same for the ends) even though it exceeds the bounds of the visible region.

I have no idea whether the GH Actions test failures are related to my PR.

CC @iloveeclipse
If you don't feel comfortable merging that change shortly before M2, I can also provide a revert PR and try again for 2026-03.

@danthe1st
Copy link
Contributor Author

Oh also it would probably be good if someone could verify whether this actually fixes eclipse-xtext/xtext#3531
CC @cdietrich

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.

SegmentedModeTest.testSegmentation fails since I20251007-1800 on all platforms

2 participants