Add font zoom functionality to console view#2579
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Ctrl +/- (including numpad +/-) zoom support to the Eclipse Console view to align behavior with text editors (Issue #2578).
Changes:
- Installs a key listener on the console’s
StyledTextto detect Ctrl+Plus / Ctrl+Minus. - Implements font resizing logic with min/max bounds and a step size, creating a derived SWT
Font. - Disposes the created font on view disposal.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
74720eb to
21d419f
Compare
|
Have you thought about persisting the current zoom over the current session? That will be likely next immediate customer request after using this feature for the first time & IDE restart. Assuming the code moved to the page (from the console), we would always know "where we are" and should be able to persist the zoom in the preference store by using the |
21d419f to
c051ad1
Compare
|
@trancexpress Please check this PR. |
|
With: The Console view can be zoomed when I'm in the Java editor and I press: The change here makes me press The Java editor zoom in / zoom out resets whatever the zoom level was in the console, which is probably OK. |
There was a problem hiding this comment.
Pull request overview
Adds Ctrl +/- key handling to the Console view’s text widget to support zooming the console font size, addressing the usability gap described in #2578.
Changes:
- Add a key listener on the console
StyledTextto zoom font size on Ctrl + / Ctrl - (including numpad +/-). - Implement per-console-type font persistence via the console plug-in preference store and restore it on page creation.
- Minor whitespace adjustment in
ConsoleView.dispose().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleView.java | Minor formatting-only change in dispose(). |
| debug/org.eclipse.ui.console/src/org/eclipse/ui/console/TextConsolePage.java | Adds zoom key listener, font resizing logic, and preference-based font load/save for console pages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cb6dbf0 to
0c77159
Compare
9dd0a16 to
e1d88b4
Compare
|
@iloveeclipse Please check this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e1d88b4 to
8c76e12
Compare
iloveeclipse
left a comment
There was a problem hiding this comment.
Note: before pushing fixes, please rebase on master first.
| } | ||
| FontData[] newFontData = fontData.clone(); | ||
| for (FontData fd : newFontData) { | ||
| if (fd != null) |
There was a problem hiding this comment.
Please always use blocks for if/for/while statements, independently how many lines are inside.
| id="org.eclipse.ui.commands.toggleState"> | ||
| </state> | ||
| </command> | ||
| <command |
There was a problem hiding this comment.
These commands can be executed via "Ctrl+3", and guess what ... They change current editor font, if editor is currently active, or "Debug Shell" view font etc.
We need <enabledWhen> on the handler to restrict it to Console view only.
iloveeclipse
left a comment
There was a problem hiding this comment.
Note: before pushing fixes, please rebase on master first.
iloveeclipse
left a comment
There was a problem hiding this comment.
Note: before pushing fixes, please rebase on master first.
8c76e12 to
5cc0461
Compare
5cc0461 to
b114c03
Compare
|
@raghucssit : I don't see changes in the patch itself after last rebase you did? |
-Add key listener on text widget of console view which listens ctrl plus and control minus(including numpad +/-) -Zoom in/out of the console view text in steps based on the keys pressed. see eclipse-platform#2578
b114c03 to
2f6f049
Compare
|
I have fixed all the review comments and issues present at last review..
|
|
Regarding bug that activates editor content on selection of Stacktrace Console has the below fix. I will create a separate issue at jdt debug repo and contribute below patch. |
|
Stacktrace of the Exception if we don't use Display UI execution. Async is used to avoid any possible deadlocks. |
iloveeclipse
left a comment
There was a problem hiding this comment.
Quick review: have not tested yet, but I see some pieces that need to be fixed.
| return ConsolePluginImages.getImageDescriptor(key); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Please remove this, just init the ZoomHandler on ConsoleManager startup
| * zoom level, instead of resetting to the default size.</li> | ||
| * </ul> | ||
| * | ||
| * @since 3.17 |
There was a problem hiding this comment.
@since is only needed on API. This class is not API.
| if (!(console instanceof TextConsole textConsole)) { | ||
| return; | ||
| } | ||
| int baseHeight; |
There was a problem hiding this comment.
Please check for display here and run any operation whic accesses Font & Co from UI thread only (call asyncExec() as you do it below).
| * @param height the font height to apply, in points | ||
| */ | ||
| private static void applyHeight(TextConsole textConsole, int height) { | ||
| Display display = Display.getDefault(); |
There was a problem hiding this comment.
Instead of this
Display display = Display.getDefault();
if (display.getThread() != Thread.currentThread())
please use if (Display.getCurrent() == null), here and in other places
| return; | ||
| } | ||
| Display display = Display.getDefault(); | ||
| if (display.getThread() != Thread.currentThread()) { |
There was a problem hiding this comment.
Please start the method with this check.
| * Key used to remember, on the console itself, the custom font created for | ||
| * zooming, so it can be reused/replaced and eventually disposed. | ||
| */ | ||
| static final String ZOOM_FONT_ATTRIBUTE = ConsoleZoomInHandler.class.getName() + ".zoomFont"; //$NON-NLS-1$ |
There was a problem hiding this comment.
I would not store the "global" zoom font size but the zoomStep value as delta, and/or store the delta value per console type.
Every console can define its own font & size, and font sizes defined by different consoles may differ. Consoles may have individual font preferences settings which can be configured.
Current proposal increases font size for all consoles and sets it to the font size defined by the current console +- zoom step value. Also it enforces last zoomed value even if user changes console font size in preferences.
This all resuls in an unexpected behavior:
- If the current console has "unusual" font size and/or other consoles font size differ from the current one and if the user only wanted to change font size in the current console only. Here I would expect that if we apply zoom, we apply zoom delta (+-1 step) to the currently set font size in each console.
- If the user changes Console font size for some console to "20" in preferences but we already had "zoomed" to font size "10", it will stay at 10. I would expect that we rezet zoom delta once user changes the font size via preferences, so that whatever is chosen by user in preferences "wins".
| return; | ||
| } | ||
| sZoomHeight = Integer.valueOf(newHeight); | ||
| persistZoomHeight(newHeight); |
There was a problem hiding this comment.
Actually the code above can be removed entirely and we can simply iterate over all consoles below, applying the zoom step to each console font size (not the font size of the current console)
-Add key listener on text widget of console view which listens ctrl plus and control minus(including numpad +/-)
-Zoom in/out of the console view text in steps based on the keys pressed.
see #2578