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.

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

Fix size of tab items when large text padding is used #989

Merged
merged 2 commits into from
Jan 30, 2024
Merged

Fix size of tab items when large text padding is used #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a913fdc
CTabFolderRender: Factor out shared functionality related to close icon
HeikoKlare Jan 19, 2024
3779c51
Fix size of tab items when large text padding is used
HeikoKlare Jan 19, 2024
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
24 changes: 11 additions & 13 deletions .../org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2972,19 +2972,17 @@ boolean setItemSize(GC gc) {
for (int i = 0; i < items.length; i++) {
CTabItem tab = items[i];
int width = widths[i];
if (tab.height != tabHeight || tab.width != width) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a "dangerous" optimization as it assumes that tab items only have changed (and need some recalculation) if their size changed. That assumptions breaks with this PR.

changed = true;
tab.shortenedText = null;
tab.shortenedTextWidth = 0;
tab.height = tabHeight;
tab.width = width;
tab.closeRect.width = tab.closeRect.height = 0;
if (showClose || tab.showClose) {
if (i == selectedIndex || showUnselectedClose) {
Point closeSize = renderer.computeSize(CTabFolderRenderer.PART_CLOSE_BUTTON, SWT.NONE, gc, SWT.DEFAULT, SWT.DEFAULT);
tab.closeRect.width = closeSize.x;
tab.closeRect.height = closeSize.y;
}
changed = true;
tab.shortenedText = null;
tab.shortenedTextWidth = 0;
tab.height = tabHeight;
tab.width = width;
tab.closeRect.width = tab.closeRect.height = 0;
if (showClose || tab.showClose) {
if (i == selectedIndex || showUnselectedClose) {
Point closeSize = renderer.computeSize(CTabFolderRenderer.PART_CLOSE_BUTTON, SWT.NONE, gc, SWT.DEFAULT, SWT.DEFAULT);
tab.closeRect.width = closeSize.x;
tab.closeRect.height = closeSize.y;
}
}
}
Expand Down
63 changes: 34 additions & 29 deletions ...ipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,17 +358,11 @@ protected Point computeSize (int part, int state, GC gc, int wHint, int hHint) {
}
}

width += getTextPadding(item, state) * 2;

if (parent.showClose || item.showClose) {
if ((state & SWT.SELECTED) != 0 || parent.showUnselectedClose) {
if (!applyLargeTextPadding(parent)) {
if (width > 0) width += INTERNAL_SPACING;
} else {
if (width > 0) width -= INTERNAL_SPACING;
}
width += computeSize(PART_CLOSE_BUTTON, SWT.NONE, gc, SWT.DEFAULT, SWT.DEFAULT).x;
}
if (shouldApplyLargeTextPadding(parent)) {
HeikoKlare marked this conversation as resolved.
Show resolved Hide resolved
width += getLargeTextPadding(item) * 2;
} else if (shouldDrawCloseIcon(item)) {
if (width > 0) width += INTERNAL_SPACING;
width += computeSize(PART_CLOSE_BUTTON, SWT.NONE, gc, SWT.DEFAULT, SWT.DEFAULT).x;
}
}
break;
Expand All @@ -379,27 +373,27 @@ protected Point computeSize (int part, int state, GC gc, int wHint, int hHint) {
return new Point(width, height);
}

private boolean shouldDrawCloseIcon(CTabItem item) {
CTabFolder folder = item.getParent();
boolean showClose = folder.showClose || item.showClose;
boolean isSelectedOrShowCloseForUnselected = (item.state & SWT.SELECTED) != 0 || folder.showUnselectedClose;
return showClose && isSelectedOrShowCloseForUnselected;
}

/**
* Returns padding for the text of a tab when image is not available or is hidden.
*
* @param item CTabItem
* @param state current state
*
* Returns padding for the text of a tab item when showing images is disabled for the tab folder.
*/
private int getTextPadding(CTabItem item, int state) {
private int getLargeTextPadding(CTabItem item) {
CTabFolder parent = item.getParent();
String text = item.getText();

if (text != null && parent.getMinimumCharacters() != 0) {
if (applyLargeTextPadding(parent)) {
return TABS_WITHOUT_ICONS_PADDING;
}
return TABS_WITHOUT_ICONS_PADDING;
}

return 0;
}

private boolean applyLargeTextPadding(CTabFolder tabFolder) {
private boolean shouldApplyLargeTextPadding(CTabFolder tabFolder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The present condition results in a large text padding only when showing images is disabled for all tabs (selected and unselected) of the tab folder.
But this leads to no text padding/margin for unselected tabs when showing images is disabled for all 'unselected' tabs.
image

  • Separation between unselected tabs takes a hit,

Similarly, there is no margin or padding applied to the selected tab when only the selected tab image is set to be hidden.
image

Suggested change
private boolean shouldApplyLargeTextPadding(CTabFolder tabFolder) {
private boolean shouldApplyLargeTextPadding(CTabItem item) {
CTabFolder folder = item.getParent();
return (!folder.showSelectedImage && (item.state & SWT.SELECTED) != 0) ||
(!folder.showUnselectedImage && (item.state & SWT.SELECTED) == 0);
}

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's true, but that is the behavior even without the changes in this PR. And in particular, that is the behavior that has been existing for years and must not be changed.
The padding-based separation of tabs, as introduced with #785, is only active if images are completely disabled for tabs. This is currently identified by both showing images of selected and unselected tabs is set to false, as the former was always true before #785, so that this way of configuration cannot produce regressions. Still, we should think of more explicit approach to apply text-based padding to a CTabFolder (see #785 (comment)).

Note that #945 is also still open for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the context of avoiding regression it makes sense to not apply this suggestion. Will evaluate this on #945 👍

return !tabFolder.showSelectedImage && !tabFolder.showUnselectedImage;
}

Expand Down Expand Up @@ -1416,7 +1410,7 @@ void drawSelected(int itemIndex, GC gc, Rectangle bounds, int state ) {
// draw Image
Rectangle trim = computeTrim(itemIndex, SWT.NONE, 0, 0, 0, 0);
int xDraw = x - trim.x;
if (parent.single && (parent.showClose || item.showClose)) xDraw += item.closeRect.width;
if (parent.single && shouldDrawCloseIcon(item)) xDraw += item.closeRect.width;
Image image = item.getImage();
if (image != null && !image.isDisposed() && parent.showSelectedImage) {
Rectangle imageBounds = image.getBounds();
Expand All @@ -1433,7 +1427,7 @@ void drawSelected(int itemIndex, GC gc, Rectangle bounds, int state ) {
}

// draw Text
xDraw += getTextPadding(item, state);
xDraw += getLeftTextMargin(item);
int textWidth = rightEdge - xDraw - (trim.width + trim.x);
if (!parent.single && item.closeRect.width > 0) textWidth -= item.closeRect.width + INTERNAL_SPACING;
if (textWidth > 0) {
Expand Down Expand Up @@ -1469,8 +1463,19 @@ void drawSelected(int itemIndex, GC gc, Rectangle bounds, int state ) {
gc.setBackground(orginalBackground);
}
}
if (parent.showClose || item.showClose) drawClose(gc, item.closeRect, item.closeImageState);
if (shouldDrawCloseIcon(item)) drawClose(gc, item.closeRect, item.closeImageState);
}
}

private int getLeftTextMargin(CTabItem item) {
int margin = 0;
if (shouldApplyLargeTextPadding(parent)) {
HeikoKlare marked this conversation as resolved.
Show resolved Hide resolved
margin += getLargeTextPadding(item);
if (shouldDrawCloseIcon(item)) {
margin -= item.closeRect.width / 2;
}
}
return margin;
}

void drawTabArea(GC gc, Rectangle bounds, int state) {
Expand Down Expand Up @@ -1631,7 +1636,7 @@ void drawUnselected(int index, GC gc, Rectangle bounds, int state) {
Rectangle imageBounds = image.getBounds();
// only draw image if it won't overlap with close button
int maxImageWidth = x + width - xDraw - (trim.width + trim.x);
if (parent.showUnselectedClose && (parent.showClose || item.showClose)) {
if (shouldDrawCloseIcon(item)) {
maxImageWidth -= item.closeRect.width + INTERNAL_SPACING;
}
if (imageBounds.width < maxImageWidth) {
Expand All @@ -1647,9 +1652,9 @@ void drawUnselected(int index, GC gc, Rectangle bounds, int state) {
}
}
// draw Text
xDraw += getTextPadding(item, state);
xDraw += getLeftTextMargin(item);
int textWidth = x + width - xDraw - (trim.width + trim.x);
if (parent.showUnselectedClose && (parent.showClose || item.showClose)) {
if (shouldDrawCloseIcon(item)) {
textWidth -= item.closeRect.width + INTERNAL_SPACING;
}
if (textWidth > 0) {
Expand All @@ -1667,7 +1672,7 @@ void drawUnselected(int index, GC gc, Rectangle bounds, int state) {
gc.setFont(gcFont);
}
// draw close
if (parent.showUnselectedClose && (parent.showClose || item.showClose)) drawClose(gc, item.closeRect, item.closeImageState);
if (shouldDrawCloseIcon(item)) drawClose(gc, item.closeRect, item.closeImageState);
}
}

Expand Down
Loading