create a FlatButtonWithToolTip component for MediaPlayerControl
ClosedPublic

Authored by astippich on Jul 3 2018, 5:34 PM.

Details

Summary

Factors out the code of MediaPlayerControl to a button component so that the code is reused
and make it compatible with qqc2 actions
T7576

Diff Detail

Repository
R255 Elisa
Branch
elisa_button
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2229
Build 2247: arc lint + arc unit
astippich requested review of this revision.Jul 3 2018, 5:34 PM
astippich created this revision.
astippich edited the summary of this revision. (Show Details)Jul 3 2018, 5:39 PM
astippich edited the summary of this revision. (Show Details)
astippich updated this revision to Diff 40411.Aug 25 2018, 11:30 AM

rebase on top of master

ping, rolling this out would be a great step forward for a pure qqc2 application

I do not understand your intent with this patch. Especially the way you provide a special ToolButton with a lot of things in it.
Please explain better why each element in ElisaToolButton is here ?

I have spent some more time looking at your patch and fail to see the rationale behind the way tooltips are added. Did you try the ToolTip attached property ? That would be the "good" way to do it.

As an example, I did try this:

ToolButton {
    objectName: 'enqueueButton'
    text: i18nc("Add current list to playlist", "Enqueue")
    icon.name: "media-track-add-amarok"

    ToolTip.visible: hovered
    ToolTip.delay: Qt.styleHints.mousePressAndHoldInterval
    ToolTip.text: "hello world"

    onClicked: enqueue()

    Layout.leftMargin: 0
    Layout.rightMargin: 0
}

As far as I can tell, this is doing a fine job. We might want to add an Elisa component to modify the default values for "ToolTip.visible" and "ToolTip.delay" as a way to avoid duplicating too much lines.

I do not understand your intent with this patch. Especially the way you provide a special ToolButton with a lot of things in it.
Please explain better why each element in ElisaToolButton is here ?

The intent is to move to QtQuickControls2 for the entire app. QtQuickControls1 is deprecated, QQC2 should be lighter on resource usage, and should simplify HighDPI handling.

Believe me, I did not want to implement a custom ToolButton, but I've found the one provided by Qt insufficient for our needs, which is described in the summary. The standard QQC2 Toolbutton reacts by default to spacebar, which makes it unusable to use this as a shortcut.
Secondly, it does not show tooltips from the action name. The most important issue is that the ToolButton always displays an icon _and_ a string when used with an Action component, which is found everywhere in Elisa.
Thus I created a custom ToolButton, which is compatible to the Action component and switched the MediaPlayerControls as a first example to it. MediaPlayerControls actually already implements a somewhat custom made QQC2 ToolButton. I implemented Action components for the buttons like everywhere else in Elisa to make it consistent and used the new ToolButton component. This alone provides a nice cleanup.

The plan is to switch all remaining ToolButtons to this new component if it is accepted.

As an example, I did try this:

ToolButton {
    objectName: 'enqueueButton'
    text: i18nc("Add current list to playlist", "Enqueue")
    icon.name: "media-track-add-amarok"
 
    ToolTip.visible: hovered
    ToolTip.delay: Qt.styleHints.mousePressAndHoldInterval
    ToolTip.text: "hello world"
 
    onClicked: enqueue()
 
    Layout.leftMargin: 0
    Layout.rightMargin: 0
}

As far as I can tell, this is doing a fine job. We might want to add an Elisa component to modify the default values for "ToolTip.visible" and "ToolTip.delay" as a way to avoid duplicating too much lines.

I didn't know about this, the code was copied from our LabelWithToolTip component, but I'm happy to switch over to the standard one. If you use this example with an Action component with text and icon.name, it shows both, so this issue remains.

astippich updated this revision to Diff 42098.Sep 21 2018, 3:35 PM
  • use tooltip directly

I still have high doubt with your current approach (ElisaToolButton). See my inline comment.
I fear that with your patch patch we increase the maintenance cost and add more things to learn for newcommers.

src/qml/ElisaToolButton.qml
45–61

I am definitely not a fan of custom styling. I did it for two progress bar in the media control bar. I still regret it and still believe that was an error.
We are not a big team working on Elisa. All this code increase the maintenance cost. Is it really worth ?

astippich added inline comments.Oct 12 2018, 8:05 PM
src/qml/ElisaToolButton.qml
45–61

I would also like to not go this route, but I really haven't found another way (open for suggestions!). I would very much like to convert Elisa to a pure qqc2 application, since qqc1 is officially deprecated, and qqc2 simplifies or actually enables proper HiDPI support, and is said to be lighter on resources. And last but not least the behavior on spacebar push for the standard qqc2 toolbutton component.
Imho this outweighs the disadvantages, but if you disagree, just give me a definite no and I will abandon this diff.

mgallien added inline comments.Oct 14 2018, 7:18 PM
src/qml/ElisaToolButton.qml
45–61

I have spent quite some time thinking about your answer.

I would like to propose to you that we first work on a full port to QQC2 and later look again at the spacebar question. I am not happy to differ in behavior from normal controls (especially in a desktop application). Is this working for you ?
I know that there is also the question about the icons+test buttons. I will have to look at this also but would prefer to have one patch for each solution.

astippich added inline comments.Oct 15 2018, 8:15 PM
src/qml/ElisaToolButton.qml
45–61

Fine by me, the spacebar issue is only of secondary importance for me here.
So, would you accept this revision without the spacebar override or would you like to investigate the icon+text issue before?

mgallien added inline comments.Oct 17 2018, 3:39 PM
src/qml/ElisaToolButton.qml
45–61

I will need more time to understand why we need to override the default behavior. I will be mostly offline until Monday evening. I am really busy with real life. I will try to come with an answer on Monday. I would say that I would prefer not to override the appearance such that any style is still able to make our buttons look native.

astippich added inline comments.Oct 17 2018, 6:48 PM
src/qml/ElisaToolButton.qml
45–61

I think I've found the issue. Since you talked about the style, I changed the default style for Elisa, and then the text does not appear. So maybe the qqc2 style is broken. I will do some more investigations

see D16284

I have been working on this subject to help come this diff come to a conclusion. Thanks for your work on this as it is important.

I believe that we have been overusing ToolButton at places just to get a flat look instead of doing the proper job (i.e. customizing the look of a Button component).

I also better understand (thanks to D16284) why we get icon+text. I believe that this will not change as it is done on purpose to maximize compatibility with QWidget equivalent of ToolButton. We are the guilty ones here given we had used ToolButton components in places where we expected a custom behavior (i.e. no text).

I have a private (at the moment) branch with no QQCv1 components. There are several regressions. I had kept overusing ToolButton like in master branch.

I would like to try to come up with a long term and maintainable solution for each of our button cases:

  • flat button with text and icon ;
  • flat button with tooltip and icon ;
  • flat round button with icon ;

The result should be fully usable with keyboard only, mouse+keyboard and touch screens. That means that ToolButton as is is not the solution because the focus behavior is not going to work for keyboard only usage.
Our solution should work with org.kde.desktop and Fusion style (at least).

see D16284

I have been working on this subject to help come this diff come to a conclusion. Thanks for your work on this as it is important.

I believe that we have been overusing ToolButton at places just to get a flat look instead of doing the proper job (i.e. customizing the look of a Button component).

I also better understand (thanks to D16284) why we get icon+text. I believe that this will not change as it is done on purpose to maximize compatibility with QWidget equivalent of ToolButton. We are the guilty ones here given we had used ToolButton components in places where we expected a custom behavior (i.e. no text).

Though since QQCv1 and the Fusion style are behaving the same, i.e. no text, may also be a valid reason to change the behavior of the desktop style.

I have a private (at the moment) branch with no QQCv1 components. There are several regressions. I had kept overusing ToolButton like in master branch.

I would like to try to come up with a long term and maintainable solution for each of our button cases:

  • flat button with text and icon ;
  • flat button with tooltip and icon ;
  • flat round button with icon ;

    The result should be fully usable with keyboard only, mouse+keyboard and touch screens. That means that ToolButton as is is not the solution because the focus behavior is not going to work for keyboard only usage. Our solution should work with org.kde.desktop and Fusion style (at least).

Alright, let me know when and where I can help or test.

astippich updated this revision to Diff 44665.Nov 1 2018, 7:37 PM
  • do not use toolbutton
astippich retitled this revision from introduce ElisaToolButton and use it in MediaPlayerControl to create a ButtonWithToolTip component for MediaPlayerControl.Nov 1 2018, 7:39 PM
astippich edited the summary of this revision. (Show Details)

see D16284

I have been working on this subject to help come this diff come to a conclusion. Thanks for your work on this as it is important.

I believe that we have been overusing ToolButton at places just to get a flat look instead of doing the proper job (i.e. customizing the look of a Button component).

I also better understand (thanks to D16284) why we get icon+text. I believe that this will not change as it is done on purpose to maximize compatibility with QWidget equivalent of ToolButton. We are the guilty ones here given we had used ToolButton components in places where we expected a custom behavior (i.e. no text).

I have a private (at the moment) branch with no QQCv1 components. There are several regressions. I had kept overusing ToolButton like in master branch.

I would like to try to come up with a long term and maintainable solution for each of our button cases:

  • flat button with text and icon ;
  • flat button with tooltip and icon ;
  • flat round button with icon ;

    The result should be fully usable with keyboard only, mouse+keyboard and touch screens. That means that ToolButton as is is not the solution because the focus behavior is not going to work for keyboard only usage. Our solution should work with org.kde.desktop and Fusion style (at least).

I've gone ahead and factored out the current button code that was copied all over in MediaPlayerControl. As it is now, it's mostly a refactoring only with the exception that actions are now used.
Imho we don't need a round button anymore.
If you agree on the implementation, I will roll this out to all qqc1 toolbuttons in the views and playlist.

see D16284

I have been working on this subject to help come this diff come to a conclusion. Thanks for your work on this as it is important.

I believe that we have been overusing ToolButton at places just to get a flat look instead of doing the proper job (i.e. customizing the look of a Button component).

I also better understand (thanks to D16284) why we get icon+text. I believe that this will not change as it is done on purpose to maximize compatibility with QWidget equivalent of ToolButton. We are the guilty ones here given we had used ToolButton components in places where we expected a custom behavior (i.e. no text).

I have a private (at the moment) branch with no QQCv1 components. There are several regressions. I had kept overusing ToolButton like in master branch.

I would like to try to come up with a long term and maintainable solution for each of our button cases:

  • flat button with text and icon ;
  • flat button with tooltip and icon ;
  • flat round button with icon ;

    The result should be fully usable with keyboard only, mouse+keyboard and touch screens. That means that ToolButton as is is not the solution because the focus behavior is not going to work for keyboard only usage. Our solution should work with org.kde.desktop and Fusion style (at least).

I've gone ahead and factored out the current button code that was copied all over in MediaPlayerControl. As it is now, it's mostly a refactoring only with the exception that actions are now used.
Imho we don't need a round button anymore.
If you agree on the implementation, I will roll this out to all qqc1 toolbuttons in the views and playlist.

I lack time until next week for a proper review.

I agree about the round buttons.

Do you have time to add support for flat buttons in org.kde.desktop style ?

We will also need to fix the platform menu support. Currently they look alien in Plasma.

I lack time until next week for a proper review.

Take your time. Please note there is still custom styling involved, but it's no step backwards as we already have those buttons in the MediaPlayerControl.
I would still like to merge it this way and roll it out to the other buttons. We can do the proper fix for this button component later, since it will probably take quite some until we can require a frameworks version that has the proper fixes.

I agree about the round buttons.

Do you have time to add support for flat buttons in org.kde.desktop style ?

I'll see what I can do.

We will also need to fix the platform menu support. Currently they look alien in Plasma.

I have a somewhat working version locally using a QQC2, item-based menu. It currently lacks the display of the shortcut, but displays the icon and the text.
On the other hand, the qt.platforms menu doesn't show the icon, but the shortcut if I remember correctly. I'll also try to have a look at this issue.

astippich updated this revision to Diff 44830.Nov 4 2018, 10:47 AM
  • rename to FlatButtonWithToolTip

D16659 and its dependent revision will allow to remove custom styling in the future

astippich retitled this revision from create a ButtonWithToolTip component for MediaPlayerControl to create a FlatButtonWithToolTip component for MediaPlayerControl.Nov 4 2018, 10:50 AM
mgallien accepted this revision.Nov 17 2018, 8:38 AM
This revision is now accepted and ready to land.Nov 17 2018, 8:38 AM
This revision was automatically updated to reflect the committed changes.