-
Notifications
You must be signed in to change notification settings - Fork 223
Allow using visible regions with projections #3074
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
base: master
Are you sure you want to change the base?
Allow using visible regions with projections #3074
Conversation
48e4b9a
to
88d2623
Compare
Test Results 2 778 files ± 0 2 778 suites ±0 1h 36m 42s ⏱️ - 7m 29s 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. |
@danthe1st : thanks for PR, few things:
|
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). |
bb4f64a
to
5274f99
Compare
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
I have made the requested changes. However, I want to note the following:
|
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
5274f99
to
5b7cb97
Compare
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
5b7cb97
to
fb861bc
Compare
I am not sure why the Jenkins build seems to have issues getting tycho set up (maybe issues with https://repo.eclipse.org/?) |
Yes there are infrastructure problems again:
I restarted the build. |
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
fb861bc
to
c5fb5f3
Compare
It looks like there was a test failure: https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/PR-3074/lastCompletedBuild/testReport/ |
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
c5fb5f3
to
f34844d
Compare
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
f34844d
to
7ff50f2
Compare
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
7ff50f2
to
2ce3ba0
Compare
I now switched to an approach that should have fewer side effects. Instead of adding additional projection regions, I am now using the |
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
d2f4f5a
to
7a9cf09
Compare
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
1756109
to
a783bc4
Compare
Thank you for your review. I made the requested changes and updated the description. |
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
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
a783bc4
to
4327dc9
Compare
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
4327dc9
to
bdd39d0
Compare
Just a hint: for all the comments you have resolved, please use "Resolve Conversation" button so it is easier to see what is left open. |
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
bdd39d0
to
c5c5500
Compare
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Outdated
Show resolved
Hide resolved
...eclipse.jface.text/projection/org/eclipse/jface/text/source/projection/ProjectionViewer.java
Show resolved
Hide resolved
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
c5c5500
to
c5c2977
Compare
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
c5c2977
to
da38e3d
Compare
I am not sure in what ways my changes are related (assuming they are related) to the test failure on MacOS (and I don't have a MacOS device to test it on). It might be related to #294 |
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
da38e3d
to
aab9218
Compare
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
aab9218
to
f1ed5b3
Compare
Any chance this feature be available in the next Eclipse version 2025-09 ? |
I don't think so (and I don't think it would be a good idea even if that was reviewed and deemed ok content-wise now) since we are approaching RC1 and as this change could impact other parts of the IDE and maybe also RCP application, it would be ideal if it was merged before even M1 (so if there are issues originating from it, there is enough time to fix them before a release). That being said, I think the implementation is at a point where it can be re-reviewed, especially since this bugfix has been merged. One thing I'm not quite sure about (yet) is the following note in the PR description:
I (think I) could make it so that expanding folding regions that may have parts outside of the visible regions just ignores these parts. |
Thank you for your reply and all the great work you have done so far You said 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 ? Do you think there is any chance this feature will be merged in the next version of Eclipse after 202509 like 202512 ? |
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)
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).
¯\_(ツ)_/¯ |
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 callingenableProjections()
aftersetVisibleRegions
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
I also didn't test it (much) with regions overlapping with the bounds of the visible region.
If needed, I can change it to try to make sure that parts outside of the visible regions cannot be expanded.
testing with JDT
To test this with JDT as requested in eclipse-jdt/eclipse.jdt.ui#2264, do the following:
Is there a way to launch an Eclipse installation like that from the command line? If yes, @totomomo18 might want to test it.
Note
Expanding a folding region outside the currently visible region will make that folding region visible. If wanted, I can change that.