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

Add styling enhancements to signatures (JavaElementLinks) in Javadoc. Fixes #1073 #1074

Merged
merged 15 commits into from
Mar 26, 2024

Conversation

RedeemerSK
Copy link
Contributor

@RedeemerSK RedeemerSK commented Jan 15, 2024

What it does

Adds various enhancements to styling / formatting of signatures in Javadoc presentation (HTML-based) views:

  • togglable text wrapping (method parameters, generics type parameters used in method)
  • togglable text formatting (generics type parameters used, method parameter names, method name, method class)
  • togglable generic type parameters references text coloring
  • togglable generic type parameters nesting levels text coloring
  • togglable enhancement states: OFF / always ON / ON only on mouse hover over new styling button
  • toggable enhancements state preferences saved separately for Javadoc hover view and Javadoc view, color preferences are shared
  • styling button also acts as a menu where all aspects of enhancements can be configured : togglable states, color preferences
  • defines 4 default colors for type parameters references / levels coloring, further references / levels re-use them repratedly by default
  • supports Dark mode (theme) by defining separate default colors and gracefully handling switching themes

Examples:
Default enhancements:
javadoc_new
When hovered on styling button
javadoc_new_colored

Defaults for colors:
lightMode-colors-withFormatting
darkMode-colors-withFormatting

Changing settings example:
preview_overlay

colorPreferencesSubmenu-6colors
colorPreferencesSubmenu-0colors

See Eclipse JDT forums topic I created to communicate goals, approach, decisions taken and progress.

How to test

At this stage of WIP prototype:
Have Javadoc for various Java source code elements displayed in both Javadoc hover (viewer) and Javadoc view. Interact with new styling menu button (added to toolbars) to form assumptions about what new visual items do & see if things then behave as one expects them to behave.

Author checklist

Changes are currently WIP and thus at this stage not meant for thorough code-review, but rather for other people to be able to try them and provide feedback & future code-reviewer to have a look at how changes are implemented.

@akurtakov
Copy link
Contributor

akurtakov commented Jan 16, 2024

Is this one still WIP as the title says? I guess so as it's not building and test are not passing. This is the minimum to be done before the patch can be considered for review.

@RedeemerSK RedeemerSK force-pushed the issue_1073 branch 3 times, most recently from f5c90e6 to 3665d0a Compare January 16, 2024 23:01
@RedeemerSK
Copy link
Contributor Author

RedeemerSK commented Jan 16, 2024

Is this one still WIP as the title says? I guess so as it's not building and test are not passing. This is the minimum to be done before the patch can be considered for review.

Failing tests are fixed now. This PR is labeled WIP since the nature of my changes (enhancements) is such that I would first like to get some feedback from other person / people on current prototype before requesting actual merge.

@RedeemerSK
Copy link
Contributor Author

RedeemerSK commented Jan 17, 2024

List of enhancements:

Formatting enhancements:

  1. method name is made to visually stand out by highest CSS font-weight being bold
  2. method qualifier (encompassing class) is made visually less significant by lowering CSS opacity
  3. method return type is made to visually stand out by making text italic
  4. method parameters names are made to visually stand out by making text italic
  5. generic type parameters are made visually less significant by lowering CSS font-weight
  6. opening '<' and closing '>' of generic type parameters sections are made to visually stand-out by highest CSS font-weight being bold
  7. opening '<' of nested generic type parameters sections are offset by single space
  8. method parameters openning '(' is offset by space

Wrapping enhancements:

  1. method parameters are displayed on separate lines
  2. prepended method generic type parameters are displayed on separate 1st line, therefore following method qualifier + method name are displayed on separate line

Type parameters references coloring enhancements:

  1. each unique type parameter (class if type is bound) can be collored with different configurable color
  2. type parameter specifying uppper/lower wildcard bounding is colored in whole (including '? extends' / '? super') but is considered as if there was no bounding, just base (bounded) type
  3. there are default colors for first 4 unique references, they are also re-used repeatedly for further unique references
  4. light theme and dark theme have different default colors
  5. color defaults and preferences are shared with type parameters (nesting) levels coloring enhancements

Type parameters (nesting) levels coloring enhancements:

  1. each unique type parameters nesting level can be collored with different configurable color
  2. there are default colors for first 4 levels, they are also re-used repeatedly for further levels
  3. light theme and dark theme have different default colors
  4. color defaults and preferences are shared with type parameters references coloring enhancements

To notice / consider / think about

Things that I would like to point out to have somebody else to notice & think about:

  1. default preferences for both Javadoc hover & view: formatting ON / wrapping ON/ type parameters references coloring ON_HOVER / type parameters (nesting) levels coloring OFF
  2. enhancements toggling could be done also when hovering (somewhere) inside the Javadoc content itself, but I kept it just when hovering over new styling button in toolbar
  3. should we be worried about this being the 1st place to have names of classes uniquely colored ? may it later cause unreasonable expectations / requests by the users regarding other places (e.g code editor) ?
  4. type parameters (nesting) levels coloring - do you think it's justified to keep it included as it may be eventually used by some users ?
  5. for icons I re-used existing ones that I found most fitting
  6. there are color defaults for first 4 colors, if more are needed, those 4 defaults are re-used repeatedly (1..2..3..4..1..) unless configured manually
  7. suggestion for better colors ?
  8. new toolbar button acts as A) hover target to enable enhancements with hover-only preference B) way to show configuration menu - I tried to keep visual changes to UI widgets to mininum
  9. all aspects of enhancements are fully configurable in the configuration menu, I avoided adding new or editing existing Eclipse (JDT) preferences pages completely
  10. Javadoc view and Javadoc hover viewer have their own preferences for state of enhancements, but share colors preferences
  11. Javadoc hover viewer has new toolbar button aligned to right
  12. Javadoc hover viewer has another new button added aligned to right - it's temprary for debugging purposes and will be removed
  13. Javadoc view has new toolbar button where separator was before - between navigation buttons (back, forward) and remaining buttons
  14. Colors Preferences sub-menu presents all colors for which colors are configured as menu items - either by default or manually
  15. Colors Preferences sub-menu presents colors currently used in content as enabled menu items and possible to change
  16. Colors Preferences sub-menu presents colors currently not used in content as diasabled menu items
  17. Preview overlay presenting enhancement states displayed when Styling menu is shown

Thank you in advance

@RedeemerSK RedeemerSK force-pushed the issue_1073 branch 4 times, most recently from b9c3c8f to 590a86f Compare January 21, 2024 15:20
@RedeemerSK RedeemerSK changed the title WIP: Add styling enhancements to signatures (JavaElementLinks) in Javadoc. Fixes #1073 Add styling enhancements to signatures (JavaElementLinks) in Javadoc. Fixes #1073 Jan 21, 2024
@RedeemerSK
Copy link
Contributor Author

I don't have access to MacOS so I can't test it on that platform. So if there's anyone willing to do that, it would be greatly appreciated.

For Linux, I am testing on native Ubuntu 22.04.3 LTS. After fully implementing required behavior of added GUI widgets on Windows, it all seems to work exactly the same also on Linux ... to my surprise, since I expected SWT may behave slightly different when it comes to when and which events are dispatched.

@RedeemerSK RedeemerSK force-pushed the issue_1073 branch 3 times, most recently from bfc5b8f to 3970cc2 Compare January 27, 2024 15:53
@RedeemerSK RedeemerSK force-pushed the issue_1073 branch 2 times, most recently from cd7440f to c6c89cc Compare January 30, 2024 20:27
@RedeemerSK
Copy link
Contributor Author

@akurtakov do you by chance have any feedback / opinion about the prototype ?
thanks in advance

@stephan-herrmann
Copy link
Contributor

I like the idea. Have you also considered type annotations in signatures? This would be particularly interesting in the context of null annotations where implicit annotations (from @NonNullByDefault or from external annotations) are visualized in those hovers, too.

@jukzi
Copy link
Contributor

jukzi commented Feb 14, 2024

i gave this PR a try: The superbold text looks blury and hard to read :-(, otherwise it looks OK and all tests pass.
image
Please also state some examples in JDK or eclipse workspace where the new formatting is noteworthy helpfull.
This feature should wait for an M1 release and have a option to disable for the case of failure.

@RedeemerSK
Copy link
Contributor Author

i gave this PR a try: The superbold text looks blury and hard to read :-(, otherwise it looks OK and all tests pass. image

I tried to make the method name stand out the most, so I gave it font-weight: 800 which I thought was the most non-disruptive way (to keep consistency in text font, sizes, underlining only on link hover, etc.) ... with font-weight: bold it would look like this:
image
other example
image
Would that be OK or does it still make sense to try to make method name to stand out the most ? In that case, does anyone have any other idea how to achieve it ?

Btw if anyone wants to play with the styling, I'm attaching 2 javadoc view browser content examples (screenshots above). Open in browser and use it's DevTools (F12) to tweak styling on-the-fly. Tip: disable display: none applied to checkboxes that are used to toggle enhancements to be able to toggle them.
eclipse-javadoc-examples.zip

Please also state some examples in JDK or eclipse workspace where the new formatting is noteworthy helpfull.

Will address soon

@RedeemerSK
Copy link
Contributor Author

RedeemerSK commented Feb 16, 2024

I like the idea. Have you also considered type annotations in signatures? This would be particularly interesting in the context of null annotations where implicit annotations (from @NonNullByDefault or from external annotations) are visualized in those hovers, too.

Nope. Also I'm not sure what exactly you have in mind. @stephan-herrmann Could you please describe your idea little bit more ?
Thanks

@RedeemerSK
Copy link
Contributor Author

Rebased onto master + changed superbold text to standard bold.

... This feature should wait for an M1 release and have a option to disable for the case of failure.

Toggle menu item added (for now added as 2nd commit, most probably will squash later). When disabled, no changes are done to generated HTML content (tags), although CSS prepended to generated HTML does contain enhancements segement that is however not used.

enhancements_toggling

Or did you @jukzi mean feature-flag instead ?

@jjohnstn
Copy link
Contributor

Hi @RedeemerSK The new design is much simpler, thanks. I agree that you had some cool UI/UX hacks and perhaps they can be used for something in the future. With the latest code, I have a few comments:

  1. The pull-down arrow says "Enable or Disable styling enhancements". It should be changed to be Styling enhancements or something like it
  2. The pull-down is disabled when the toggle is disabled. It probably wouldn't hurt to enable it all the time so preferences can be changed.
  3. I changed one color and then changed it back using restore defaults. I found the following in my log: !MESSAGE Following custom color preferences were removed:
    javadocElementsStyling.typesParamsReferenceColor_1 = RGB {252, 233, 79} which may or may not be expected
  4. Afterwards, the Javadoc view got locked and I can't get it to change by selecting new methods. It is locked on one method with styling selected. I didn't do much other than try the various settings (disabling/enabling and turning off/on colorization). I am on Linux and was looking Collection.toArray(T[] a)

The test errors in the latest build after rebasing are of no consequence as some Java 22 constants have been added but the I-build hasn't caught up yet.

Hi @jjohnstn, thanks for the review

  1. The pull-down arrow says "Enable or Disable styling enhancements". It should be changed to be Styling enhancements or something like it

As far as I can tell SWT does not support unique tooltip text on arrow that is different from tooltip for the button, So either it stays like it is now or tooltip for the whole button will get changed or removed.

Looking at other arrows, you are correct.

  1. The pull-down is disabled when the toggle is disabled. It probably wouldn't hurt to enable it all the time so preferences can be changed.

Done

Thanks.

  1. I changed one color and then changed it back using restore defaults. I found the following in my log: !MESSAGE Following custom color preferences were removed:
    javadocElementsStyling.typesParamsReferenceColor_1 = RGB {252, 233, 79} which may or may not be expected

I added this intentionally thinking it can be useful for the user to see what colors were reset to defaults ... if they would want to go back to them (e.g. they triggered reset by accident).

Removed now.

Great.

  1. Afterwards, the Javadoc view got locked and I can't get it to change by selecting new methods. It is locked on one method with styling selected. I didn't do much other than try the various settings (disabling/enabling and turning off/on colorization). I am on Linux and was looking Collection.toArray(T[] a)

I was not able to reproduce this (with new version having changes for above points). If you happen to find exact steps that trigger that for you, let me know then.

However, assuming it's really my changes causing described issue, then I guess it would most probably be addition & usage of org.eclipse.jdt.internal.ui.infoviews.JavadocView.fIgnoringNewInputOverride that could cause new inputs of JavadocView to be ignored ... but then it should cause issue only for one single following attempt to set new input to view.

Looking at the code I can't imagine a way it could 'lock' JavadocView. Still I made one fix there (made the field volatile since it's accessed not only from SWT thread) but I don't expect this would fix the issue, if the issue really is related to fIgnoringNewInputOverride.

I'll keep an eye out for it. This may be a problem we solve after the merge if it isn't a one-off in my set-up.

@RedeemerSK
Copy link
Contributor Author

@jjohnstn Shall I squash the commits as a preparation for merge then ? (not sure what the git hsitory preference / merge workflow here is)

@jjohnstn
Copy link
Contributor

@RedeemerSK No need to squash. I do that when I merge. The test failure is a known flaky test.

@jjohnstn jjohnstn merged commit 0b3382e into eclipse-jdt:master Mar 26, 2024
6 of 9 checks passed
@jjohnstn
Copy link
Contributor

@RedeemerSK Do you want to write a N&N entry and submit a PR to www.eclipse.org-eclipse or would you like me to do it? Try and make it brief with an example.

@RedeemerSK
Copy link
Contributor Author

@RedeemerSK Do you want to write a N&N entry and submit a PR to www.eclipse.org-eclipse or would you like me to do it? Try and make it brief with an example.

Will try soon

@RedeemerSK
Copy link
Contributor Author

@jjohnstn while preparing N&N entry I found 2 (lets call them) corner cases when types parameters coloring was not working as expected.

  • Missing coloring (for classes/interfaces from other compilation unit) missing_types_coloring
  • Unwanted coloring of type label(s) outside of generic signature (diamond brackets) unwanted_types_coloring

After fix

  • missing_types_coloring_fix
  • unwanted_types_coloring_fix

I prepared a fix ... is it fine to not make a new issue for it ? I would probably just create new branch (e.g. "issue_1073_fix") with fix commit and create PR like that if that's fine.

Thanks for reply

@iloveeclipse
Copy link
Member

just create new branch (e.g. "issue_1073_fix") with fix commit and create PR like that if that's fine.

Yes, please. In the commit message please refer to the original issue and/or this PR.

RedeemerSK added a commit to RedeemerSK/eclipse.jdt.ui that referenced this pull request Mar 29, 2024
…dt#1074)

Missing coloring for class / interface from other compilation unit.
Applied coloring to labels outside of diamond brackets.
@RedeemerSK
Copy link
Contributor Author

RedeemerSK commented Mar 29, 2024

Yes, please. In the commit message please refer to the original issue and/or this PR.

Thanks @iloveeclipse for quick reply.

PR created #1296

RedeemerSK added a commit to RedeemerSK/www.eclipse.org-eclipse that referenced this pull request Apr 2, 2024
RedeemerSK added a commit to RedeemerSK/www.eclipse.org-eclipse that referenced this pull request Apr 2, 2024
@RedeemerSK
Copy link
Contributor Author

@RedeemerSK Do you want to write a N&N entry and submit a PR to www.eclipse.org-eclipse or would you like me to do it? Try and make it brief with an example.

Will try soon

@jjohnstn here it is: eclipse-platform/www.eclipse.org-eclipse#145
And while you're at it, please take a look at #1296 as well. Thank you

jjohnstn pushed a commit that referenced this pull request Apr 3, 2024
Missing coloring for class / interface from other compilation unit.
Applied coloring to labels outside of diamond brackets.
jjohnstn pushed a commit to jjohnstn/www.eclipse.org-eclipse that referenced this pull request Apr 5, 2024
akurtakov pushed a commit to eclipse-platform/www.eclipse.org-eclipse that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants