Unexpected padding in CTabFolders without images

[HeikoKlare]

Describe the bug
The headers in CTabFolders always have a large padding if they either do not have a usable image assigned or if they are configured to now show icons. The latter case was the intention of #785, but as a side effect it also applies to when simply no image is provided (like in all existing CTabFolders without images).

The reason is that the extra padding is also applied when the image of a tab is null or disposed:

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

Lines 396 to 398 in f19db14

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

To Reproduce
The behavior can be seen when opening the CTabFolder in the CustomControlsExample.

Before:

After:

Expected behavior
CTabFolders without images shall be shown without extra padding like before #785.

Screenshots
The issue can, e.g., be seen in the plug-in settings. Originally posted in a comment of the original PR: #785 (comment)

Before:

After:

Version since
Since #785 / SWT 3.125.

[HeikoKlare]

@shubhamWaghmare-sap do you know why you decided to not only apply the padding when the setting says to do so (i.e., showSelectedImage == true) but also when no image is attached to the TabItem at all? If there is no specific reason, simply removing the application of the extra padding in case there is no image assigned (or if it is diposed) should solve the issue.

At least for me, changing the above posted snipped as follows solves the issue for me:

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

[shubhamWaghmare-sap]

The idea behind padding is to provide a clearly visible separation between tabs, when no image is available/visible for tabs.

With images visible, it is easy to identify the individual open tabs.

If images are not visible, without padding the separation is not very clear between two tabs i.e. which part of the text belongs to which tab is hard to identify, as also mentioned by you @HeikoKlare in eclipse-platform/eclipse.platform.ui#1071 (comment)

Hence the padding helps with the visual separation between tabs, when tab image is not visible or not available (is null or disposed) or when the setting says to not show image ( showSelectedImage or showUnSelectedImage)

Expected behavior
CTabFolders without images shall be shown without extra padding like before #785.

@HeikoKlare Could you please elaborate on why Expected behavior is desirable over the current behavior with padding when image is not assigned or disposed?

[merks]

This is how it looks now:

This is what it looked like before:

How it looks now seems way too much space. How it was before looks compact and I don't think we should so dramatically change the look for all the places that that this widget is used.

[HeikoKlare]

@HeikoKlare Could you please elaborate on why Expected behavior is desirable over the current behavior with padding when image is not assigned or disposed?

The problem here is with respect to compatbility. I like the option to have tab folder with large text padding as introduced with #785. But that behavior should be enabled explicitly (currently by setting showSelectedImage=false). The padding was, however, applied to all tabs that have no images, so even the existing usages of tab folders that have no images changed their appearance. That may be desired for some or even all of them, but then these use cases should make the decision on their own and explicitly configure their tab folders rather than having them changed by central configuration. Otherwise, existing UIs may rely on the small spacing and than completely break when updating SWT. Thus, this is comparable to an API breakage for me. I don't want to say that cannot make such a change in general. But in case we want to have that, we should first have a discussion on that and not only a decision of two committers while everyone else is confused after merging the change (like it was now). When approving the original PR, I was not aware of the effect on all CTabFolders but expected that to be behavior that needs to be configured explicitly, otherwise I would have proposed to start such a discussion already then.

An improvement of the current implementation could be to make the size of the padding configurable in specializations or even instances of the CTabFolderRenderer, as proposed by @satz: #785 (comment)

Meanwhile, the behavior for existing CTabFolder should be like before #785 due to #947. So after the next I-Build you should be able to update your Eclipse to have the pre-existing appearance again (@merks fyi).

[shubhamWaghmare-sap]

@HeikoKlare Thanks for the explanation.

I don't want to say that cannot make such a change in general. But in case we want to have that, we should first have a discussion on that and not only a decision of two committers while everyone else is confused after merging the change (like it was now).

Agreed. Makes sense. 👍
I'll evaluate more from my end to see if this side effect is visible elsewhere.
Thanks for #947

[shubhamWaghmare-sap]

Following are the open discussions for improvements from different issues around eclipse-platform/eclipse.platform.ui#1071 & #785 :

1. The requirement to make the size of padding configurable, either by [a] a setter on CTabFolder/CTabItem, or [b] specializations of CTabFolderRenderer, or via any other approach.

2. A configuration to enable/disable the large text padding via a property on CTabFolder or item, or may be by allowing specialization of shouldApplyLargeTextPadding method.

   • Ex: Allows to configure if the padding should also be applied for tabs without any image

3. Behavior of tab text padding in case the new preference to hide tab icons is selected and explicitly the CSS property swt-selected-image-visible is set to true. This is how the tabs look currently:
   

   The padding applied by the new "hide tab icons" preference when images are completely disabled for all tabs is removed as selected tab image is now visible.

   • Is there a need to have padding for the unselected tabs in this case for a clear visual separation?
     • Can be handled via [2]
     • Is a different color for tab border a possible solution?

Addressing these can provide flexibility and enhance the visual appearance of the tabs. Any comments on the "need" for each of the above mentioned improvements or any additions to the list are welcome.

[BeckerWdf]

What's the state of this issue? Does it still exist or is it fixed?

[HeikoKlare]

The original issue should be fixed (from an end users' perspective) via #947, but it is not a sufficient solution from a developer's point of view. In particular, there is no way to explicitly configure whether large paddings shall be used in tab headers. You can only defined that implicitly by a specific combination of showSelectedImage and showUnselectedImage properties:

2. A configuration to enable/disable the large text padding via a property on CTabFolder or item, or may be by allowing specialization of shouldApplyLargeTextPadding method.

I would propose to put the other ideas for improvement into a separate issue, as they are not directly related to the original issue here.