> On April 17, 2012, 4:17 a.m., C. Boemann wrote: > > I like it very much, and I found only a few issues. Before we merge this > > however, I think it should be made public in a branch sopeople can test > > build it beforehand. After all there may be parts of the code that you > > don't build and thus hasn't found.
Puh, good that you like it, after all now 2-days-of-work :) Just pushed this into a branch named "tuneKoUnitAPI", this time also with the unit tests for the part of the KoUnit API I worked on, which were missing here. And of course also with the issues fixed as mentioned in my replies to your review comments. > On April 17, 2012, 4:17 a.m., C. Boemann wrote: > > libs/kopageapp/dialogs/KoPAConfigureDialog.cpp, line 50 > > <http://git.reviewboard.kde.org/r/104626/diff/1/?file=56917#file56917line50> > > > > uhm is this correct? Yes, correct. That signal and that slot now take KoUnit as parameter. And just to make sure (but I guess you know that and this is not what you question here :) ) the normalized signal/slot signatures result in faster wiring up of the connection. > On April 17, 2012, 4:17 a.m., C. Boemann wrote: > > libs/odf/KoUnit.h, line 80 > > <http://git.reviewboard.kde.org/r/104626/diff/1/?file=56935#file56935line80> > > > > Maybe make this into QFlags, if we want as you Say to have imperial vs > > metric too; > > btw for the ui list it may be an idea to have a SortImperialFirst which > > we can set depending of locale (just an idea) Guess you are right, I should be consequent here and right-straight make that a QFlags already now. Will add next. SortImperialFirst is a nice idea, indeed. Will file as wish item, so the idea is not lost. > On April 17, 2012, 4:17 a.m., C. Boemann wrote: > > libs/widgets/KoUnitDoubleSpinBox.cpp, line 119 > > <http://git.reviewboard.kde.org/r/104626/diff/1/?file=56938#file56938line119> > > > > in the future when editing a line with wrong hacking style please take > > the oppotunity to fix that too Yup, usually do so. Seems I missed a few. Went though the whole patch again and hopefully now did that for all lines touched. > On April 17, 2012, 4:17 a.m., C. Boemann wrote: > > sheets/part/dialogs/PreferenceDialog.cpp, line 449 > > <http://git.reviewboard.kde.org/r/104626/diff/1/?file=56946#file56946line449> > > > > Hmmm do we really want pixel listed in Sheets? No idea, it's just the status quo, so I did not change anything here. > On April 17, 2012, 4:17 a.m., C. Boemann wrote: > > libs/koreport/wrtembed/reportscene.cpp, line 155 > > <http://git.reviewboard.kde.org/r/104626/diff/1/?file=56924#file56924line155> > > > > uhm no, at least it doesn't look like it. i mean it has just be set one > > line above So I will leave this TODO for now. Any idea who knows (or should know) more about this code these days? - Friedrich W. H. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104626/#review12550 ----------------------------------------------------------- On April 16, 2012, 9:27 p.m., Friedrich W. H. Kossebau wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104626/ > ----------------------------------------------------------- > > (Updated April 16, 2012, 9:27 p.m.) > > > Review request for Calligra and C. Boemann. > > > Description > ------- > > Sigh... so the proposal from https://git.reviewboard.kde.org/r/104607/ from > boemann > "I'd say fix the code in KoUnit so it reports the unit in the order you > like, and use KoUnit everywhere to define the order" > turned into quite some hacking and in the end I found myself redoing the > KoUnit API partially (because the old confused me too often). > > So let's have some feedback on the current state, to see what is welcome and > what is not, and what else could/should be done :) > > Changed: > * rename KoUnit::unit(...) to KoUnit::fromSymbol(...) <- more Qt'ish > * turn static KoUnit::unitName(KoUnit) into member method KoUnit::symbol() > <- as typical use-case is on existing KoUnit instance, also shorter and more > OOed > * rename KoUnit::Unit to KoUnit::Type <- "type" feels a better term here > * added KoUnit::type() and KoUnit::setFactor(...) <- useful in a few places > * remove KoUnit::unitDescription(...) from API <- not used outside > * rename KoUnit::PixelVisibility to KoUnit::ListFilter <- more general, > some might want to add other flags like HideNoneMetrics > > Fixes: > * ensure the same order of unit types in all unit type selectors in the UI > * update the page layout dialog on a change of the document's unit property > * update the changeUnitActions on a change of the document's unit property > > > Diffs > ----- > > filters/karbon/image/ImageExportOptionsWidget.cpp 2b5d541 > karbon/ui/KarbonPart.cpp 5e6a958 > krita/plugins/extensions/imagesize/dlg_imagesize.cc 96490c9 > krita/plugins/tools/defaulttools/kis_tool_measure.cc 5e9afa3 > krita/ui/widgets/kis_custom_image_widget.cc 5b00fb9 > libs/kopageapp/KoPADocument.cpp 240171a > libs/kopageapp/dialogs/KoPAConfigureDialog.cpp 8912db3 > libs/koproperty/editors/spinbox.cpp d923c6e > libs/koreport/common/KoReportItemBase.cpp 7f6a575 > libs/koreport/common/krsectiondata.cpp 38c14c8 > libs/koreport/wrtembed/KoReportDesigner.cpp 68a61f9 > libs/koreport/wrtembed/KoReportDesignerItemBase.cpp f3ff8dc > libs/koreport/wrtembed/KoReportDesignerItemLine.cpp ce54e7c > libs/koreport/wrtembed/reportscene.cpp dd32f7a > libs/koreport/wrtembed/reportsection.cpp 52447cf > libs/main/KoDocument.h 66bf3ac > libs/main/KoDocument.cpp 831ed9d > libs/main/KoRuler.cpp 4272b9b > libs/main/KoView.cpp 81dafd3 > libs/main/KoView_p.h 9b3dff2 > libs/main/config/KoConfigGridPage.h 01373e9 > libs/main/config/KoConfigGridPage.cpp a3e1d6f > libs/main/config/KoConfigMiscPage.h 2c36996 > libs/main/config/KoConfigMiscPage.cpp 7f54ef1 > libs/odf/KoUnit.h 1f035fe > libs/odf/KoUnit.cpp 43cc908 > libs/widgets/KoPageLayoutWidget.cpp c9f0fc0 > libs/widgets/KoUnitDoubleSpinBox.cpp f9f00da > plugins/paragraphtool/Ruler.cpp 3053696 > plugins/textshape/dialogs/ParagraphBulletsNumbers.cpp 87c1b85 > sheets/DocBase.cpp a9812c6 > sheets/dialogs/LayoutDialog.cpp d1090f4 > sheets/part/Doc.cpp 4c40b87 > sheets/part/HeaderItems.cpp 5fc3cfa > sheets/part/HeaderWidgets.cpp 3fac4cc > sheets/part/dialogs/PreferenceDialog.cpp fae954a > words/part/KWApplicationConfig.cpp b5fd980 > words/part/KWOdfLoader.cpp 238c7fe > words/part/dialogs/KWPageSettingsDialog.h 0a028a9 > words/part/dialogs/KWPageSettingsDialog.cpp f577353 > > Diff: http://git.reviewboard.kde.org/r/104626/diff/ > > > Testing > ------- > > Played with (hopefully) all touched widgets, seems to work. > > > Thanks, > > Friedrich W. H. Kossebau > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
