Review Request 120235: restore ResizeOrigin

[Vishesh Handa]

> On Sept. 17, 2014, 2:54 p.m., Vishesh Handa wrote:
> > src/plasmaquick/dialog.cpp, line 301
> > <https://git.reviewboard.kde.org/r/120235/diff/2/?file=312805#file312805line301>
> >
> >     Would this actually do anything?
> >     
> >     resizeOrigin = MainItem;
> >     // do stuff which changes the size and sends a resizeEvent
> >     resizeOrigin = Undefined;
> >     
> >     Then the resizeEvent would be called with the resizeOrigin as "Undefined". Hmm, I belive there was going to be an assert.
> 
> Marco Martin wrote:
>     when the width of mainItem is actually changed in this function, causes slotMainItemSizeChanged to be called

I'm confused. I thought we were disconnecting that connect statement in the next line.


> On Sept. 17, 2014, 2:54 p.m., Vishesh Handa wrote:
> > src/plasmaquick/dialog.cpp, line 470
> > <https://git.reviewboard.kde.org/r/120235/diff/2/?file=312805#file312805line470>
> >
> >     Why? It's being done at the end of the function. What was breaking?
> >     
> >     Also, considering that updateTheme internally uses the frameSvgItem's size, we want to call it after the size has been updated.
> >     
> >     Unless I'm missing something?
> 
> Marco Martin wrote:
>     before the first call of updateTheme() there is no svg loaded, so margins will always be 0

So maybe we can load the SVG in componentCompleted? Adding an extra updateTheme seems mysterious, and I can imagine someone in the future coming along and removing it because it isn't obvious why it should be there.


> On Sept. 17, 2014, 2:54 p.m., Vishesh Handa wrote:
> > src/plasmaquick/dialog.cpp, line 1069
> > <https://git.reviewboard.kde.org/r/120235/diff/2/?file=312805#file312805line1069>
> >
> >     Oh. I didn't realize we had cases when the mainItem could be 0.
> >     
> >     Could you please add an assert in syncToMainItemSize for checking the mainItem.
> 
> Marco Martin wrote:
>     yes, if the qml part doesn't assign it, there will be none,
>     syncToMainItem already checks if it does exists

Yes, but syncToMainItem should never be called if the mainItem is not set so it makes sense to manually assert then.


> On Sept. 17, 2014, 2:54 p.m., Vishesh Handa wrote:
> > src/plasmaquick/dialog.cpp, line 946
> > <https://git.reviewboard.kde.org/r/120235/diff/2/?file=312805#file312805line946>
> >
> >     Why set this variable at all? It's being changed again at the end of the function and not being used anywhere else.
> 
> Marco Martin wrote:
>     slotMainItemSizeChanged won't be executed in this case

Hmm. Considering that you're not disconnecting or reconnecting, it will be called. Maybe you want to disconnect?


- Vishesh


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


On Sept. 17, 2014, 4:03 p.m., Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120235/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 4:03 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> A dialog can be resize for two reasons: the mainItem size changes, or the dialog size changes.
> 
> the first can happen programmatically, caused by the Layout, or just by assigning the width.
> 
> the second can be caused either programmatically, assigning the size of the dialog or externally by the windowmanager, that is the only one theat in the end has the only final control of the window size
> 
> 
> Diffs
> -----
> 
>   src/plasmaquick/dialog.cpp 79a871b 
>   tests/dialog_minWidthHeightRepositioning.qml 37bd622 
> 
> Diff: https://git.reviewboard.kde.org/r/120235/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140917/642dc436/attachment.html>