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

Fixes #358 - fade-out instead elide text (trim with...) in tabs #538

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

asashnov
Copy link
Contributor

Fixes #358 - fade-out instead elide text (trim with...) in tabs

This is how it looks like with the change:
Screenshot from 2020-11-21 21-38-03

@asashnov asashnov requested a review from mgautierfr November 21, 2020 14:47
@kelson42 kelson42 self-requested a review November 21, 2020 15:12
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@asashnov Thank you for this promising PR but:

  • It applies to any title, even if very smal ("Bird" for example) and in fact there should be not fading because there is a lot of space.
  • I wonder if it would not be better practive to have the CSS in the stylesheet file, instead of having it mixed with the C++ code in setStyleSheet().

@kelson42
Copy link
Collaborator

@asashnov Is that ready to review again?

@asashnov
Copy link
Contributor Author

@kelson42 No, this PR is not ready yet.

I moved the style to .css file, but didn't found a way to use separate styles in QTabBar for each tab. Seems it should involve to create our own implementation.

Also, I'll think about implementing ::paint() method to render kind of fade-out effect on top of standard rendering.

@mgautierfr
Copy link
Member

Also, I'll think about implementing ::paint() method to render kind of fade-out effect on top of standard rendering.

You've arrived to the same conclusion than me. The css rule you apply is nice but it applies to all texts, even if they are not too long.
We haven't found a way to do it without reimplementing the paint method of the widget.
This is obviously doable, but we need to understand the low level parts of Qt.

@kelson42
Copy link
Collaborator

And this is not possible to have the width of the widget and do the math to apply the CSS only if necessary (width big enough)?

@stale
Copy link

stale bot commented Dec 4, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Dec 4, 2020
@asashnov
Copy link
Contributor Author

asashnov commented Dec 8, 2020

@kelson42 I found a way to get QRect for text in the tab.
But stylesheet cannot be applied only to specified tabs (see my comment in the issue).

Instead, we can fillRect() in paintEvent() and use a gradient, the tab background color, and the alpha channel to imitate the effect we had with this CSS.

Screenshot from 2020-12-09 01-00-50

In this screenshot the background color for the active tab is wrong, and no alpha channel gradient, but the position of QRect is right.
Also, flags if we need this trick or not is also set correctly.

@stale
Copy link

stale bot commented Dec 17, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Dec 17, 2020
@asashnov
Copy link
Contributor Author

asashnov commented Dec 18, 2020

Unfortunately, there is NO an easy way to get the background color of selected tab like:

  QColor c0;
  c0 = tab.palette.color(QPalette::Active, QPalette::Button);
  c0 = QApplication::style().standardPalette().color(QPalette::Active, QPalette::Button);
  c0 = palette().color(QPalette::Active, QPalette::Button);

(I tried many variants also)

Since we install our own custom style from 'style.css'
QApplication::style() returns QStyleSheetStyle object.

This class is a private part of Qt (no public header for it).

Moreover, in its implementation (see Qt/5.12.6/Src/qtbase/src/widgets/styles/qstylesheetstyle.cpp)
it declares local for this .cpp file class QRenderRule
which actually takes case of getting the exact CSS rule for selected tab.

Also, I saw in the sources there is really no way to specify tab by tab number.

For more details see:

void QStyleSheetStyle::drawControl(ControlElement ce, const QStyleOption *opt, QPainter *p,
            const QWidget *w) const
{
...
case CE_TabBarTabShape:
  ...
   QRenderRule subRule = renderRule(w, opt, PseudoElement_TabBarTab);


`QStyle` and its derived classes have no interface for getting colors back.
They can only draw a control or its part on some `QPaintDevice`.

So we have no easy way here to get back our 'white' color we set our style.css file:

```css
QTabBar::tab:selected {
  background-color: white;
  border-bottom: 2px solid white;
}

So I decided to just hardcode it in the code and write comments that it should be in sync in both places.

An alternative solution may involve 'picking' this active tab background color from the widget
after it has been painted by QTabBar::paintEvent().

This is how it looks now:

Screenshot from 2020-12-19 01-33-16

@kelson42
Copy link
Collaborator

@mgautierfr Please review

@kelson42 kelson42 force-pushed the 358-fade-out-text-in-tabs-title branch from 7c4e08c to 7e0c903 Compare December 20, 2020 14:27
@kelson42
Copy link
Collaborator

@asashnov I have rebased on git master and test it. Work perfectly I believe. Might need a bit of rework at the time we will support the "dark mode" and user modes... but this is an other topic. Thank you very much.

@kelson42 kelson42 self-requested a review December 20, 2020 14:29
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

On the user side looks good. Waiting @mgautierfr review for the code.

@stale
Copy link

stale bot commented Dec 28, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Dec 28, 2020
@asashnov asashnov self-assigned this Jan 6, 2021
@stale stale bot removed the stale label Jan 6, 2021
@stale
Copy link

stale bot commented Jan 15, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Jan 15, 2021
@asashnov asashnov force-pushed the 358-fade-out-text-in-tabs-title branch from 7e0c903 to 474c758 Compare February 8, 2021 12:30
@stale stale bot removed the stale label Feb 8, 2021
@asashnov
Copy link
Contributor Author

asashnov commented Feb 8, 2021

Rebased, squashed, long commentary removed (it still here in this 'Conversation').

@asashnov asashnov force-pushed the 358-fade-out-text-in-tabs-title branch from 474c758 to a1e00e9 Compare February 8, 2021 12:36
@asashnov asashnov force-pushed the 358-fade-out-text-in-tabs-title branch from a1e00e9 to c621079 Compare February 8, 2021 12:39
Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

We are definitively on the good way but it seems there is a small issue with RightToLeft text.

src/tabbar.cpp Outdated Show resolved Hide resolved
src/tabbar.cpp Outdated Show resolved Hide resolved
@asashnov asashnov force-pushed the 358-fade-out-text-in-tabs-title branch from b4cc704 to 8759c81 Compare February 12, 2021 18:27
@asashnov
Copy link
Contributor Author

Qt::LayoutDirection - Specifies the direction of Qt's layouts and text handling (https://doc.qt.io/qt-5/qt.html#LayoutDirection-enum)

When we open arabic wiki, and see arabic in tab's title, we change just 'text layout'.

But my code checked 'widget layout' of the application- that was a problem.

Fixed: In the current implementation the text layout direction is given, not application widgets layout.

@asashnov asashnov force-pushed the 358-fade-out-text-in-tabs-title branch from 8759c81 to c060776 Compare February 15, 2021 08:15
@asashnov asashnov force-pushed the 358-fade-out-text-in-tabs-title branch from c060776 to 7252b1b Compare February 21, 2021 16:37
@asashnov
Copy link
Contributor Author

We are definitively on the good way but it seems there is a small issue with RightToLeft text.

Fixed in 7252b1b (Feb 15, 2021)

@mgautierfr
Copy link
Member

We are good 🎉

Just mentioning that the the need to keep the selected_bg_color in sync with the css will be a bit complex with #320.
We cannot just have a const value. But this is a problem for #320 :)

@mgautierfr mgautierfr merged commit 4a37fb7 into master Feb 22, 2021
@mgautierfr mgautierfr deleted the 358-fade-out-text-in-tabs-title branch February 22, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace tab's title ellpsis by a fade-out
3 participants