CTabFolder: Add 'showSelectedImage' property #785

shubhamWaghmare-sap · 2023-08-29T08:49:10Z

Added new property "showSelectedImage" to CTabFolder parallel to existing property "showUnselectedImage"
Image rendering in drawSelected method updated in CTabFolderRenderer with handling the new property "showSelectedImage"
Added standard padding for text of tabs when tab icon is either not available or is hidden (both for selected and unselected tabs)
Pre-requisite for issue and pull request : Add Options to hide tab icons and and show full text in view area eclipse.platform.ui#1071

github-actions · 2023-08-29T09:25:02Z

Test Results

    299 files ±0     299 suites ±0 5m 24s ⏱️ -18s
  4 097 tests +1   4 089 ✔️ +1   8 💤 ±0 0 ❌ ±0
12 203 runs +3 12 130 ✔️ +3 73 💤 ±0 0 ❌ ±0

Results for commit 473ffaa. ± Comparison against base commit 27feab2.

♻️ This comment has been updated with latest results.

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

HeikoKlare

Thanks for this extension.
I guess the naming like getSelectedImageVisible() instead of isSelectedImageVisible() is because of consistency with the existing API, isn't it? Apart from that, I only have some minor comments.

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolder.java

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

HeikoKlare · 2023-10-05T08:32:15Z

@shubhamWaghmare-sap Looks like you have addressed the review comments. Are you still working on this PR or it is already worth to re-review?

shubhamWaghmare-sap · 2023-10-05T10:50:22Z

@shubhamWaghmare-sap Looks like you have addressed the review comments. Are you still working on this PR or it is already worth to re-review?

@HeikoKlare Yes. We have incorporated the review comments in this PR. It is ready to be re-reviewed now. Thanks.
We do have some open questions like a better name for the method getTextPadding(...) added in CTabFolderRenderer and the need for text padding for unselected tabs as well, when image is hidden or not available.

Hoping to get answers via discussions on PR. 👍

A short overview of changes:

Text padding added in both selected and unselected tab when image is hidden or not available
In case there is no text for the tab, no padding is added.
Refactoring.

HeikoKlare

Thanks for the improvements to this PR and sorry for the late reply.

In general, I am fine with the changes. I only have two concerns with respect the padding:

I would propose to separate the padding for the case that icons are not shown from the existing INTERAL_SPACING (in a variable such as TABS_WITHOUT_ICONS_PADDING) as they apply to different behaviors and should thus not influence each other.
I would even be in favor of having a bigger spacing when no icons are shown. This seems to require 1. to work properly (see my detailed comments).

To me, a value like 14 for such a TABS_WITHOUT_ICONS_PADDING feels good :

But that may be a matter of personal preference, it might also be smaller but at least should be somehow independent from the existing spacing.

HeikoKlare · 2023-11-28T16:25:50Z

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

@@ -355,6 +356,7 @@ protected Point computeSize (int part, int state, GC gc, int wHint, int hHint) {
 							gc.setFont(gcFont);
 						}
 					}
+					width += getTextPadding(item, state);


I would suggest to apply the padding two times to the overall width, so that the text is centered within the tab (as the padding is applied one time to the x offset of the text).

Suggested change

width += getTextPadding(item, state);

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

Incorporated applying padding twice but not for the cases where the close is visible for unselected tabs.
In case the close is visible for the unselected tabs (via property showUnselectedClose), applying padding twice results in large spacing between tabs as follow:

Hence with applying padding only once for unselected tabs, when close is visible, looks more uniform as follows:

I see the problem. But isn't the goal that the text looks centered within the tab and the close button just shown up within the padding once you hover over it? Then I would propose to not add the close button size in this case instead of just applying half the padding, because that will visually break once you change the padding value.

For example, if you increase the padding value to 30, it would look like this:

When always applying twice the padding and instead not applying the close button size in case unselected images are not shown, it would look as follows:

In that case, code is reverted to adding the the padding twice uniformly to the tab width.
Also, in case close button is visible for unselected tab, the close button size is still added to tab width. If we remove the close button size, the text gets truncated and the padding would need to be increased.

With the goal to center the text within tab, close button size wouldn't matter much.

HeikoKlare · 2023-11-28T16:28:28Z

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

 						if ((state & SWT.SELECTED) != 0 || parent.showUnselectedClose) {
 							if (width > 0) width += INTERNAL_SPACING;


To me, it feels strange if this internal spacing is also applied when no images are shown, as it leads to a large padding right to the text, especially in case the padding has a significantly higher value.

Suggested change

if ((state & SWT.SELECTED) != 0 || parent.showUnselectedClose) {

if (width > 0) width += INTERNAL_SPACING;

if (((state & SWT.SELECTED) != 0 && parent.showSelectedImage)

|| ((state & SWT.SELECTED) == 0 && parent.showUnselectedImage)) {

if (width > 0) width += INTERNAL_SPACING;

}

if ((state & SWT.SELECTED) != 0 || parent.showUnselectedClose) {

The INTERNAL_SPACING here is applied in case the close is visible for tabs. I believe this has to be there irrespective of the current change.
Also, the spacing is applied only when no images are shown. The primary objective of applying the padding is to have proper visual separation between tabs when icons/images are not visible.

I believe there is a gap in my understanding.
@HeikoKlare Could you please elaborate a little in what you wanted to convey via this suggested change?

Sorry for the confusion. It's not a gap in your understanding, just something must have went wrong when I added the suggestion. The actual suggestion should be different. Let me explain it textually: The code here does two things: first, it adds the INTERNAL_SPACING to the width; seconds, it adds the size of the close button. The latter must always be done, that is correct. But the former one is, from my understanding, supposed to ensure that between the text and the close button there is some spacing. However, when icons are disabled there is already a large gap and adding this addition spacing makes the padding to the left and right of the text uneven.

This is a screenshot with additional spacing (current state):

And this without (my proposal):

It is only a small different, but it makes the text look slightly uncentered (because it actually is uncentered).

One note: my explanation is conflicting with your statement:

Also, the spacing is applied only when no images are shown.

Maybe I missed something in the code, but my understand is that this code will always be excuted when the tab to be drawn has a close icon.

@HeikoKlare Thanks for clarifying. The code is now updated accordingly and the tab text is now centered.

Before

After

Also, the spacing is applied only when no images are shown.

Maybe I missed something in the code, but my understand is that this code will always be excuted when the tab to be drawn has a close icon.

You are right. The code here gets executed when tab has a close icon. I misunderstood it before.

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

...wt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_custom_CTabFolder.java

shubhamWaghmare-sap · 2023-12-04T09:41:58Z

Thanks for the improvements to this PR and sorry for the late reply.

In general, I am fine with the changes. I only have two concerns with respect the padding:

I would propose to separate the padding for the case that icons are not shown from the existing INTERAL_SPACING (in a variable such as TABS_WITHOUT_ICONS_PADDING) as they apply to different behaviors and should thus not influence each other.

I would even be in favor of having a bigger spacing when no icons are shown. This seems to require 1. to work properly (see my detailed comments).

To me, a value like 14 for such a TABS_WITHOUT_ICONS_PADDING feels good : But that may be a matter of personal preference, it might also be smaller but at least should be somehow independent from the existing spacing.

Thanks @HeikoKlare for the review.
I agree with the bigger spacing. To me it looks much more clean and provides better visual separation between tabs. The same has been incorporated in latest commit.

Also the padding is separated to a new constant TABS_WITHOUT_ICONS_PADDING

HeikoKlare

The changes look good to me now. I have one nitpicky finding with respect to the text padding left.
@shubhamWaghmare-sap please also squash all your commits into one in preparation of merging this PR.
@sratz you have an open change request. Have your concerns be addressed or do you want to re-review?

Finally, I have one comment left but only for the sake of documentation, as I do not have a solution for that, so it should not affect this PR:
To me the behavior when showing the close button for unselected tabs does not yet feel right for me. The padding and text placement calculation is equal to when the close button is shown (for the selected tab). This is fine for the case when hovering over an unselected tab, but does not feel right for the other tabs (consider the "Tip of the Day" tab in the following screenshot). But since only one of these cases can be optimized without having tab headers jumping around, I do not see a better solution for this.

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

HeikoKlare

Thanks for this great contribution. Content-wise, the changes are ready to be merged from my side.

@shubhamWaghmare-sap Can you please squash the commits in this PR into one?

@sratz Can we remove your change request or is there anything left to do?

sratz · 2023-12-13T15:07:54Z

Looks good.

@shubhamWaghmare-sap: Can you rebase on top of master?

tab icon is not visible 1. Added showSelectedImage option to CTabFolder 2. Added showSelectedTabImage and showUnselectedTabImage methods to renderer

shubhamWaghmare-sap · 2023-12-14T11:19:52Z

@sratz @HeikoKlare
Thanks for all the comments and suggestions and approving the changes.
I have squashed all commits and re-based on top of master.

HeikoKlare · 2023-12-14T12:08:31Z

@shubhamWaghmare-sap Thank for this contribution! Looking forward to having the Platform UI PR merged and the feature available in Eclipse.

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolder.java

merks · 2023-12-16T13:32:57Z

Is it an unintended side effect that the tabs are so much wider now?

It doesn't look very nice...

HeikoKlare · 2023-12-16T14:57:28Z

Is it an unintended side effect that the tabs are so much wider now?

Yes. it is.

It doesn't look very nice...

True.

The larger padding is intended to applied to tab folders that usually contain images but that may be disabled (such as tab folder of Eclipse views: eclipse-platform/eclipse.platform.ui#1071). But it currently applies to all tabs for which showing images is disabled or that have no image assigned:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java

Lines 396 to 398 in 96692a5

    
           if (image == null || image.isDisposed() || ((state & SWT.SELECTED) != 0 && !parent.showSelectedImage) 
        
           		|| ((state & SWT.SELECTED) == 0 && !parent.showUnselectedImage)) 
        
           	return TABS_WITHOUT_ICONS_PADDING;

The latter is true for all existing tab folders without images, such as the one in the screenshot. So the functionality added with this PR has to be improved, either by only applying the padding when images are explicitly disabled or by adding a propery for a tab folder that defines whether the new behavior (no images but larger padding) shall be enabled.

sratz · 2023-12-18T13:16:11Z

either by only applying the padding when images are explicitly disabled or by adding a propery for a tab folder that defines whether the new behavior (no images but larger padding) shall be enabled.

Alternatively: Would it make sense to expose

TABS_WITHOUT_ICONS_PADDING

as a method rather than a constant (with backwards-compatible default value), such that the subclass in platform.ui can control (and potentially fine-tune via preferences) this behavior?

HeikoKlare · 2023-12-18T14:13:27Z

Alternatively: Would it make sense to expose

TABS_WITHOUT_ICONS_PADDING

as a method rather than a constant (with backwards-compatible default value), such that the subclass in platform.ui can control (and potentially fine-tune via preferences) this behavior?

Making the value configurable (potentially also via preferences) sound like a good idea to me. I was not aware that platform.ui has a specialization of this renderer, so providing the padding value via method and overwriting it in platform.ui would be possible. It could also be made "dynamically" configurable via some setter logic. Then you would not need to make a subclass for overwriting the value but can just do that by instantiating the renderer.

…clipse-platform#945 Currently, all headers in CTabFolders without an image have a large text padding. This applies to both the case where no image is assigned to a tab item and the case where images shall not be shown as the properties showSelectedImage and showUnselectedImage are both false. While the latter is intended behavior to switch between image-based tab folders and image-less, padding-based tab folders (see eclipse-platform#785), the former unintentionally breaks with the existing UI experience for CTabFolders that do not use images. This change makes the identification of whether large text padding shall be applied explicit by factoring it out into a method (which may be replaced by a different implementation later on). It reduces the cases in which the large text padding is applied to the intended case (showSelectedImage = showUnselectedImage = false) and restores the behavior for all existing use cases. Contributes to eclipse-platform#945

Currently, all headers in CTabFolders without an image have a large text padding. This applies to both the case where no image is assigned to a tab item and the case where images shall not be shown as the properties showSelectedImage and showUnselectedImage are both false. While the latter is intended behavior to switch between image-based tab folders and image-less, padding-based tab folders (see #785), the former unintentionally breaks with the existing UI experience for CTabFolders that do not use images. This change makes the identification of whether large text padding shall be applied explicit by factoring it out into a method (which may be replaced by a different implementation later on). It reduces the cases in which the large text padding is applied to the intended case (showSelectedImage = showUnselectedImage = false) and restores the behavior for all existing use cases. Contributes to #945

praveen-skp mentioned this pull request Aug 29, 2023

Add Options to hide tab icons and and show full text in view area eclipse-platform/eclipse.platform.ui#1071

Merged

sratz changed the title ~~Handle selected tab icon~~ CTabFolder: Add 'showSelectedImage' property Aug 29, 2023

sratz requested changes Sep 25, 2023

View reviewed changes

HeikoKlare reviewed Sep 26, 2023

View reviewed changes

shubhamWaghmare-sap requested review from sratz and HeikoKlare October 5, 2023 10:50

HeikoKlare reviewed Nov 28, 2023

View reviewed changes

HeikoKlare reviewed Dec 11, 2023

View reviewed changes

...eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolderRenderer.java Show resolved Hide resolved

HeikoKlare approved these changes Dec 13, 2023

View reviewed changes

sratz approved these changes Dec 13, 2023

View reviewed changes

Added image/icon handling options for tabs and improved text padding if

473ffaa

tab icon is not visible 1. Added showSelectedImage option to CTabFolder 2. Added showSelectedTabImage and showUnselectedTabImage methods to renderer

shubhamWaghmare-sap force-pushed the handle_selected_tab_icon branch from 89e25ca to 473ffaa Compare December 14, 2023 10:53

sratz merged commit dacba96 into eclipse-platform:master Dec 14, 2023
13 checks passed

BeckerWdf assigned shubhamWaghmare-sap Dec 14, 2023

BeckerWdf added this to the 4.31 M1 milestone Dec 14, 2023

fedejeanne reviewed Dec 15, 2023

View reviewed changes

...les/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/CTabFolder.java Show resolved Hide resolved

HeikoKlare mentioned this pull request Dec 16, 2023

Bump org.eclipse.swt version due to new API #944

Merged

HeikoKlare mentioned this pull request Dec 18, 2023

Unexpected padding in CTabFolders without images #945

Open

HeikoKlare mentioned this pull request Dec 19, 2023

Apply large padding only when image for tab item shall not be shown #945 #947

Merged

HeikoKlare mentioned this pull request Jan 19, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTabFolder: Add 'showSelectedImage' property #785

CTabFolder: Add 'showSelectedImage' property #785

shubhamWaghmare-sap commented Aug 29, 2023 •

edited

Loading

github-actions bot commented Aug 29, 2023 •

edited

Loading

HeikoKlare left a comment

HeikoKlare commented Oct 5, 2023

shubhamWaghmare-sap commented Oct 5, 2023

HeikoKlare left a comment

HeikoKlare Nov 28, 2023

shubhamWaghmare-sap Dec 4, 2023

HeikoKlare Dec 6, 2023

shubhamWaghmare-sap Dec 7, 2023 •

edited

Loading

HeikoKlare Nov 28, 2023

shubhamWaghmare-sap Dec 4, 2023

HeikoKlare Dec 6, 2023

shubhamWaghmare-sap Dec 7, 2023

shubhamWaghmare-sap commented Dec 4, 2023 •

edited

Loading

HeikoKlare left a comment

HeikoKlare left a comment

sratz commented Dec 13, 2023

shubhamWaghmare-sap commented Dec 14, 2023

HeikoKlare commented Dec 14, 2023

merks commented Dec 16, 2023

HeikoKlare commented Dec 16, 2023

sratz commented Dec 18, 2023 •

edited

Loading

HeikoKlare commented Dec 18, 2023

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

		if ((state & SWT.SELECTED) != 0 \|\| parent.showUnselectedClose) {
		if (width > 0) width += INTERNAL_SPACING;

CTabFolder: Add 'showSelectedImage' property #785

CTabFolder: Add 'showSelectedImage' property #785

Conversation

shubhamWaghmare-sap commented Aug 29, 2023 • edited Loading

github-actions bot commented Aug 29, 2023 • edited Loading

Test Results

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare commented Oct 5, 2023

shubhamWaghmare-sap commented Oct 5, 2023

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare Nov 28, 2023

Choose a reason for hiding this comment

shubhamWaghmare-sap Dec 4, 2023

Choose a reason for hiding this comment

HeikoKlare Dec 6, 2023

Choose a reason for hiding this comment

shubhamWaghmare-sap Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

HeikoKlare Nov 28, 2023

Choose a reason for hiding this comment

shubhamWaghmare-sap Dec 4, 2023

Choose a reason for hiding this comment

HeikoKlare Dec 6, 2023

Choose a reason for hiding this comment

shubhamWaghmare-sap Dec 7, 2023

Choose a reason for hiding this comment

shubhamWaghmare-sap commented Dec 4, 2023 • edited Loading

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

sratz commented Dec 13, 2023

shubhamWaghmare-sap commented Dec 14, 2023

HeikoKlare commented Dec 14, 2023

merks commented Dec 16, 2023

HeikoKlare commented Dec 16, 2023

sratz commented Dec 18, 2023 • edited Loading

HeikoKlare commented Dec 18, 2023

shubhamWaghmare-sap commented Aug 29, 2023 •

edited

Loading

github-actions bot commented Aug 29, 2023 •

edited

Loading

shubhamWaghmare-sap Dec 7, 2023 •

edited

Loading

shubhamWaghmare-sap commented Dec 4, 2023 •

edited

Loading

sratz commented Dec 18, 2023 •

edited

Loading