> On Oct. 14, 2013, 11 a.m., Kevin Ottens wrote: > > Looks good indeed. > > > > Maybe an idea for an improvement: What about having the internal methods > > use a QScopedPointer on the dialog? It'd avoid the delete before the > > return, and if someone modifies the file later on adding more such returns > > it reduces the risk of missing one such delete. > > > > Since those dialogs are exec'd anyway that should do the trick.
Sounded like a good idea, but it does not work because createKMessageBox() deletes the dialog itself. This causes a double-delete when leaving the internal method. Since one can't pass a QScopedPointer as argument to turn createKMessageBox() into a "sink" method, it cannot work. - Aurélien ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113197/#review41682 ----------------------------------------------------------- On Oct. 11, 2013, 10:56 a.m., Aurélien Gâteau wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/113197/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2013, 10:56 a.m.) > > > Review request for KDE Frameworks. > > > Repository: kdelibs > > > Description > ------- > > Factorize code between regular and *WId functions, reducing the file size by > 12%. > > This patch adds another level of indirection. For example the sorry() > function is changed from: > > void sorry(QWidget *parent, ...args) > { > QDialog *dialog = new QDialog(parent); > /* fill dialog */ > } > > to: > > static void sorryInternal(QDialog *dialog, ...args) > { > /* fill dialog */ > } > > void sorry(QWidget *parent, ...args) > { > sorryInternal(new QDialog(parent), ...args); > } > > This makes it possible to turn the sorryWId() function into a forward > function rather than a slightly-different copy of the original sorry() > function: > > void sorryWId(WId parent_id, ...args) > { > sorryInternal(createWIdDialog(parent_id), ...args); > } > > createWIdDialog() is a helper function to create a dialog which is a child of > a window belonging to another process. > > Note: I kept most of the code to the place where it originally was in the > file. This keeps the diff small, but readability would be improved by > grouping together similar functions. Let me know if you think it is worth > doing so. > > > Diffs > ----- > > tier1/kwidgetsaddons/src/kmessagebox.cpp 0cfa491 > > Diff: http://git.reviewboard.kde.org/r/113197/diff/ > > > Testing > ------- > > Builds, kmessageboxwidtest.cpp runs correctly. No other tests are available > though. > > > Thanks, > > Aurélien Gâteau > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
