Add dtpr to KReportUnit
ClosedPublic

Authored by piggz on Dec 22 2016, 2:54 PM.

Details

Summary

Move all inline functions to .cpp
Add operator= due to const member d
Add factor() to obtain the pixelConversion value

Test Plan

checked unit conversions in property editor for 3 different items in kreportexample

Diff Detail

Repository
R14 KReport
Branch
dptr-unit
Lint
No Linters Available
Unit
No Unit Test Coverage
piggz updated this revision to Diff 9291.Dec 22 2016, 2:54 PM
piggz retitled this revision from to Add dtpr to KReportUnit.
piggz updated this object.
piggz edited the test plan for this revision. (Show Details)
piggz added a reviewer: staniek.
Restricted Application added a project: KReport. · View Herald TranscriptDec 22 2016, 2:54 PM
staniek requested changes to this revision.Dec 22 2016, 4:00 PM
staniek edited edge metadata.
staniek added inline comments.
src/common/KReportUnit.cpp
439

Here and in many other places: { -> \n{

468

Can we have the ctor and dtor on the top at line 50 or so?

src/common/KReportUnit.h
101

unit -> type?

105

Needs docs like this:

Assigns specified type and factor 1.0 to the object

105

unit -> type?

107

If we have operator= then also the copy ctor needs to be implemented explicitely.

123

While we can't keep the code that uses d-pointer inline I propose to keep all these short static methods inline. And even prepend inline keyword to them.

218

If it's inline let's keep the code in the header.

This revision now requires changes to proceed.Dec 22 2016, 4:00 PM
piggz marked 6 inline comments as done.Dec 22 2016, 5:06 PM
piggz added inline comments.
src/common/KReportUnit.h
101

im not sure...the whole class is about Units ... what about Type > Unit? or leave?

123

Does it make sense to have static-inline? me research seems inconclusive :)

piggz updated this revision to Diff 9294.Dec 22 2016, 5:07 PM
piggz edited edge metadata.
piggz marked an inline comment as done.
  • Address review comments
staniek requested changes to this revision.Dec 22 2016, 5:31 PM
staniek edited edge metadata.
staniek added inline comments.
src/common/KReportUnit.h
101

I understand Type as a type of unit. KReportUnit::Unit sounds problematic, KReportUnit::Type sounds ok. Since the type name is Type then I proposed name of the argument to be called type to also follow that term.

123
This revision now requires changes to proceed.Dec 22 2016, 5:31 PM
staniek added inline comments.Dec 22 2016, 5:40 PM
src/common/KReportUnit.h
123

Remove all }; from the header

piggz marked 6 inline comments as done.Dec 22 2016, 5:44 PM
piggz updated this revision to Diff 9295.Dec 22 2016, 5:54 PM
piggz edited edge metadata.
  • Address review comments
piggz updated this revision to Diff 9296.Dec 22 2016, 5:57 PM
piggz edited edge metadata.
  • Missing bits
piggz marked an inline comment as done.Dec 22 2016, 5:57 PM
piggz updated this revision to Diff 9297.Dec 22 2016, 6:09 PM
  • Missed one
staniek requested changes to this revision.Dec 22 2016, 9:46 PM
staniek edited edge metadata.
staniek added inline comments.
src/common/KReportUnit.h
85

Note for later: I plan something for ListOption: T4967

92

Sorry for asking for more but when we're at the topic of ctors, can we move these two statics to line 123 or so where the statics are?

This revision now requires changes to proceed.Dec 22 2016, 9:46 PM
piggz updated this revision to Diff 9306.Dec 22 2016, 10:05 PM
piggz edited edge metadata.
piggz marked an inline comment as done.
  • Move static functions to be wither others
staniek accepted this revision.Dec 22 2016, 10:14 PM
staniek edited edge metadata.

Cool.

This revision is now accepted and ready to land.Dec 22 2016, 10:14 PM
This revision was automatically updated to reflect the committed changes.