D11410: [RFC] Add option to make applet fixed width

[Radek Hušek]

Pitel created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
Pitel requested review of this revision.

REVISION SUMMARY
@hein refused D10944 <https://phabricator.kde.org/D10944> with wish to move the config option to contaiment level.
This patch does it: It adds a `IntList` config option `fixedSizeOverride`
to panel containment, checkbox in config overlay to modify it, and
sets `fillWidth` and `fillHeight` of an applet to `false` if it is
in `fixedSizeOverride` array.

Note that the checkbox introduced by this patch still needs some polishing.

REPOSITORY
R119 Plasma Desktop

BRANCH
fixedSizeOverride

REVISION DETAIL
https://phabricator.kde.org/D11410

AFFECTED FILES
containments/panel/contents/config/main.xml
containments/panel/contents/ui/ConfigOverlay.qml
containments/panel/contents/ui/main.qml

To: Pitel
Cc: mart, hein, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Radek Hušek]

Pitel added a comment.


Also do we want this or two options instead:

- no expanding: `maximumWidth = preferredWidth`, and
- no shrinking: `minimumWidth = preferredWidth`?

Because when attempting to use `fillWidth = false` with full version of task manager it will probably overflow the panel.

REPOSITORY
R119 Plasma Desktop

REVISION DETAIL
https://phabricator.kde.org/D11410

To: Pitel
Cc: mart, hein, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Marco Martin]

mart added a comment.


it's moving on the right track.. i would like the checkbox to appear like the one in the panel spacer, in the context menu could be tricky as would need to inject into the applet's contextualactons, so let's forget about it for now.

one thing that could be done is to put that button in the applet handle that appears on hover when the panel controller is open, with the same icon of the add panel spacer in a checkable toolbutton (just icon, tooltip on hover) the panel spacer should use the same config ui.

i would like to expose this ui only for applets that want to expand, and not for those that are already compact (i don't think this option would make any sense in kicoff for instance)

now, on the configuration side.. it's interesting the approach you took with a list of ids in the panel config, i tought more on something directly down in libplsma's applet, but this stays more confined to the panel, so i like it.

INLINE COMMENTS

main.xml:13
+ <entry name="fixedSizeOverride" type="IntList">
+ <label>list of items for which we set fillWidth = false</label>
+ </entry>

list of applet ids that should not have an expanding size policy to save space

REPOSITORY
R119 Plasma Desktop

REVISION DETAIL
https://phabricator.kde.org/D11410

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Radek Hušek]

Pitel updated this revision to Diff 29940.
Pitel added a comment.

Post by Marco Martin
it's moving on the right track.. i would like the checkbox to appear like the one in the panel spacer, in the context menu could be tricky as would need to inject into the applet's contextualactons, so let's forget about it for now.

I'm not sure that is good idea: There are already some items injected in context menus (e.g. Unlock widgets action) and looking at my panel some applets' context menus are not injected. I believe it is because they have multiple menus bound to specific parts of itself and only applet's global menu is (and can) be injected. Most notable examples are systray except the expansion arrow and task icon manager except empty space (if it has set `fixedWidth` I was unable to find a place in it which would show me context menu containing Unlock widget action).

Post by Marco Martin
one thing that could be done is to put that button in the applet handle that appears on hover when the panel controller is open, with the same icon of the add panel spacer in a checkable toolbutton (just icon, tooltip on hover) the panel spacer should use the same config ui.

The combox is currently in the popup showing on hover when panel controller is open. I can make it replace spacer's specific option. I can just remove spacer's option from its context menu and add some temporary code to convert spacer's option into the new one. (And a few version later we can drop this code.) Is it what you meant or did I misunderstand you?

Post by Marco Martin
i would like to expose this ui only for applets that want to expand, and not for those that are already compact (i don't think this option would make any sense in kicoff for instance)

Done.

Changes:

- Fix config item label
- Show checkbox only for expanding applets
- A bit polish checkbox placement

REPOSITORY
R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D11410?vs=29745&id=29940

BRANCH
fixedSizeOverride

REVISION DETAIL
https://phabricator.kde.org/D11410

AFFECTED FILES
containments/panel/contents/config/main.xml
containments/panel/contents/ui/ConfigOverlay.qml
containments/panel/contents/ui/main.qml

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Marco Martin]

mart added a comment.

Post by Radek Hušek

Post by Marco Martin
it's moving on the right track.. i would like the checkbox to appear like the one in the panel spacer, in the context menu could be tricky as would need to inject into the applet's contextualactons, so let's forget about it for now.

I'm not sure that is good idea: There are already some items injected in context menus (e.g. Unlock widgets action) and looking at my panel some applets' context menus are not injected. I believe it is because they have multiple menus bound to specific parts of itself and only applet's global menu is (and can) be injected. Most notable examples are systray except the expansion arrow and task icon manager except empty space (if it has set `fixedWidth` I was unable to find a place in it which would show me context menu containing Unlock widget action).

I'm fine with dropping the context menu action, would be nice for the spacer to share the same mechanism and ui, definitely!

Post by Radek Hušek

Post by Marco Martin
one thing that could be done is to put that button in the applet handle that appears on hover when the panel controller is open, with the same icon of the add panel spacer in a checkable toolbutton (just icon, tooltip on hover) the panel spacer should use the same config ui.

The combox is currently in the popup showing on hover when panel controller is open. I can make it replace spacer's specific option. I can just remove spacer's option from its context menu and add some temporary code to convert spacer's option into the new one. (And a few version later we can drop this code.) Is it what you meant or did I misunderstand you?

i would like it as a checkable toolbutton with no label (but a tooltip) in handleRow, between configureButton and the label

REPOSITORY
R119 Plasma Desktop

REVISION DETAIL
https://phabricator.kde.org/D11410

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Radek Hušek]

Pitel updated this revision to Diff 30555.
Pitel added a comment.


- Replace checkbox with a checkable button
- Add support for panel spacer -- currently we just check whether the applet is panel spacer and if it is we use `applet.configuration.expanding` instead of our `fixedSizeOverride`. I think we should remove context action to set `expanding` from panel spacer after this is landed and then convert it to use `fixedSizeOverride` array.

REPOSITORY
R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D11410?vs=29940&id=30555

BRANCH
fixedSizeOverride

REVISION DETAIL
https://phabricator.kde.org/D11410

AFFECTED FILES
containments/panel/contents/config/main.xml
containments/panel/contents/ui/ConfigOverlay.qml
containments/panel/contents/ui/main.qml

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Radek Hušek]

Pitel updated this revision to Diff 30728.
Pitel added a comment.


- Remove QtQuick.Controls import (was needed only by Checkbox)

REPOSITORY
R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D11410?vs=30555&id=30728

BRANCH
fixedSizeOverride

REVISION DETAIL
https://phabricator.kde.org/D11410

AFFECTED FILES
containments/panel/contents/config/main.xml
containments/panel/contents/ui/ConfigOverlay.qml
containments/panel/contents/ui/main.qml

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Marco Martin]

mart added a comment.


almost there :)
I have a couple of points, then it can go :)

INLINE COMMENTS

ConfigOverlay.qml:385
+ iconSource: "distribute-horizontal-x"
+ visible: currentApplet.expandingApplet
+ tooltip: i18n("Make applet non-expanding size")

this will probably need as well
currentApplet.applet.pluginName == "org.kde.plasma.panelspacer"

or it gets lost when the spacer is configured as not expanding and i guess the spacer is special and we want this option to be always there
(step 2 would be removing the option in the spacer and make it use this system, but it's for a next commit)

ConfigOverlay.qml:402
+ }
+ var tmp = plasmoid.configuration.fixedSizeOverride
+ var index = tmp.indexOf(currentApplet.applet.id)

is it necessary to use a temp copy?

REPOSITORY
R119 Plasma Desktop

REVISION DETAIL
https://phabricator.kde.org/D11410

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Radek Hušek]

Pitel added inline comments.

INLINE COMMENTS

mart wrote in ConfigOverlay.qml:385
this will probably need as well
currentApplet.applet.pluginName == "org.kde.plasma.panelspacer"
or it gets lost when the spacer is configured as not expanding and i guess the spacer is special and we want this option to be always there
(step 2 would be removing the option in the spacer and make it use this system, but it's for a next commit)

It does not, it is already in definition of `expandingApplet` (line 254 of `main.qml`).

mart wrote in ConfigOverlay.qml:402
is it necessary to use a temp copy?

My reasons for temp copy were:

- `push` method has no effect on `plasmoid.configuration.fixedSizeOverride` (I assume this has something to do with the whole configuration magic behind but it might be worth further investigating.)
- QML does not notify the change of property if the property is an array and you change only its elements (because the property still contains the same object) so we need to trigger this signal somehow.

REPOSITORY
R119 Plasma Desktop

REVISION DETAIL
https://phabricator.kde.org/D11410

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Radek Hušek]

Pitel updated this revision to Diff 31036.
Pitel added a comment.


Fix tooltip text.

REPOSITORY
R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D11410?vs=30728&id=31036

BRANCH
fixedSizeOverride

REVISION DETAIL
https://phabricator.kde.org/D11410

AFFECTED FILES
containments/panel/contents/config/main.xml
containments/panel/contents/ui/ConfigOverlay.qml
containments/panel/contents/ui/main.qml

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Marco Martin]

mart added a comment.


we're here, mergein few days when 5.13 freezes and 5.14 branch opens

REPOSITORY
R119 Plasma Desktop

REVISION DETAIL
https://phabricator.kde.org/D11410

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Marco Martin]

mart added a comment.


any updates on this?

REPOSITORY
R119 Plasma Desktop

REVISION DETAIL
https://phabricator.kde.org/D11410

To: Pitel, #plasma
Cc: mart, hein, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol