> 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
