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.

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

Add capability for styling DocumentControl tabs #2574

Conversation

eng-myousif
Copy link
Contributor

@eng-myousif eng-myousif commented Nov 7, 2023

This PR is aiming to expose some feature for DocumentControl the following:
1- It is possible to set the Background and Foreground colors for Tabs and Close ( x ) button.
2- It is possible to cancel a closing a page when attempt to close it using close ( x ) button.

@eng-myousif eng-myousif force-pushed the mo_youssef/enable_styling_document_control_tabs branch from c77e57e to d0cc489 Compare November 7, 2023 10:07
src/Eto/Forms/Controls/DocumentControl.cs Outdated Show resolved Hide resolved
@@ -417,8 +584,11 @@ void Drawable_Paint(object sender, PaintEventArgs e)

posx = 0;

DrawTab(g, tabPrev, -1);
DrawTab(g, tabNext, -1);
if (allowNavigating)

This comment was marked as off-topic.

src/Eto/Forms/Controls/DocumentControl.cs Outdated Show resolved Hide resolved
@eng-myousif eng-myousif force-pushed the mo_youssef/enable_styling_document_control_tabs branch from d0cc489 to c4bb3ff Compare November 7, 2023 10:50
@eng-myousif eng-myousif changed the title Mo youssef/Enable styling DocumentControl tabs Add capability for styling DocumentControl tabs Nov 8, 2023
@cwensley
Copy link
Member

cwensley commented Nov 8, 2023

Hey @eng-myousif, thanks a ton for the PR! This looks great.

A few things, first I don't think the color properties should be on the DocumentControl itself, only on ThemedDocumentControlHandler, which one could use a style or set directly with that class. I don't foresee these ever being able to be supported in the same way with any other implementation.

Additionally, it would be great to see the PageClosing event and HasNavigationButtons implemented for the Gtk handler if possible.

As for the name of HasNavigationButtons I would consider using an Allow or Use prefix, as it's something you can set instead of just query.

g.DrawImage(tab.Image, tabRect.X + TabPadding.Left + (maxImageSize.Width - imageSize.Width) / 2, (tabDrawable.Height - imageSize.Height) / 2);
g.SaveTransform();
g.ImageInterpolation = ImageInterpolation.High;
var imageSize = tab.ImageSize.IsEmpty ? tab.Image.Size : tab.ImageSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Having ImageSize public property and setting it to a deliberately "wrong" value can end up in less-than-beautiful results. I would argue the desired image size should be lead by the ThemedDocumentControl itself (it knows the dimensions of the tabs it renders and it also should know the ideal dimensions for the image inside that tab).

Shall we just scale tab.Image up/down to fit that ideal rectangle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got your point, I will revert changes for exposing ImageSize and let you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cwensley ,

  • I moved the color properties to ThemedDocumentControlHandler

  • Implemented PageClosing for the Gtk handler.

  • Couldn't implement HasNavigationButtons (now called HasNavigationButtons) for the Gtk handler as the gtk Nootbook control doesn't allow to hide the HasForwardStepper and HasBackwordStepper because they are readonly, Hence I move the property to the ThemedDocumentControlHandler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ievgen-baida ,
I have introduce a property UseFixedTabHeight along with a checkbox in the DocumentControlSection demo dialog. When set UseFixedTabHeight to true it resizes the images inside the Tab height, then we have a fixed tab height that is calculated by the font size and vertical padding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

Only thing, I wonder if instead of relying on the font size when calculating the height of the image we would rather use some sensible standard size, like 16x16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ievgen-baida, sounds like a good idea. Applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. Looks good to me.

@eng-myousif eng-myousif force-pushed the mo_youssef/enable_styling_document_control_tabs branch from 5463328 to 6389fe0 Compare November 14, 2023 08:50
@eng-myousif eng-myousif force-pushed the mo_youssef/enable_styling_document_control_tabs branch from e99d2eb to 2b57282 Compare November 16, 2023 08:55
@cwensley
Copy link
Member

Hey @eng-myousif, this looks great! Let me know when it's ready and I'll merge in the PR.

@ievgen-baida
Copy link
Contributor

Hi @cwensley, we finalized it from our side. You can merge it.

Would it be possible to create a patch release, so that we could use it in production?

@cwensley cwensley merged commit adfa141 into picoe:develop Nov 17, 2023
3 checks passed
@cwensley
Copy link
Member

Hey @ievgen-baida, yes I can certainly create a patch release. I'll try to do that shortly.

@cwensley cwensley added this to the 2.8.2 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants