Re: Review Request 128915: resize the view just after setting the containment

[Marco Martin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/
---

(Updated Sept. 15, 2016, 7:05 a.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma.


Changes
---

Submitted with commit 8f10b73cc1054cdf8faddc01c304c7604c25a162 by Marco Martin 
to branch master.


Repository: plasma-framework


Description
---

when the view is in SizeRootObjectToView mode, the root object is resized in 
the event handler, that is too late at startup.
resize the root object right after having announced the new containment, so the 
view subclass can have the view resized to the proper size beforehand, removing 
an useless containment graphicsobject resize.


Diffs
-

  src/plasmaquick/containmentview.cpp 8517099 

Diff: https://git.reviewboard.kde.org/r/128915/diff/


Testing
---


Thanks,

Marco Martin

Re: Review Request 128915: resize the view just after setting the containment

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99202
---


Ship it!




Ship It!

- David Edmundson


On Sept. 15, 2016, 10:57 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 10:57 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

Re: Review Request 128915: resize the view just after setting the containment

[Marco Martin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/
---

(Updated Sept. 15, 2016, 10:57 a.m.)


Review request for Plasma.


Repository: plasma-framework


Description
---

when the view is in SizeRootObjectToView mode, the root object is resized in 
the event handler, that is too late at startup.
resize the root object right after having announced the new containment, so the 
view subclass can have the view resized to the proper size beforehand, removing 
an useless containment graphicsobject resize.


Diffs (updated)
-

  src/plasmaquick/containmentview.cpp 8517099 

Diff: https://git.reviewboard.kde.org/r/128915/diff/


Testing
---


Thanks,

Marco Martin

Re: Review Request 128915: resize the view just after setting the containment

[Marco Martin]

> On Sept. 15, 2016, 10:01 a.m., Aleix Pol Gonzalez wrote:
> > src/plasmaquick/containmentview.cpp, line 101
> > 
> >
> > Wouldn't it be better to pass the size with the incubation properties?
> 
> Marco Martin wrote:
> hmm, you can't access it for the item that you load with setSource i 
> think?
> 
> Aleix Pol Gonzalez wrote:
> Yes, I looked into it and it would be possible but only to whoever 
> constructs the object, as it's being incubated within the Containment.
> 
> It might be worth looking into though, as the values set there will be 
> initialized before the properties are bound. Possibly it doesn't even make 
> sense to have the item created before it has a view altogether.

that is internal behavior of qquickview tough, not really relevant there, 
unless you want to try to patch qquickview itself, which may make sense


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99192
---


On Sept. 15, 2016, 9:20 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 9:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

Re: Review Request 128915: resize the view just after setting the containment

[Marco Martin]

> On Sept. 15, 2016, 10:34 a.m., David Edmundson wrote:
> > src/plasmaquick/containmentview.cpp, line 137
> > 
> >
> > does it render this redundant?
> 
> Aleix Pol Gonzalez wrote:
> Otherwise it could make sense to statically have an `anchors.fill: 
> parent`.

I don't think so, as this is resizing the graphics object that is the instance 
of appletinterface, that is a different item than rootObject()


> On Sept. 15, 2016, 10:34 a.m., David Edmundson wrote:
> > src/plasmaquick/containmentview.cpp, line 140
> > 
> >
> > this if isn't very useful now.
> > If it was null, we'd have crashed already.
> 
> Aleix Pol Gonzalez wrote:
> Good catch. In fact this should also get outside the initial if 
> altogether. If graphicObject is nullptr it still should be set.

hmm, i would rather guard the access to rootobject above


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99195
---


On Sept. 15, 2016, 9:20 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 9:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

Re: Review Request 128915: resize the view just after setting the containment

[Aleix Pol Gonzalez]

> On Sept. 15, 2016, 12:34 p.m., David Edmundson wrote:
> > src/plasmaquick/containmentview.cpp, line 137
> > 
> >
> > does it render this redundant?

Otherwise it could make sense to statically have an `anchors.fill: parent`.


> On Sept. 15, 2016, 12:34 p.m., David Edmundson wrote:
> > src/plasmaquick/containmentview.cpp, line 140
> > 
> >
> > this if isn't very useful now.
> > If it was null, we'd have crashed already.

Good catch. In fact this should also get outside the initial if altogether. If 
graphicObject is nullptr it still should be set.


- Aleix


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99195
---


On Sept. 15, 2016, 11:20 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 11:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

Re: Review Request 128915: resize the view just after setting the containment

[Aleix Pol Gonzalez]

> On Sept. 15, 2016, 12:01 p.m., Aleix Pol Gonzalez wrote:
> > src/plasmaquick/containmentview.cpp, line 101
> > 
> >
> > Wouldn't it be better to pass the size with the incubation properties?
> 
> Marco Martin wrote:
> hmm, you can't access it for the item that you load with setSource i 
> think?

Yes, I looked into it and it would be possible but only to whoever constructs 
the object, as it's being incubated within the Containment.

It might be worth looking into though, as the values set there will be 
initialized before the properties are bound. Possibly it doesn't even make 
sense to have the item created before it has a view altogether.


- Aleix


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99192
---


On Sept. 15, 2016, 11:20 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 11:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

Re: Review Request 128915: resize the view just after setting the containment

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99195
---


Fix it, then Ship it!





src/plasmaquick/containmentview.cpp (line 137)


does it render this redundant?



src/plasmaquick/containmentview.cpp (line 140)


this if isn't very useful now.
If it was null, we'd have crashed already.


- David Edmundson


On Sept. 15, 2016, 9:20 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 9:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

Re: Review Request 128915: resize the view just after setting the containment

[Marco Martin]

> On Sept. 15, 2016, 10:01 a.m., Aleix Pol Gonzalez wrote:
> > src/plasmaquick/containmentview.cpp, line 101
> > 
> >
> > Wouldn't it be better to pass the size with the incubation properties?

hmm, you can't access it for the item that you load with setSource i think?


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99192
---


On Sept. 15, 2016, 9:20 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 9:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

Re: Review Request 128915: resize the view just after setting the containment

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128915/#review99192
---




src/plasmaquick/containmentview.cpp (line 101)


Wouldn't it be better to pass the size with the incubation properties?


- Aleix Pol Gonzalez


On Sept. 15, 2016, 11:20 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128915/
> ---
> 
> (Updated Sept. 15, 2016, 11:20 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> when the view is in SizeRootObjectToView mode, the root object is resized in 
> the event handler, that is too late at startup.
> resize the root object right after having announced the new containment, so 
> the view subclass can have the view resized to the proper size beforehand, 
> removing an useless containment graphicsobject resize.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/containmentview.cpp 8517099 
> 
> Diff: https://git.reviewboard.kde.org/r/128915/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Marco Martin
> 
>