Improve ORODocument
ClosedPublic

Authored by piggz on Nov 19 2016, 12:05 AM.

Details

Summary

Port OROPage

Port OROSection

Port and improve OROPrimitive

Port OROTextBox

Port OROLine

Port OROImage

Port OROPicture

Port OPORect and OROEllipse

Port OROCheck

Clone using methods not accessing the d pointer directtly

Test Plan

run kreportexample and ensure rendering as expected

Diff Detail

Repository
R14 KReport
Branch
dptr_oro
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
staniek added inline comments.Nov 29 2016, 12:46 AM
src/common/KReportRenderObjects.cpp
45

clear is not really needed

48

same here

66–71

Second arg not needed

71

Second arg not needed, default constructed value is returned here, which for Qt is nullptr.

90–95

Second arg not needed

100

Second arg not needed

143

For symmetry don't we want to also delete page?

148

For symmetry don't we want to also delete section?

183

not really needed

199

not really needed

218

Second arg not needed

223

Second arg not needed

267

For symmetry don't we want to also delete primitive?

287

KReport::recordCount() is qint64 so how about using qint64 here too?

293

Let's initialize all members here because it's possible.

321

Second arg not needed

326

Second arg not needed

421

If we add delete to OROPage::removePrimitive() this line won't be correct. Either:

  • Add void OROPage::takePrimitive(OROPrimitive* primitive) and use it here, or
  • call d->page->d->primitives.removeOne(this)
470

TODO: rename to KReportTextStyleData, add initialization of KReportTextStyleData::alignment and KReportTextStyleData::backgroundOpacity.

484–492

Let's initialize all this in Private::Private(). Cleaner and less Coverity warnings.

558–583

can be clone() const

592

Missing copying:

  • wordWrap
  • canGrow
  • requiresPostProcessing
642

can be clone() const

716

can be clone() const

748

can be clone() const

818

can be clone() const

873–878

can be clone() const

955

can be clone() const

src/common/KReportRenderObjects.h
58

+title param.

63

While we're looking here. Add docs informing that ownership is transferred.

65

Add docs informing that the page object will be removed and deleted.

70

Add docs informing that ownership is transferred.

71

Setion -> Section

71

Add docs informing that the section object will be removed and deleted.

201

How about keeping X and Y by using Qt::Orientation and removing Sort?

210

+const

280

Not done...

322

let's add arg name

324–325

let's add arg name

327–328

let's add arg name

347–348

let's add arg name

350–351

let's add arg name

402–403

let's add arg name

422–423

let's add arg name

425–426

let's add arg name

427–428

let's add arg name

This revision now requires changes to proceed.Nov 29 2016, 12:47 AM
piggz marked 39 inline comments as done.Nov 30 2016, 9:20 PM
piggz updated this revision to Diff 8649.Nov 30 2016, 10:01 PM
piggz edited edge metadata.
piggz marked 3 inline comments as done.
  • Implement review comments

Not done TODO as that will be done in another class

piggz marked 2 inline comments as done.Nov 30 2016, 10:08 PM
piggz updated this revision to Diff 8650.Nov 30 2016, 10:09 PM
piggz edited edge metadata.
  • 2 missed comments
staniek requested changes to this revision.Nov 30 2016, 11:13 PM
staniek edited edge metadata.

Almost there!

src/common/KReportRenderObjects.cpp
592

TODO

963

TODO

src/common/KReportRenderObjects.h
58

TODO

71

TODO

225

+param name

280

TODO

src/renderer/KReportHTMLCSSRenderer_p.cpp
96

TODO

src/renderer/KReportKSpreadRenderer.cpp
49

typo

This revision now requires changes to proceed.Nov 30 2016, 11:13 PM
piggz updated this revision to Diff 8712.Dec 2 2016, 10:57 PM
piggz edited edge metadata.
piggz marked 19 inline comments as done.
  • Implement remaining TODOs
staniek requested changes to this revision.Dec 4 2016, 12:10 AM
staniek edited edge metadata.

Almost there just one thing - xLessThan!

src/common/KReportRenderObjects.h
229–232

I mean xLessThan

This revision now requires changes to proceed.Dec 4 2016, 12:10 AM
piggz added a comment.Dec 4 2016, 9:11 AM

Yes, i meant to ask a question about that.

What do you mean 'move to cpp'?

Do you mean, it will remain a static method of OROSection, OROSection::Private or just a static function that is not a member of anything?

In D3423#66959, @piggz wrote:

Yes, i meant to ask a question about that.

What do you mean 'move to cpp'?

Do you mean, it will remain a static method of OROSection, OROSection::Private or just a static function that is not a member of anything?

Sorry Adam, I mean a static function in cpp, one that is not a member of anything.

piggz updated this revision to Diff 8751.Dec 4 2016, 10:11 PM
piggz edited edge metadata.
  • Move function to cpp
piggz marked 2 inline comments as done.Dec 4 2016, 10:12 PM

all done!

staniek requested changes to this revision.Dec 4 2016, 10:14 PM
staniek edited edge metadata.
staniek added inline comments.
src/common/KReportRenderObjects.cpp
23

Let's have static or use namespace { }

This revision now requires changes to proceed.Dec 4 2016, 10:14 PM
piggz added a comment.Dec 5 2016, 7:11 AM

Having read the differences between static and using a namespace I will use static. Hopefully we can close this later this evening :)

piggz updated this revision to Diff 8794.Dec 5 2016, 9:39 PM
piggz edited edge metadata.
  • Final comment!
staniek accepted this revision.Dec 5 2016, 10:09 PM
staniek edited edge metadata.

Good job :)

This revision is now accepted and ready to land.Dec 5 2016, 10:09 PM
This revision was automatically updated to reflect the committed changes.
staniek added inline comments.Dec 9 2016, 5:15 PM
src/plugins/CMakeLists.txt
1

Do you remember the reason?

piggz added a comment.Dec 9 2016, 5:47 PM

Yes, it was becuase we un-exported the classes needed by plugins. We can re-enable it, and I will also create a task to create a new barcode plugin based on some other library.