Skip to content

Modernize layout of Fonts & Colors preference page for better alignment and usability #3968

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.

Sign up for GitHub

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

Open
deepika-u wants to merge 1 commit into
base: master
Choose a base branch
from
+54 −58
Open

Modernize layout of Fonts & Colors preference page for better alignment and usability #3968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 50 additions & 56 deletions ...e.ui.workbench/eclipseui/org/eclipse/ui/internal/themes/ColorsAndFontsPreferencePage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2003, 2016 IBM Corporation and others.
* 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
Expand Down Expand Up @@ -78,6 +78,9 @@
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.ISharedImages;
import org.eclipse.ui.IWorkbench;
import org.eclipse.ui.IWorkbenchPreferencePage;
import org.eclipse.ui.PlatformUI;
Expand Down Expand Up @@ -614,11 +617,14 @@ public Color getBackground(Object element) {
private Button goToDefaultButton;

/**
* The button to expand the tree.
*
* @since 4.5
* The toolitem to expand the tree.
*/
private Button expandAllButton;
private ToolItem expandAllItem;

/**
* The toolitem to collapse the tree.
*/
private ToolItem collapseAllItem;

/**
* Map of definition FontDefinition->FontData[] capturing the changes explicitly
Expand Down Expand Up @@ -832,40 +838,44 @@ protected Control createContents(Composite parent) {
});

final SashForm advancedComposite = new SashForm(parent, SWT.VERTICAL);
GridData sashData = new GridData(SWT.FILL, SWT.FILL, true, true);
advancedComposite.setLayoutData(sashData);
advancedComposite.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));

Composite mainColumn = new Composite(advancedComposite, SWT.NONE);
GridLayout layout = new GridLayout();
layout.numColumns = 2;
GridLayout layout = new GridLayout(2, false);
Copy link
Copy Markdown
Contributor

@merks merks May 6, 2026

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.

Copy link
Copy Markdown
Contributor Author

@deepika-u deepika-u May 6, 2026

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.

Copy link
Copy Markdown
Contributor

@merks merks May 6, 2026

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.

layout.marginWidth = 0;
layout.marginHeight = 0;
mainColumn.setFont(parent.getFont());
Copy link
Copy Markdown
Contributor

@merks merks May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? This might have an impact that I don't notice.

Copy link
Copy Markdown
Contributor Author

@deepika-u deepika-u May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@merks merks May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, better safe than sorry I think.

Copy link
Copy Markdown
Contributor Author

@deepika-u deepika-u May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

mainColumn.setLayout(layout);

GridData data = new GridData(GridData.BEGINNING);
data.horizontalSpan = 2;
Label label = new Label(mainColumn, SWT.LEFT);
label.setText(RESOURCE_BUNDLE.getString("colorsAndFonts")); //$NON-NLS-1$
myApplyDialogFont(label);
label.setLayoutData(data);
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(mainColumn);
createTree(leftColumn);

// --- buttons
// RIGHT COLUMN (toolbar + buttons)
Composite controlColumn = new Composite(mainColumn, SWT.NONE);
data = new GridData(GridData.FILL_VERTICAL);
controlColumn.setLayoutData(data);
layout = new GridLayout();
layout.marginHeight = 0;
layout.marginWidth = 0;
controlColumn.setLayout(layout);
controlColumn.setLayout(new GridLayout());
controlColumn.setLayoutData(new GridData(SWT.FILL, SWT.FILL, false, true));

// we need placeholder to offset the filter control of the table
Label placeholder = new Label(controlColumn, SWT.NONE);
GridData placeholderData = new GridData(SWT.TOP);
placeholderData.heightHint = convertVerticalDLUsToPixels(12);
placeholder.setLayoutData(placeholderData);
// Toolbar at TOP (aligned with filter text)
ToolBar treeToolbar = new ToolBar(controlColumn, SWT.FLAT);
treeToolbar.setLayoutData(new GridData(SWT.END, SWT.BEGINNING, false, false));

expandAllItem = new ToolItem(treeToolbar, SWT.PUSH);
expandAllItem.setImage(PlatformUI.getWorkbench().getSharedImages().getImage(ISharedImages.IMG_ELCL_EXPANDALL));
expandAllItem.setToolTipText(RESOURCE_BUNDLE.getString("expandAll")); //$NON-NLS-1$

collapseAllItem = new ToolItem(treeToolbar, SWT.PUSH);
collapseAllItem
.setImage(PlatformUI.getWorkbench().getSharedImages().getImage(ISharedImages.IMG_ELCL_COLLAPSEALL));
collapseAllItem.setToolTipText("collapseAll"); //$NON-NLS-1$

fontChangeButton = createButton(controlColumn, RESOURCE_BUNDLE.getString("openChange")); //$NON-NLS-1$
fontSystemButton = createButton(controlColumn, WorkbenchMessages.FontsPreference_useSystemFont);
Expand All @@ -874,41 +884,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);
expandAllButton = createButton(controlColumn, RESOURCE_BUNDLE.getString("expandAll")); //$NON-NLS-1$
expandAllButton.setEnabled(true);
// --- end of buttons
// end of buttons

createDescriptionControl(mainColumn);

Composite previewColumn = new Composite(advancedComposite, SWT.NONE);
GridLayout previewLayout = new GridLayout();
previewLayout.marginTop = 7;
previewLayout.marginWidth = 0;
previewLayout.marginHeight = 0;
previewColumn.setFont(parent.getFont());
previewColumn.setLayout(previewLayout);

// --- create preview control
Composite composite = new Composite(previewColumn, SWT.NONE);
previewColumn.setLayout(new GridLayout());
previewColumn.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));

GridData data2 = new GridData(GridData.FILL_BOTH);
composite.setLayoutData(data2);
GridLayout layout2 = new GridLayout(1, true);
layout2.marginHeight = 0;
layout2.marginWidth = 0;
composite.setLayout(layout2);
// 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(GridData.FILL_BOTH));
previewComposite.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));
stackLayout = new StackLayout();
stackLayout.marginHeight = 0;
stackLayout.marginWidth = 0;
previewComposite.setLayout(stackLayout);
// -- end of preview control
// end of preview control

defaultFontPreview = createFontPreviewControl();
defaultColorPreview = createColorPreviewControl();
Expand Down Expand Up @@ -972,9 +969,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);
tree = new FilteredTree(parent, SWT.SINGLE | SWT.H_SCROLL | SWT.V_SCROLL | SWT.BORDER, filter, true, true);
tree.setQuickSelectionMode(true);
GridData data = new GridData(GridData.FILL_BOTH | GridData.VERTICAL_ALIGN_FILL);

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);
Expand All @@ -989,10 +987,7 @@ private String getText(Object element) {
tree.getViewer().setComparator(new ViewerComparator() {
@Override
public int category(Object element) {
if (element instanceof ThemeElementCategory) {
return 0;
}
return 1;
return (element instanceof ThemeElementCategory) ? 0 : 1;
}
});
tree.getViewer().setInput(WorkbenchPlugin.getDefault().getThemeRegistry());
Expand Down Expand Up @@ -1261,9 +1256,8 @@ private void hookListeners() {
}
updateControls();
}));

expandAllButton.addSelectionListener(widgetSelectedAdapter(event -> tree.getViewer().expandAll()));

expandAllItem.addListener(SWT.Selection, e -> viewer.expandAll());
collapseAllItem.addListener(SWT.Selection, e -> viewer.collapseAll());
}

@Override
Expand Down
6 changes: 4 additions & 2 deletions ...orkbench/eclipseui/org/eclipse/ui/internal/themes/ColorsAndFontsPreferencePage.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
###############################################################################
# Copyright (c) 2003, 2014, 2015 IBM Corporation and others.
# Copyright (c) 2003, 2014, 2015, 2026 IBM Corporation and others.
#
# This program and the accompanying materials
# are made available under the terms of the Eclipse Public License 2.0
Expand All @@ -17,7 +17,9 @@ openChange=&Edit...
reset=&Reset
editDefault=Ed&it Default...
goToDefault=&Go to Default
expandAll=E&xpand All
#expandAll=E&xpand All
expandAll=Expand All
collapseAll=Collapse All
value=&Value
colorsAndFonts=Colors and &Fonts (font, size, type, ? = any character, * = any string) :
description=Descriptio&n:
Expand Down
Loading