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.

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

Sticky scrolling in Eclipse #1894

Merged

Conversation

Christopher-Hermann
Copy link
Contributor

@Christopher-Hermann Christopher-Hermann commented May 22, 2024

Description
With this change, sticky scrolling is introduced into Eclipse text editors, as it is available for many other modern IDEs. Sticky scrolling will keep certain source code lines visible and in a fixed position on the screen as the user scrolls down the page. This technique improves user experience by keeping information within reach at all times.

The feature can be enabled via the TextEditor settings.

Provides feature for #1719 and eclipse-jdt/eclipse.jdt.ui#1364

Current state
There are some TODOs left, however the general functionality should work. I'm already creating the PR to get early feedback and maybe there are some volunteer beta testers.

Testing
Enable the feature in the preferences: General > Editors > Text Editors
Check "Enable sticky scrolling"

The sticky lines should be visible in all source/text editors and should work for every language since it is working on line indentation.
stickyScrolling

@Christopher-Hermann Christopher-Hermann marked this pull request as draft May 22, 2024 09:35
Copy link
Contributor

github-actions bot commented May 22, 2024

Test Results

  605 files   -  1 210    605 suites   - 1 210   22m 25s ⏱️ - 1h 8m 42s
7 658 tests +    22  7 426 ✅ +    18  232 💤 +  4  0 ❌ ±0 
8 045 runs   - 16 024  7 812 ✅  - 15 508  233 💤  - 516  0 ❌ ±0 

Results for commit f2c9b50. ± Comparison against base commit 385b30b.

This pull request skips 4 tests.
UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay
org.eclipse.jface.tests.viewers.ListViewerTest ‑ testRevealBug69076
org.eclipse.jface.text.tests.contentassist.ContextInformationTest ‑ testContextInfo_hide_focusOut
org.eclipse.urischeme.internal.registration.TestUnitWinRegistry ‑ testWinRegistry

♻️ This comment has been updated with latest results.

@AObuchow
Copy link
Contributor

Just wanted to say this looks awesome & a great modernization effort for the Eclipse IDE :) Great work

@mickaelistria
Copy link
Contributor

That's a very interesting proposal and the quality already seems good!
What defines which lines are to remain visible? Identation only?

@iloveeclipse
Copy link
Member

What defines which lines are to remain visible? Identation only?

This seem to produce unexpected effects with code formatted like this:

    private void writeObject(java.io.ObjectOutputStream s)
        throws java.io.IOException
    {

see
image

@iloveeclipse
Copy link
Member

iloveeclipse commented May 22, 2024

In general I believe adding first type in the file wastes space / just duplicates already shown information, as the first type is already shown in the editor tab title.

On Linux I see the right side of the "sticky" widget is shown one pixel over the scrollbar, as seen in the screenshot above.

Beside this, I would wish a bit different background for the sticky part, as it is hard to differentiate from actual code (also seen in the screenshot above).

@BeckerWdf
Copy link
Contributor

That's a very interesting proposal and the quality already seems good! What defines which lines are to remain visible? Identation only?

I think yes. How do other IDEs solve this?

@Christopher-Hermann
Copy link
Contributor Author

What defines which lines are to remain visible? Identation only?

This seem to produce unexpected effects with code formatted like this:

    private void writeObject(java.io.ObjectOutputStream s)
        throws java.io.IOException
    {

That's a very interesting proposal and the quality already seems good! What defines which lines are to remain visible? Identation only?

I think yes. How do other IDEs solve this?

Yes, currently it is working with indentation only. At least VS Code does the exact same thing, see the EnumMap example:
Bildschirmfoto 2024-05-23 um 08 21 46

But of course we can try to find a better solution.
On the first look I thought about using the code folding information. But this has some drawbacks as for example Java doesn't provide information for if else statements. Furthermore, this won't work for language that doesn't provide code folding at all.

@mickaelistria
Copy link
Contributor

On the first look I thought about using the code folding information

That's a good idea to get started from.
As we could already notice, indentation is sometimes not sufficient. While folding may not be perfect, its internals are a good inspiration: it does rely on document annotations (which are set by JDT when reconciling/parsing the Java file). A similar strategy allowing document annotations would be interesting here, because the problem "what to stick in" cannot be fully fixed with plain text logic, it seems language-specific and even code-style-specific.
So one possible approach could be to define a new "sticky" document annotation type in Platform and show in the sticky area the lines that have this sticky annotation. Then, language analyzers such as JDT can mark some lines as "sticky" according to language-specific logic. If no line is marked as sticky, indentation can be used as fall-back, with its limits.
What do you think?

@Christopher-Hermann
Copy link
Contributor Author

So one possible approach could be to define a new "sticky" document annotation type in Platform and show in the sticky area the lines that have this sticky annotation. Then, language analyzers such as JDT can mark some lines as "sticky" according to language-specific logic. If no line is marked as sticky, indentation can be used as fall-back, with its limits. What do you think?

Yes, that's a good idea. I will finish the work on the layout and on the fallback logic based on indentation. Afterwards we can have a look how we can implement the annotation logic.

@Madjosz
Copy link

Madjosz commented May 23, 2024

grafik
Eclipse Theia seems to work better in this regard then than VS code as it sticks the correct lines with class and function names in 1TBS as well as in Allman style. The sticking of the if brace seems to be an artifact and only happens after restart. @Christopher-Hermann are you sure this behavior keeps appearing after the file is fully loaded and parsed? I see the same behaviour in Theia during loading of the file.

Regarding redundant class name stick one could argue that you can have multiple non-public top level classes where you want the stick.

Having sticky scroll use the folding functionality sounds like a good approach. Having conditionals not stick yet sounds more like a missing feature on the folding side to me.

@Wittmaxi
Copy link
Contributor

This is a very important feature that eclipse is lacking (and I get asked about a lot!).
I am very glad that somebody is working on it - I am willing to test it, if you need help with that :)

@Christopher-Hermann
Copy link
Contributor Author

On Linux I see the right side of the "sticky" widget is shown one pixel over the scrollbar, as seen in the screenshot above.

It seems that the layout of the scroll bar is different on the different operating systems. It would be nice to get a screenshot from a Windows user as well, so that I can address the issue.

On MacOS the scroll bar is transparent with the latest client:
ScrollBar

Beside this, I would wish a bit different background for the sticky part, as it is hard to differentiate from actual code (also seen in the screenshot above).

Yes you are right, it's hard to see. Any color suggestions? I would like to reuse a existing color, but I cannot find a matching one.
Bildschirmfoto 2024-05-28 um 13 04 18

@Christopher-Hermann
Copy link
Contributor Author

Having sticky scroll use the folding functionality sounds like a good approach. Having conditionals not stick yet sounds more like a missing feature on the folding side to me.

Yes, most probably you are right and this could also be improved in the code folding in general.

However, I propose merging this change as a "beta feature" after addressing layout questions and providing unit tests. Since the feature is disabled by default it should not have any impacts on the daily work. And having it in the official eclipse release will increase the amount of tester significantly. Even I don't use my target IDE in my daily work, so it's hard to test every single editor feature, like quick fixes, etc.

The sticky scrolling based on annotations/code folding can then be implemented on a separate PR.

@Madjosz
Copy link

Madjosz commented May 29, 2024

It would be nice to get a screenshot from a Windows user as well, so that I can address the issue.

This is how it currently looks in Windows 11 light theme without OS scaling and dark theme with 125% OS scaling:
Sticky Scroll in Windows 11
The line number and code alignment is pixel perfect. What I don't like is the yellow background, that is too much.

Overall a very nice outcome, thank you for your efforts!

@Christopher-Hermann
Copy link
Contributor Author

What I don't like is the yellow background, that is too much.

Yes, I played around a litte bit with the colors, but I don't like this as well. I removed the coloring and incorporated a second vertical separator beneath the sticky lines, which improved visibility slightly. However, there is still room for improvement.
image

An other option would be to color the line numbers:
Bildschirmfoto 2024-05-29 um 16 19 58

@Christopher-Hermann Christopher-Hermann force-pushed the sticky_scrolling branch 2 times, most recently from 9eeebb2 to f2fda23 Compare June 10, 2024 11:11
@Christopher-Hermann Christopher-Hermann changed the title [WIP] Sticky scrolling in Eclipse Sticky scrolling in Eclipse Jun 10, 2024
@Christopher-Hermann Christopher-Hermann marked this pull request as ready for review June 10, 2024 11:15
@Christopher-Hermann
Copy link
Contributor Author

The feature/pull request is now ready for review. The sticky lines calculation is fully reliant on the indentation of the source code. I propose we merge it as a "beta" feature to gather preliminary feedback, particularly from users coding in languages other than Java, which was my exclusive testing focus.

For future enhancements, the sticky scrolling functionality can be refined by introducing a sticky lines provider capable of interpreting code folding or working from a new annotation as suggested in a comment above. To allow for broader use across different languages, we could implement this with extension points.

The "final" state of the UI now contains two horizontal separators to make the separation between the sticky lines and the actual source code easier to see. Ideas for improvement are welcome:
Bildschirmfoto 2024-06-10 um 13 06 44

@BeckerWdf
Copy link
Contributor

I propose we merge it as a "beta" feature to gather preliminary feedback, particularly from users coding in languages other than Java, which was my exclusive testing focus.

That's a good idea.

With this change, sticky scrolling is introduced into Eclipse text editors. Sticky scrolling will keep certain source code lines visible and in a fixed position on the screen as the user scrolls down the page. This technique improves user experience by keeping information within reach at all times.

The feature can be enabled via the TextEditor settings.

Provides feature for eclipse-platform#1719 and eclipse-jdt/eclipse.jdt.ui#1364
@Christopher-Hermann
Copy link
Contributor Author

Thanks for your valuable review and input. This helps a lot.

  1. Have you looked into this change in dark mode?
  2. When enabling/disabling breadcrumbs, the sticky scrolling-window is not updated correctly

On MacOS it's working. I need to check how I can fix and test this on windows.
2024-06-10_17-17-30

  1. (optional) Line annotations aren't shown in the sticky lines

You mean the blue coloring, right? This does only makes sense for the last sticky line, since all other sticky lines will be related to another source code part. Not sure if it is worth to invest here.

  1. Why is there a cap on the amount of sticky lines? (I agree that >10 is likely not practical)

Yes, it seemed to be unnecessary to have more than 10 lines. Of course we could remove this limitation, but on the other hand this goes in the same direction as your 6. comment.

@mickaelistria mickaelistria added enhancement New feature or request noteworthy Noteworthy feature usability labels Jun 10, 2024
@mickaelistria mickaelistria added this to the 4.33 M1 milestone Jun 10, 2024
@mickaelistria
Copy link
Contributor

I'm merging this right now because the current code and feature quality are already good and it's the right time to push such experimental features in.
@Wittmaxi Can you please open dedicated issues for the point you've mentioned above that still require some improvements?

@mickaelistria mickaelistria merged commit 577fcac into eclipse-platform:master Jun 10, 2024
9 of 11 checks passed
@Christopher-Hermann Christopher-Hermann deleted the sticky_scrolling branch June 10, 2024 19:15
@mickaelistria mickaelistria linked an issue Jun 10, 2024 that may be closed by this pull request
Christopher-Hermann added a commit to Christopher-Hermann/eclipse.platform.ui that referenced this pull request Jun 10, 2024
The calculation of the sticky-scrolling lines is now throttled, executing only every 100 ms, to prevent potential performance bottlenecks.

Follow up to eclipse-platform#1894
Christopher-Hermann added a commit to Christopher-Hermann/eclipse.platform.ui that referenced this pull request Jun 11, 2024
The calculation of the sticky-scrolling lines is now throttled, executing only every 100 ms, to prevent potential performance bottlenecks.

Follow up to eclipse-platform#1894
mickaelistria pushed a commit that referenced this pull request Jun 11, 2024
The calculation of the sticky-scrolling lines is now throttled, executing only every 100 ms, to prevent potential performance bottlenecks.

Follow up to #1894
@jukzi
Copy link
Contributor

jukzi commented Jun 11, 2024

I need to get used to it, but looks promising. Just annoying that it feels like the row count is sometimes +-1 line off
Always for (empty) lines after javadoc before a method:
image

In this example it is unsure to which blockstart the block belongs, depends how much pixels exactly i scroll:
image
image

Christopher-Hermann added a commit to Christopher-Hermann/eclipse.platform.ui that referenced this pull request Jun 13, 2024
Scrolling on the sticky lines control is dispatched to the linked source viewer. With this change the source viewer's scrolling behavior remains consistent, regardless of whether sticky scrolling is enalbed or not.

Follow up for eclipse-platform#1894
Christopher-Hermann added a commit to Christopher-Hermann/eclipse.platform.ui that referenced this pull request Jun 13, 2024
Scrolling on the sticky lines control is dispatched to the linked source viewer. With this change the source viewer's scrolling behavior remains consistent, regardless of whether sticky scrolling is enalbed or not.

Follow up for eclipse-platform#1894
Christopher-Hermann added a commit to Christopher-Hermann/eclipse.platform.ui that referenced this pull request Jun 13, 2024
Scrolling on the sticky lines control is dispatched to the linked source viewer. With this change the source viewer's scrolling behavior remains consistent, regardless of whether sticky scrolling is enalbed or not.

Follow up for eclipse-platform#1894
Christopher-Hermann added a commit to Christopher-Hermann/eclipse.platform.ui that referenced this pull request Jun 13, 2024
Scrolling on the sticky lines control is dispatched to the linked source viewer. With this change the source viewer's scrolling behavior remains consistent, regardless of whether sticky scrolling is enalbed or not.

Follow up for eclipse-platform#1894
BeckerWdf pushed a commit that referenced this pull request Jun 14, 2024
Scrolling on the sticky lines control is dispatched to the linked source viewer. With this change the source viewer's scrolling behavior remains consistent, regardless of whether sticky scrolling is enalbed or not.

Follow up for #1894
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request noteworthy Noteworthy feature usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sticky Option for breadcrumb / additional to breadcrumb
8 participants