Patch for plasmoid subsystem(containments/desktop) in plasma-desktop
ClosedPublic

Authored by konstantinshtepa on Jan 19 2017, 6:07 PM.

Details

Summary

Under T5144

Set of pathes to fix plasmoid background mechanics.
Fixed:

  • Bug 375307 (Sometimes Plasmoid doesn't free all his space in LayoutManager),
  • Bug 375308 (Plasmoid applet doesn't have maximum size handlers).
  • Bug 375349 (Plasmoid can't be resized to declared minimumWidth).

A detailed explanation of bugs, fixes and mechanics changes

Before anything else I'd like to explain why I need this patch. At winter holidays I had a free time and decided to rewrite plasma-simplemonitor(maded by dhabyx) - port it to plasma5 and improve it. As one of new features I added "skins" - posibility to change UI looks. Let's describe one skin as "row" and other as "column". Skins had different default sizes and different minimum sizes. As behaviour it sugges that when user change one skin to another plasmoid size supposed to change to default size of new skin. In plasma5 there is no resize function in plasmoid anymore. Instead docs pointed to use Layout properties. And so I tried to set maximum sizes in Layout. It doesn't work. When researching this problem it was occurred that there is no such functionality in plasmoids despite that plasmoid already uses Layout minimum sizes. And so I made this patch to fix that. When debugging this patch I found other bugs and fixed them alongside.

To read next you should understand QML and Qt/C++. Especially what is QPropertyAnimation, how it works and behave over object properties.
I would mention bugs in order of it's difficulties and their involvement in patch. From easy to hard.

  1. Bug 375349 (Plasmoid can't be resized to declared minimumWidth).

This bug was because of mess in size relationship. Because of it plasmoid wouldn't get smaller than Layout.minimumWidth + 2 * handleWidth. Bug was in appletItem minimumWidth calculation and in AppletHandler.qml resize calculation(resizeHandle.onPositionChanged) .

  1. Bug 375307 (Sometimes Plasmoid doesn't free all his space in LayoutManager),

Bug happens because of appletItem size animations. Let's see how bug happening. User is done resizing appletItem, after that appletItem would be resized and positioned in layoutManager to fit cell politics, result sizes and position would be locked in layoutManager as area which used by this plasmoid. Now will start animation over applet height, width, x and y. Guess what would happend if somebody tried to move plasmoid or resize it in exact that moment? Yup. appletItem would try to release area in layoutManager with current(still animated) properties which != end properies. And it would lead to some area in layoutManager to not release properly.
I did many things(for example tried to implement and use "end" properties) to patch this but I think that it should be fully patched with implemtation of appletItem.floating property and functions positionItem and releasePosition which now handle all operations over layoutManager to lock\unlock area.

  1. Bug 375308 (Plasmoid applet doesn't have maximum size handlers)

When implementing this patch I encountered several questions:

  • What to do in case of minimum > maximum size?

I think that when this happened the other restrictive size should be changed to match new one. So I added check to implemet this behaviour to handlers of restrictive sizes.

  • What to do with qreal -> int convertation for maxinum size?

Default maxsize is Number.POSITIVE_INFINITY and that it would be autoconverted into int.NEGATIVE_MAX_VALUE which is not good. So I added simple check to see if qreal value would be too much to handle in int. And if it so to change it to 1000000. I don't have any better idea how to handle this.

  • What to do when appletItem size set by layoutManager would be > than maximumSize?

I created "innerAppletItem" in face of mouseListener. To handle all of animations I created additional properties like endHeight, endX, etc. "End" - because they symbolise what height, x, and other would be at end of animation. With these additional properties current end values always known and using them "innerAppletItem" can handle all kind of size and position changes even when it animations are running.

  • What to do when plasmoid is moved to new place and animations is on?

Because of this I just can not write in "innerAppletItem"(mouseListener) "anchors.centerIn: parent". So I implemented x and y change handlers in appletItem which would move position of "innerAppletItem" to place of old center(where it was according to appletItem.parent coordinates) and then play animation of move to a new place.

Test Plan

Tested with various plasmoids on single monitor. Worked well.

Diff Detail

Repository
R119 Plasma Desktop
Branch
plasmoid_size_restraints
Lint
No Linters Available
Unit
No Unit Test Coverage
konstantinshtepa retitled this revision from to Added max size restrictions handlers for plasmoids.
konstantinshtepa updated this object.
konstantinshtepa edited the test plan for this revision. (Show Details)
Restricted Application added a project: Plasma. · View Herald TranscriptJan 19 2017, 6:07 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
konstantinshtepa retitled this revision from Added max size restrictions handlers for plasmoids to Patch for plasmoid subsystem(containment/desktop) in plasma-desktop.Jan 19 2017, 6:10 PM
konstantinshtepa updated this object.
konstantinshtepa changed the visibility from "Public (No Login Required)" to "No One".
konstantinshtepa changed the edit policy from "All Users" to "konstantinshtepa (Konstantin Shtepa)".
konstantinshtepa changed the visibility from "No One" to "konstantinshtepa (Konstantin Shtepa)".

It was not supposed to go there, but. sigh... arc...
I will try to make it better.

konstantinshtepa retitled this revision from Patch for plasmoid subsystem(containment/desktop) in plasma-desktop to Patch for plasmoid subsystem(containments/desktop) in plasma-desktop.Jan 19 2017, 6:32 PM
konstantinshtepa updated this object.
konstantinshtepa added a reviewer: Plasma.
konstantinshtepa changed the visibility from "konstantinshtepa (Konstantin Shtepa)" to "Public (No Login Required)".
konstantinshtepa changed the edit policy from "konstantinshtepa (Konstantin Shtepa)" to "All Users".

That's quite a huge patch.

Can you give a much bigger explanation on exactly what it's doing and why.

"Fixes bug N" doesn't really explain what the original problem was.

containments/desktop/package/contents/ui/AppletAppearance.qml
70–82

why do you do

property int foo
Binding on foo {
value: Math.max()
}

instead of just

property int foo: Math.max

?

214

Point is a native QML type.
http://doc.qt.io/qt-5/qml-point.html

property point lastPoint

then use that in your logic above.
It can be better because you then get one signal when it changes not two.

437

if maximum is infinite, (so undefined in QtQuick.Layouts) set it to 1000000

why?
It's far easier to test for isFinite() in other bits of code than a random big init

That's quite a huge patch.
Can you give a much bigger explanation on exactly what it's doing and why.
"Fixes bug N" doesn't really explain what the original problem was.

Sorry, diff not supposed to be send here today and in that kind of state. And I don't see here any way to delete this diff and close this theme. Because of this information is not fully ready.
I would tomorrow add comments to code about what code does and why.

containments/desktop/package/contents/ui/AppletAppearance.qml
70–82

Binding on properties used because these properties assigned in JS(in handlers onMinimumWidthChanged and others) as lvalue and it breaks normal bindings. If someone could bring better idea how to handle situation when minimum size > maximum size without these bindings then they can be removed.

214

You can go without links. I know Qt, commited there.
One signal when it doesn't have any handlers or bindings wouldn't change any weather in QML. Point too have it's negative sides because it needs inverpretation "lastPoint.x". The question here is maybe I should have used int too as a base to properties(to maintain policy of int-based sizes).

437

Just a few lines below this size is assigned for property with int as a base. Then this property is used in operations with others int-based kde sizes. And keep it as double is not a option because of default Qt maximumSize(Number.POSITIVE_INFINITY). So there goes 1000000. Is there better way?

This comment was removed by konstantinshtepa.

Summary: reworked set of patches to plasma-desktop/containments/desktop to fix bugs in plasmoid.
Fix for

  • plasmoid applet missing maximum size handler
  • plasmoid can't be set to declared minimumWidth
  • plasmoid don't release space in layoutManager.properly due to size animations

GUI

BUG: 375349,375308,375307

konstantinshtepa updated this object.
konstantinshtepa edited the test plan for this revision. (Show Details)

Rewrited commit message in according to kde commit polititcs

Turn on background width animation again.

Fix commit message

containments/desktop/package/contents/ui/AppletAppearance.qml
59

why are you including handleWidth here?

From what I can see in the code;
appletItem should be the the size of the applet + normal margins

Then on line 290 it creates a container that's bigger than the parent item and does include the handle

A detailed explanation of bugs, fixes and mechanics changes

Before anything else I'd like to explain why I need this patch. At winter holidays I had a free time and decided to rewrite plasma-simplemonitor(maded by dhabyx) - port it to plasma5 and improve it. As one of new features I added "skins" - posibility to change UI looks. Let's describe one skin as "row" and other as "column". Skins had different default sizes and different minimum sizes. As behaviour it sugges that when user change one skin to another plasmoid size supposed to change to default size of new skin. In plasma5 there is no resize function in plasmoid anymore. Instead docs pointed to use Layout properties. And so I tried to set maximum sizes in Layout. It doesn't work. When researching this problem it was occurred that there is no such functionality in plasmoids despite that plasmoid already uses Layout minimum sizes. And so I made this patch to fix that. When debugging this patch I found other bugs and fixed them alongside.

To read next you should understand QML and Qt/C++. Especially what is QPropertyAnimation, how it works and behave over object properties.
I would mention bugs in order of it's difficulties and their involvement in patch. From easy to hard.

  1. Bug 375349 (Plasmoid can't be resized to declared minimumWidth).

This bug was because of mess in size relationship. Because of it plasmoid wouldn't get smaller than Layout.minimumWidth + 2 * handleWidth. Bug was in appletItem minimumWidth calculation and in AppletHandler.qml resize calculation(resizeHandle.onPositionChanged) .

  1. Bug 375307 (Sometimes Plasmoid doesn't free all his space in LayoutManager),

Bug happens because of appletItem size animations. Let's see how bug happening. User is done resizing appletItem, after that appletItem would be resized and positioned in layoutManager to fit cell politics, result sizes and position would be locked in layoutManager as area which used by this plasmoid. Now will start animation over applet height, width, x and y. Guess what would happend if somebody tried to move plasmoid or resize it in exact that moment? Yup. appletItem would try to release area in layoutManager with current(still animated) properties which != end properies. And it would lead to some area in layoutManager to not release properly.
I did many things(for example tried to implement and use "end" properties) to patch this but I think that it should be fully patched with implemtation of appletItem.floating property and functions positionItem and releasePosition which now handle all operations over layoutManager to lock\unlock area.

  1. Bug 375308 (Plasmoid applet doesn't have maximum size handlers)

When implementing this patch I encountered several questions:

  • What to do in case of minimum > maximum size?

I think that when this happened the other restrictive size should be changed to match new one. So I added this behaviour to all changes handlers of restrictive sizes. It also broke normal qml binding of restrictive sizes, but I solved it with using of QML Binding Items.

  • What to do with qreal -> int convertation for maxinum size?

Default maxsize is Number.POSITIVE_INFINITY and that it would be autoconverted into int.NEGATIVE_MAX_VALUE which is not good. So I added simple check to see if qreal value would be too much to handle in int. And if it so to change it to 1000000. I don't have any better idea how to handle this.

  • What to do when appletItem size set by layoutManager would be > than maximumSize?

I created "innerAppletItem" in face of mouseListener. To handle all of animations I created additional properties like endHeight, endX, etc. "End" - because they symbolise what height, x, and other would be at end of animation. With these additional properties current end values always known and using them "innerAppletItem" can handle all kind of size and position changes even when it animations are running.

  • What to do when plasmoid is moved to new place and animations is on?

Because of this I just can not write in "innerAppletItem"(mouseListener) "anchors.centerIn: parent". So I implemented x and y change handlers in appletItem which would move position of "innerAppletItem" to place of old center(where it was according to appletItem.parent coordinates) and then play animation of move to a new place.

Rewrited some code to better understanding
Fixed background-handle animation in plasmoidBackground. It was a little bug, but I don't see any sense to report it now when patch are ready. Fixed it by changing it to animate only handle-width instead of animation of full plasmoidBackground width.

mart added a subscriber: mart.Jan 25 2017, 11:35 AM

can this be splitted in multiple reviews/commits?

In D4204#80106, @mart wrote:

can this be splitted in multiple reviews/commits?

It can be splitted into multiple commits inside one branch so you can view what exactly fix what. But multiple reviews based on master? I don't think so. All patches have intersections and patch of Bug 375308 heavily depends on patch of Bug 375307.

mart added a comment.Jan 25 2017, 12:37 PM
In D4204#80106, @mart wrote:

can this be splitted in multiple reviews/commits?

It can be splitted into multiple commits inside one branch so you can view what exactly fix what. But multiple reviews based on master? I don't think so. All patches have intersections and patch of Bug 375308 heavily depends on patch of Bug 375307.

what i would like to have logically splitted is the management of the floating property (and having positionItem()/releasePosition used around)

and on the other hand the signal handlers of minimumWidthChanged/widthChanged etc in Appletappearance, that's the biggest part

containments/desktop/package/contents/ui/AppletAppearance.qml
50

mouseListener just has a simple anchors.fill:parent to this, so makes innerWidth/innerHeight redundant as they are the same as the parent?

containments/desktop/package/contents/ui/AppletAppearance.qml
50

It did. It doesn't after this patch.

It's detached so that appletItem can store the final width/height and this container then animates to that position.

davidedmundson added inline comments.Jan 25 2017, 1:01 PM
containments/desktop/package/contents/ui/AppletAppearance.qml
101

this is broken.

if I'm an applet and do:
Plasmoid.Layout.maximumWidth = 50

this appletItem.maximumWidth == 58 (assuming 4px margins)
which is correct

Now if I do:

Plasmoid.Layout.minimumWidth = 60

this appletItem.maximumWidth == 68
which is also fine, we've set it to the minimum + margins

But, now if I do:

Plasmoid.Layout.maximumWidth = 70
this appletItem.maximumWidth == 68

because we've broken the binding.

We're going to need to move the

if (minimumWidth > maximumWidth)
maximumWidth = minimumWidth;

logic somewhere lower.
Either into appletContainer or even AppletQuickItem where it does the proxying.

davidedmundson added inline comments.Jan 25 2017, 1:15 PM
containments/desktop/package/contents/ui/AppletAppearance.qml
445

Edit, maybe it won't - that's why you have the separate Binding.

However changing this to:
minimumWidth: Math.min(minimumSize.width, maximumSize.width);

for all 4

would still be a bit cleaner as then appletItem can trust these values are correct

containments/desktop/package/contents/ui/AppletAppearance.qml
101

That's strange. I didn't expirienced any of this when testing.
Binding on level appletItem.maximumWidth shouldn't be broken because they based on external Binding QML element.
I will look into this.

containments/desktop/package/contents/ui/AppletAppearance.qml
445

Ok. I didn't think about this possibility. Maybe you are right and it would be better. If so then I can remove separate bindings. I will test it.

containments/desktop/package/contents/ui/AppletAppearance.qml
445

It wouldn't work.
For example:
plasmoid.Layout.minimumHeight = 150
plasmoid.Layout.maximumHeight = 200
plasmoid.Layout.maximumHeight = 130
On third step it would think that maximumHeight = 150 and minimumHeight = 130, when it should think that maximumHeight = minimumHeight = 130.

konstantinshtepa added a comment.EditedJan 25 2017, 5:56 PM
In D4204#80142, @mart wrote:

what i would like to have logically splitted is the management of the floating property (and having positionItem()/releasePosition used around)

and on the other hand the signal handlers of minimumWidthChanged/widthChanged etc in Appletappearance, that's the biggest part

Sorry, I don't understand what you would like me to do. Do you like me to split code to add additional commit where would be introduced managment of floating property? But why? it's useless without others bug 375307(fixes of work with layoutManager) fixes. And to split bug 375307 fixes from bug 375308(maximumSize handlers) is big work for me, because all my work centered over bug 375308. Other bugs were founded when debugging bug 375308 fixes. Because of that bug 375307 fixes is never were intended by me as standalone, they currently included only as additional bug fixing to bug 375308 fixes.

P.S. I understand that this code is not well. But in first place it couldn't be well because current state of plasmoid background code is in mess and it need to be rewritten. What I propose in this diff is temporary solution which fixes bugs until I or somebody else would rewrite plasmoid background to normal code.

konstantinshtepa marked an inline comment as done.

Removed Bindings

containments/desktop/package/contents/ui/AppletAppearance.qml
445

Ok, I removed bindings at cost of two additional handlers.

davidedmundson accepted this revision.Jan 26 2017, 3:43 PM
davidedmundson added a reviewer: davidedmundson.

Heh, you're right that's what would happen. Though really any max > min on the client is undefined behaviour, so it's hard to say any is "right".

Anyway, I think there's still room for improvements in this file overall (mostly with the existing structure), but for now I think this is good to go. I've been testing it for a day, seems to work fine. Including with RTL.

This revision is now accepted and ready to land.Jan 26 2017, 3:43 PM
konstantinshtepa edited edge metadata.

Fixed littke bug which would happend with code

plasmoid.Layout.minimumHeight = 150
plasmoid.Layout.maximumHeight = 130
plasmoid.Layout.maximumHeight = 150

Bug led to maximumHeight = 150 when minimumHeight would still be = 130

Though really any max > min on the client is undefined behaviour, so it's hard to say any is "right".

You are right. It's undefined behaviour. At current state there is a hole in mechanis:

plasmoid.Layout.minimumHeight = 150
plasmoid.Layout.maximumHeight = 130
plasmoid.Layout.maximumHeight = 150

because Qt wouldn't emit signal - Layout.minimumHeight is not changed.
But I don't think that it can be fixed, it's just how QML works, and it's a user job to be sure that at his end these values are alright.

Anyway, I think there's still room for improvements in this file overall (mostly with the existing structure), but for now I think this is good to go. I've been testing it for a day, seems to work fine. Including with RTL.

Thanks!

This revision was automatically updated to reflect the committed changes.