Review Request: tune KoUnit API (and have pagelayout dialog and Words' horizontal ruler context menu update to changes of the document's unit type)

Tue Apr 17 16:28:45 BST 2012

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20120417/f84697c2/attachment.htm>