-
Notifications
You must be signed in to change notification settings - Fork 144
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.
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
Moving TabFolderLayout into SWT. #1402
Conversation
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/TabFolderLayout.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/TabFolderLayout.java
Outdated
Show resolved
Hide resolved
489204f
to
6959382
Compare
Done! |
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.
This class totally lacks documentation compared to rest of SWT. From just looking at it I have zero clue what/why/how/when to use it. Please add proper documentation before it can be accepted in SWT.
Can you be more specific?
e.g I see this
As explained in the referenced issues, this is somehow not often used directly (but not prohibited of course) if I look for example at So while better documentation is always good, I think its hard to ask the person that wants to help deduplication to do it, so I would more see this as something for all SWT/Platform commiters to enhance after we have deduplicated the code if there are anything left that is really unclear in its usage. |
* | ||
* @since 3.127 | ||
*/ | ||
class TabFolderLayout extends Layout { |
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.
Can this really be package private?
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.
@deepika-u You haven't answered here? How is this supposed to be used from platform.ui?
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.
Yeah totally agree when no modifier is mentioned it'll be defaulted to private. For this class to be available for platform.ui it needs to be public. It is done now.
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.
@akurtakov : Is anything being missed to be done from my side? Please do let me know.
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.
I think it would be good if this PR would include setting this layout in the TabFolder
as a default layout, this would also make it more clear how/when/why use it.
If it should be package private I think it needs to reside in the same package as TabFolder
Well, I still don't understand what the effects of using this one are. |
Also looking into the issue all the classes that are to be deduplicated are "internal" so they are not supposed to be generally useful which explains (to some extend) lack of documentation. I don't ask for big documentation but at least some basic one on what the effects of using it are. |
The effect is that otherwise you get randomly wrong display of Tabs, see
If they are not generally useful why we have 6 copies of the same code? ;-) |
One more observation this seems to be used with CTabFolder from the platform.ui rather than TabFolder. What happens when used with TabFolder (some screenshots please)? It should be clear whether it's for one or the other or both TabFolders and if the later maybe clarified in the name. |
I use it for |
Consider putting such an explanation as a class comment :) |
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/TabFolderLayout.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/TabFolderLayout.java
Outdated
Show resolved
Hide resolved
9143720
to
b63abcd
Compare
@akurtakov : Thanks alot. Now i'll try to focus on other repo changes for which this is the start. PDE, Platform and ui accordingly. |
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.
Thank you for driving this forward!
Moving TabFolderLayout into SWT.
Fixes #1317
This pr needs to be merged first and later eclipse-platform/eclipse.platform.ui#2186