Skip to content

Conversation

danthe1st
Copy link
Contributor

@danthe1st danthe1st commented Jun 29, 2025

This is my attempt to fix #3073 in order to implement eclipse-jdt/eclipse.jdt.ui#2264

Description

ProjectionViewer currently cannot use visible regions and projections together (except by calling enableProjections() after setVisibleRegions in which case it will show a wrong region). This PR changes the implementation of visible regions to collapse everything outside the visible region.

Note

This PR does not allow collapsing or expanding any projection/folding regions outside of the declared visible region (e.g. by other code modifying the document in some way or if a projection region is partially in the visible region).

Aside from that, this implementation has the following effect: If setVisibleRegion is called with a region that is fully collapsed, the editor is basically empty. If another behavior is desired, it would be good to know what the better behavior is.

testing with JDT

To test this with JDT as requested in eclipse-jdt/eclipse.jdt.ui#2264, do the following:

  • Create a workspace with eclipse.platform.ui and JDT-UI set up (I did so by creating a Target Platform and added whatever I thought was necessary until I no longer got any errors but I have no idea how to properly do that)
  • Use the changes in eclipse.platform.ui
  • Apply enable folding with "Only show selected Java element" eclipse-jdt/eclipse.jdt.ui#2302 with JDT-UI:
  • Create an "Eclipse Application" run configuration with this version of Eclipse Platform and the patched version of JDT-UI and start it
  • Enable Window > Preferences > Java > Editor > "Only show the selected Java Element"
  • Make sure folding is enabled at Window > Preferences > Java > Editor > Folding
  • Open a Java class
  • Select elements in the outline and observe that only the "selected element" is shown in the editor and folding still works (whether it's normal folding, custom folding regions, "Extended Folding", etc shouldn't make a difference)
    image
  • If "Enhanced Folding" is active, make sure that preserve contextual information with enhanced folding eclipse-jdt/eclipse.jdt.ui#2410 is available as well

Copy link
Contributor

github-actions bot commented Jul 3, 2025

Test Results

 2 778 files  ± 0   2 778 suites  ±0   1h 36m 42s ⏱️ - 7m 29s
 7 943 tests +11   7 714 ✅ +11  228 💤 ±0  1 ❌ ±0 
23 384 runs  +33  22 637 ✅ +33  746 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit da38e3d. ± Comparison against base commit 477d106.

♻️ This comment has been updated with latest results.

@iloveeclipse
Copy link
Member

@danthe1st : thanks for PR, few things:

  1. Please rebase on master
  2. Please squash two commits into one
  3. Please update commit message to follow commit message guidelines: https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#commit-message-recommendations

@danthe1st
Copy link
Contributor Author

danthe1st commented Jul 3, 2025

@danthe1st : thanks for PR, few things:

1. Please rebase on master

2. Please squash two commits into one

3. Please update commit message to follow commit message guidelines: https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#commit-message-recommendations

I'll make sure to do that once I located and fixed a remaining issue in combination with JDT (if I select a method that's within a custom folding region and enter text, it seems to show "too much" text for some reason). While I think that's an issue with JDT-UI (it only seems to happen with custom folding regions and extended folding), I want to be sure about it first (and I opened this PR before everything is finished to make it visible in advance/allow for discussion if applicable).

@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from bb4f64a to 5274f99 Compare July 5, 2025 12:48
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 5, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st
Copy link
Contributor Author

danthe1st commented Jul 5, 2025

I have made the requested changes. However, I want to note the following:

  • This has an impact on behavior(al compatibility). If some project/plugin/whatever calls enableProjections() and setVisibleRegion on some ProjectionViewer, this PR affects what is shown to the user.
  • I didn't do performance testing.
  • I think it would be a good idea for someone from JDT (or other projects if make use of this?) to test it before this is merged.

danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 5, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from 5274f99 to 5b7cb97 Compare July 5, 2025 14:40
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 5, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from 5b7cb97 to fb861bc Compare July 5, 2025 14:59
@danthe1st
Copy link
Contributor Author

danthe1st commented Jul 5, 2025

I am not sure why the Jenkins build seems to have issues getting tycho set up (maybe issues with https://repo.eclipse.org/?)

@merks
Copy link
Contributor

merks commented Jul 5, 2025

Yes there are infrastructure problems again:

I restarted the build.

danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 5, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from fb861bc to c5fb5f3 Compare July 5, 2025 17:24
@merks
Copy link
Contributor

merks commented Jul 5, 2025

It looks like there was a test failure:

https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/PR-3074/lastCompletedBuild/testReport/

danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 5, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from c5fb5f3 to f34844d Compare July 5, 2025 18:28
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 5, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from f34844d to 7ff50f2 Compare July 5, 2025 19:00
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 6, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from 7ff50f2 to 2ce3ba0 Compare July 6, 2025 08:06
@danthe1st
Copy link
Contributor Author

I now switched to an approach that should have fewer side effects. Instead of adding additional projection regions, I am now using the expand() and collapse() methods directly.

danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 6, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch 2 times, most recently from d2f4f5a to 7a9cf09 Compare July 6, 2025 10:51
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Jul 6, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st
Copy link
Contributor Author

danthe1st commented Aug 22, 2025

It's possible this hasn't been merged until now because I didn't care enough to ask for a re-review 😂 (I guess I should rebase and do that after the 2025-09 release)

But how is that possible since when you invoke show only selected element you only see the code of the selected element on the file you are coding lets call the file file a. how can you expand on something outside of selected element on file a ? unless you are saying that if you expand a region on a different file lets call it file b then it will disable the show only selected element on file a ?

This PR is about the change in platform.ui which provides the APIs relevant for that. If a client decides they want to show projection regions in a way that these are not entirely within or outside of folding regions, they can do so (and in fact, this situation can also occur with custom folding regions where one comment is inside the visible region and the other isn't). This decision is something I'd like to get the opinions of the committers/possibly other people.

Regarding the tests, I think there are still 3 that didn't run with the current version yet and IIRC some tests failed there with MacOS on previous versions (and I don't have a MacOS device to fix these issues if they originate from this PR). These tests would likely be executed when a committer reviews it (or clicks on the button to approve the test run which has to be redone whenever a new version is pushed because I haven't contributed to platform.ui before).

Also I don't know how big the effects of this PR are on other code based on Eclipse that isn't JDT (e.g. companies building RCP applications).

Do you think there is any chance this feature will be merged in the next version of Eclipse after 202509 like 202512 ?

¯\_(ツ)_/¯

@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from f1ed5b3 to 5be70fa Compare September 2, 2025 15:54
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Sep 2, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st
Copy link
Contributor Author

@iloveeclipse Would you like to re-review this (and eclipse-jdt/eclipse.jdt.ui#2302) at some point when you have time? I think I forgot to mention that I have made the requested changes.

@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from b9af163 to 5667d40 Compare September 12, 2025 17:14
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Sep 12, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Sep 13, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from 5667d40 to 440ad1b Compare September 13, 2025 11:16
@danthe1st
Copy link
Contributor Author

danthe1st commented Sep 13, 2025

As I didn't get any feedback until now, I made the following choice (can still be changed if there are good reasons for it):

Since it is possible for changes outside of the visible region to happen (e.g. using custom actions, other code adding projection regions or anything else that might modify the document) and users wouldn't be interested to see stuff outside of the visible region, I decided to change the logic to ignore collapsing and expanding outside of the visible region.

I also adjusted the description accordingly.

This PR does not allow collapsing or expanding any projection/folding regions outside of the declared visible region (e.g. by other code modifying the document in some way or if a projection region is partially in the visible region).

@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from 440ad1b to 16c61a9 Compare September 13, 2025 11:45
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Sep 13, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@laeubi
Copy link
Contributor

laeubi commented Sep 25, 2025

@mickaelistria can you help reviewing this?

@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from 16c61a9 to 1a79bb7 Compare September 25, 2025 14:27
danthe1st added a commit to danthe1st/eclipse.platform.ui that referenced this pull request Sep 25, 2025
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
While ProjectionViewer supports both using visible regions and
projections, these features cannot be used in conjunction. This change
allows the use of projections when visible regions are used.

Fixes eclipse-platform#3074
@danthe1st danthe1st force-pushed the projections-with-visible-regions branch from 1a79bb7 to 92394ce Compare October 6, 2025 16:24
@danthe1st
Copy link
Contributor Author

It seems like this PR missed M1 now. Would it be possible to get this reviewed before M2 (with sufficient time left for eclipse-jdt/eclipse.jdt.ui#2302)?
@mickaelistria

Or would it be better to wait for another release cycle (2026-03)? But even in that case, it would be better to get this reviewed at some point.

@mickaelistria
Copy link
Contributor

I won't review in details but this patch adds many good tests, is actually a fix, has no new API and all payload changes are internal and isolated; and code is understandable enough. So let's merge and worth case, we can revert it.

@mickaelistria mickaelistria merged commit 1848058 into eclipse-platform:master Oct 7, 2025
8 checks passed
@iloveeclipse
Copy link
Member

This PR caused regression: #3380
@mickaelistria, @danthe1st : could you please check?

@iloveeclipse
Copy link
Member

This PR caused another regression: eclipse-jdt/eclipse.jdt.ui#2532

@mickaelistria, @danthe1st : please follow up. The way the listener is added & removed is not OK, as it will be not always removed. A simple solution is doable, I've posted it on the bug, but I'm not sure if that is enough.

@mickaelistria
Copy link
Contributor

@danthe1st @iloveeclipse I think we should give about a week for a fix to be pushed, and if nothing has happened before that, we can revert.

@cdietrich
Copy link
Contributor

cdietrich commented Oct 13, 2025

causes breaking change in Xtext comment editing strategy eclipse-xtext/xtext#3531

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.

Allow external visible regions in ProjectionViewer

7 participants