Modernize layout of Fonts & Colors preference page for better alignment and usability #3968
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2003, 2026 IBM Corporation and others. | ||
| * | ||
| * This program and the accompanying materials | ||
| * are made available under the terms of the Eclipse Public License 2.0 | ||
|
|
@@ -39,7 +39,9 @@ | |
| import org.eclipse.jface.preference.PreferenceConverter; | ||
| import org.eclipse.jface.preference.PreferencePage; | ||
| import org.eclipse.jface.resource.DataFormatException; | ||
| import org.eclipse.jface.resource.ImageDescriptor; | ||
| import org.eclipse.jface.resource.JFaceResources; | ||
| import org.eclipse.jface.resource.ResourceLocator; | ||
| import org.eclipse.jface.resource.StringConverter; | ||
| import org.eclipse.jface.util.IPropertyChangeListener; | ||
| import org.eclipse.jface.util.PropertyChangeEvent; | ||
|
|
@@ -78,6 +80,8 @@ | |
| import org.eclipse.swt.widgets.FontDialog; | ||
| import org.eclipse.swt.widgets.Label; | ||
| import org.eclipse.swt.widgets.Text; | ||
| import org.eclipse.swt.widgets.ToolBar; | ||
| import org.eclipse.swt.widgets.ToolItem; | ||
| import org.eclipse.ui.IWorkbench; | ||
| import org.eclipse.ui.IWorkbenchPreferencePage; | ||
| import org.eclipse.ui.PlatformUI; | ||
|
|
@@ -614,11 +618,17 @@ public Color getBackground(Object element) { | |
| private Button goToDefaultButton; | ||
|
|
||
| /** | ||
| * The toolitem to expand the tree. | ||
| */ | ||
| private ToolItem expandAllItem; | ||
|
|
||
| /** | ||
| * The toolitem to collapse the tree. | ||
| */ | ||
| private ToolItem collapseAllItem; | ||
|
|
||
| private Image expandImage; | ||
| private Image collapseImage; | ||
|
|
||
| /** | ||
| * Map of definition FontDefinition->FontData[] capturing the changes explicitly | ||
|
|
@@ -829,43 +839,70 @@ protected Control createContents(Composite parent) { | |
| if (appliedDialogFont != null) { | ||
| appliedDialogFont.dispose(); | ||
| } | ||
| // Dispose custom images | ||
| if (expandImage != null) { | ||
| expandImage.dispose(); | ||
| } | ||
| if (collapseImage != null) { | ||
| collapseImage.dispose(); | ||
| } | ||
| }); | ||
|
|
||
| final SashForm advancedComposite = new SashForm(parent, SWT.VERTICAL); | ||
| advancedComposite.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
|
|
||
| Composite mainColumn = new Composite(advancedComposite, SWT.NONE); | ||
| GridLayout layout = new GridLayout(2, false); | ||
| layout.marginWidth = 0; | ||
| layout.marginHeight = 0; | ||
| mainColumn.setFont(parent.getFont()); | ||
|
Contributor
There was a problem hiding this comment. Why this change? This might have an impact that I don't notice.
Contributor
Author
There was a problem hiding this comment. Good point — thanks for calling that out. This line was removed during layout refactoring, but it’s not strictly required since SWT controls inherit the parent font by default. That said, I agree that keeping it makes the intent explicit and avoids any platform-specific inconsistencies. Shall i restore it to be safe and keep behavior identical to the original implementation.
Contributor
There was a problem hiding this comment. Yes, better safe than sorry I think. |
||
| mainColumn.setLayout(layout); | ||
|
|
||
| Label label = new Label(mainColumn, SWT.LEFT); | ||
| label.setText(RESOURCE_BUNDLE.getString("colorsAndFonts")); //$NON-NLS-1$ | ||
| myApplyDialogFont(label); | ||
| label.setLayoutData(new GridData(SWT.BEGINNING, SWT.CENTER, false, false, 2, 1)); | ||
|
|
||
| // LEFT COLUMN (tree + filter) | ||
| Composite leftColumn = new Composite(mainColumn, SWT.NONE); | ||
| leftColumn.setLayout(new GridLayout(1, false)); | ||
| leftColumn.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true, 1, 1)); | ||
|
|
||
| createTree(leftColumn); | ||
|
|
||
| // RIGHT COLUMN (toolbar + buttons) | ||
| Composite controlColumn = new Composite(mainColumn, SWT.NONE); | ||
| controlColumn.setLayout(new GridLayout()); | ||
| controlColumn.setLayoutData(new GridData(SWT.FILL, SWT.FILL, false, true)); | ||
|
|
||
| // Toolbar at TOP (aligned with filter text) | ||
| ToolBar treeToolbar = new ToolBar(controlColumn, SWT.FLAT); | ||
| treeToolbar.setLayoutData(new GridData(SWT.END, SWT.BEGINNING, false, false)); | ||
|
|
||
| // LOAD IMAGES ONCE | ||
| ImageDescriptor expandDesc = ResourceLocator | ||
| .imageDescriptorFromBundle("org.eclipse.ui", "icons/full/elcl16/expandall.svg") //$NON-NLS-1$ //$NON-NLS-2$ | ||
| .orElse(null); | ||
|
Contributor
There was a problem hiding this comment. If this were to fail, the button will be useless (horribly ugly) won't it?
Contributor
Author
There was a problem hiding this comment. Thought of this way. But in the similar lines, icon for Expand All is not readily available in org.eclipse.ui.ISharedImages, how should this be handled in your opinion? can i use org.eclipse.ui.ISharedImages.IMG_ELCL_COLLAPSEALL or IMG_ELCL_REMOVE?
Contributor
There was a problem hiding this comment. I think in the past the tendency was to ensure that the icon was available locally in the bundle rather than to use fragile programmatic access. Let's ask @HeikoKlare @BeckerWdf how one can best do something here that's not fragile.
Member
There was a problem hiding this comment. you could simply copy the PNG files into the bundle.
Contributor
Author
There was a problem hiding this comment. Sorry, let me correct my update and put it across again in some time.
Contributor
Author
There was a problem hiding this comment. Thanks for the insights @BeckerWdf and @merks My findings are : icons/images are already existing in below paths.
So for this pr the respective java file is at "..\git\eclipse.platform.ui\bundles\org.eclipse.ui.workbench\eclipseui\org\eclipse\ui\internal\themes\ColorsAndFontsPreferencePage.java" Option 1 - Add respective expandall, collapseall icons svg/png at freshly created path for this plugin "..\git\eclipse.platform.ui\bundles\org.eclipse.ui.workbench\icons\full\elcl16" and use like below ImageDescriptor expandDesc = AbstractUIPlugin.imageDescriptorFromPlugin( expandImage = expandDesc.createImage(); Option 2 - For API way, if i add "icons/full/elcl16/expandall.svg" corresponding entries(inline with collapseall.svg which is already accessible) in ISharedImages.java and WorkbenchImages.java. Are these the only 2 steps i need to perform or do i have to add anything else which i might have missed. Another observation as well. So anything in similar lines needs to be done? But I do understand these are used by the command framework and are not directly applicable for ToolItem usage but better to do, so that it is available as a generic solution if anywhere else - it can also be used(since collapseall is already available but not expandall)
Member
There was a problem hiding this comment.
I think yes.
Contributor
There was a problem hiding this comment. The expand-all and collapse-all I think are heavily reused so it would seem a good thing for them to be shared across those usages, e.g., 
Contributor
Author
There was a problem hiding this comment. To keep this as a generic one for everyone to use -> #3981 |
||
| if (expandDesc != null) { | ||
| expandImage = expandDesc.createImage(); | ||
| } | ||
| expandAllItem = new ToolItem(treeToolbar, SWT.PUSH); | ||
| if (expandImage != null) { | ||
| expandAllItem.setImage(expandImage); | ||
| } | ||
| expandAllItem.setToolTipText("Expand All"); //$NON-NLS-1$ | ||
|
|
||
| ImageDescriptor collapseDesc = ResourceLocator | ||
| .imageDescriptorFromBundle("org.eclipse.ui", "icons/full/elcl16/collapseall.svg") //$NON-NLS-1$ //$NON-NLS-2$ | ||
| .orElse(null); | ||
| if (collapseDesc != null) { | ||
| collapseImage = collapseDesc.createImage(); | ||
| } | ||
| collapseAllItem = new ToolItem(treeToolbar, SWT.PUSH); | ||
| if (collapseImage != null) { | ||
| collapseAllItem.setImage(collapseImage); | ||
| } | ||
| collapseAllItem.setToolTipText("Collapse All"); //$NON-NLS-1$ | ||
|
|
||
| fontChangeButton = createButton(controlColumn, RESOURCE_BUNDLE.getString("openChange")); //$NON-NLS-1$ | ||
| fontSystemButton = createButton(controlColumn, WorkbenchMessages.FontsPreference_useSystemFont); | ||
|
|
@@ -874,41 +911,28 @@ protected Control createContents(Composite parent) { | |
| editDefaultButton = createButton(controlColumn, RESOURCE_BUNDLE.getString("editDefault")); //$NON-NLS-1$ | ||
| goToDefaultButton = createButton(controlColumn, RESOURCE_BUNDLE.getString("goToDefault")); //$NON-NLS-1$ | ||
| createSeparator(controlColumn); | ||
| // end of buttons | ||
|
|
||
| createDescriptionControl(mainColumn); | ||
|
|
||
| Composite previewColumn = new Composite(advancedComposite, SWT.NONE); | ||
| previewColumn.setLayout(new GridLayout()); | ||
| previewColumn.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false)); | ||
|
|
||
| // create preview control | ||
| Composite composite = new Composite(previewColumn, SWT.NONE); | ||
| composite.setLayout(new GridLayout()); | ||
| composite.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
|
|
||
| Label label2 = new Label(composite, SWT.LEFT); | ||
| label2.setText(RESOURCE_BUNDLE.getString("preview")); //$NON-NLS-1$ | ||
| myApplyDialogFont(label2); | ||
|
|
||
| previewComposite = new Composite(composite, SWT.NONE); | ||
| previewComposite.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
| stackLayout = new StackLayout(); | ||
| previewComposite.setLayout(stackLayout); | ||
| // end of preview control | ||
|
|
||
| defaultFontPreview = createFontPreviewControl(); | ||
| defaultColorPreview = createColorPreviewControl(); | ||
|
|
@@ -972,9 +996,10 @@ private String getText(Object element) { | |
| }; | ||
| filter.setIncludeLeadingWildcard(true); | ||
|
|
||
| tree = new FilteredTree(parent, SWT.SINGLE | SWT.H_SCROLL | SWT.V_SCROLL | SWT.BORDER, filter, true, true); | ||
| tree.setQuickSelectionMode(true); | ||
|
|
||
| GridData data = new GridData(SWT.FILL, SWT.FILL, true, true); | ||
| data.widthHint = Math.max(285, convertWidthInCharsToPixels(30)); | ||
| data.heightHint = Math.max(175, convertHeightInCharsToPixels(10)); | ||
| tree.setLayoutData(data); | ||
|
|
@@ -989,10 +1014,7 @@ private String getText(Object element) { | |
| tree.getViewer().setComparator(new ViewerComparator() { | ||
| @Override | ||
| public int category(Object element) { | ||
| return (element instanceof ThemeElementCategory) ? 0 : 1; | ||
| } | ||
| }); | ||
| tree.getViewer().setInput(WorkbenchPlugin.getDefault().getThemeRegistry()); | ||
|
|
@@ -1261,9 +1283,8 @@ private void hookListeners() { | |
| } | ||
| updateControls(); | ||
| })); | ||
| expandAllItem.addListener(SWT.Selection, e -> tree.getViewer().expandAll()); | ||
| collapseAllItem.addListener(SWT.Selection, e -> tree.getViewer().collapseAll()); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're inlining, which is fine, but as a reviewer it's not easy to see that this is identical. I trust that it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling that out. Yes, this is functionally identical - new GridLayout(2, false) is equivalent to setting numColumns = 2 with the default makeColumnsEqualWidth = false.
I agree that the previous form may be easier to compare in a diff. I can revert to the expanded version if you prefer for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's more an observation. I like it simpler the way you wrote it! I trust that you are properly testing all the layout and appearance that the simpler expression produces good behavior.