> On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/allinteractionitem.h, line 47 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32532#file32532line47> > > > > You can't use i18n in a header file, nor in a static object. Instead, > > fill the list on demand when needed for the first time (if empty then > > append...). > > It's either that or I18N_NOOP, but in this case I think on demand is > > simpler. > > > > No static objects in libraries, too, so this should be a > > function-static (e.g. make a file-static function that has the > > function-static object, fills it on demand, and returns it). > > Cyril Oblikov wrote: > Hmm... what is file-static and function-static? > > David Faure wrote: > File-static is a C-style function in a .cpp file. Marked "static" so that > it's only visible inside the .cpp file. > > A function-static object is "static Foo bar;" inside of a > function/method. "Safe" because it's only created when entering the > function/method for the first time. > > In clear: > > static QStringList allInteractionItemHeaders() { > static QStringList s_headers; > if (s_headers.isEmpty()) { > s_headers << i18n("...") << ...; > } > return s_headers; > } > > In a CPP file, not in a header file :)
done > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/jobuidelegate.h, line 107 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32549#file32549line107> > > > > This is a BIC change (new virtual method in a public class). However, > > this patch is for kdelibs-frameworks (future 5.0), so in fact it's the > > right timing for making such a change. So, no objection, I just wanted to > > point this out for clarity :) > > Cyril Oblikov wrote: > I'm not sure this method should be virtual. > > David Faure wrote: > Actually it could be useful. E.g. for unittests, which could test that > the interactiondialog is called, without actually showing a dialog. > > If you had more time for your SOC I would strongly recommend to add unit > tests as the next step, BTW... > > But did you at least run jobtest (in kdelibs/kio/tests) to make sure that > the new code doesn't break it? E.g. the creation of a dialog for sure broke > it, but now you've fixed that bit. No, I didn't use tests. Are there any docs about kdelibs testing? - Cyril ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102388/#review5920 ----------------------------------------------------------- On Aug. 22, 2011, 11:12 p.m., Cyril Oblikov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102388/ > ----------------------------------------------------------- > > (Updated Aug. 22, 2011, 11:12 p.m.) > > > Review request for kdelibs and David Faure. > > > Summary > ------- > > Modeless dialog to handle interactions and modifications in CopyJob. > > > Diffs > ----- > > kio/CMakeLists.txt b517621 > kio/kio/copyjob.h eb88c7a > kio/kio/copyjob.cpp eff7825 > kio/kio/interactiondialog/abstractinteractionitem_p.h PRE-CREATION > kio/kio/interactiondialog/abstractinteractionmodel.cpp PRE-CREATION > kio/kio/interactiondialog/abstractinteractionmodel_p.h PRE-CREATION > kio/kio/interactiondialog/allinteractionitem.cpp PRE-CREATION > kio/kio/interactiondialog/allinteractionitem_p.h PRE-CREATION > kio/kio/interactiondialog/allinteractionmodel.cpp PRE-CREATION > kio/kio/interactiondialog/allinteractionmodel_p.h PRE-CREATION > kio/kio/interactiondialog/existinginteractionitem.cpp PRE-CREATION > kio/kio/interactiondialog/existinginteractionitem_p.h PRE-CREATION > kio/kio/interactiondialog/existinginteractionmodel.cpp PRE-CREATION > kio/kio/interactiondialog/existinginteractionmodel_p.h PRE-CREATION > kio/kio/interactiondialog/interactiondialog.h PRE-CREATION > kio/kio/interactiondialog/interactiondialog.cpp PRE-CREATION > kio/kio/interactiondialog/interactiondialogtab.cpp PRE-CREATION > kio/kio/interactiondialog/interactiondialogtab_p.h PRE-CREATION > kio/kio/interactiondialog/renameinteractionwidget.h PRE-CREATION > kio/kio/interactiondialog/renameinteractionwidget.cpp PRE-CREATION > kio/kio/interactiondialog/requestitemmodel.cpp PRE-CREATION > kio/kio/interactiondialog/requestitemmodel_p.h PRE-CREATION > kio/kio/interactiondialog/unreadableinteractionitem.cpp PRE-CREATION > kio/kio/interactiondialog/unreadableinteractionitem_p.h PRE-CREATION > kio/kio/interactiondialog/unreadableinteractionmodel.cpp PRE-CREATION > kio/kio/interactiondialog/unreadableinteractionmodel_p.h PRE-CREATION > kio/kio/jobuidelegate.h 25e0728 > kio/kio/jobuidelegate.cpp 85679c2 > > Diff: http://git.reviewboard.kde.org/r/102388/diff > > > Testing > ------- > > > Thanks, > > Cyril > >
