API is simpler as uses QPageSize instead of custom defined sizes
Only ported a few of the page sizes in this version but more are easily added
Removed usage of QScreen to get DPI, and use KReportDpi everywhere, and
made KReportDpi return same for X and Y, as QPageSize uses a single resolution
to get px width/height.
Details
- Reviewers
staniek - Commits
- R14:fcc16e59f586: Port from KReportPageFormat to new KReportPageSize
Diff Detail
- Repository
- R14 KReport
- Branch
- qpagelayout-piggz
- Lint
No Linters Available - Unit
No Unit Test Coverage
src/common/KReportPageSize.h | ||
---|---|---|
26 | I don't quite like the KReportPageSize namespace. It's still not only about page *sizes* but also formats. First, let's look what are the differences to what the QPageSize delivers. More formats? PS: "A4" translated name feels more user-friendly than "ISO A4". | |
30 | How about using @return here and in each function below? | |
32 | There's QPageSize::name() which defined translated strings as names. How about using pageNames()? | |
37 | There's QPageSize::key() which defines page format strings as keys. How about using pageKeys()? | |
52 | Inconsistent. "format name" term used above in pageFormatNames(), and here "string". I propose a change. | |
src/wrtembed/KReportDesigner.cpp | ||
871 | Extra lines not needed... | |
src/wrtembed/KReportDpi.cpp | ||
75 ↗ | (On Diff #1292) | I don't quite understand this simplicity -- setDpi() sets m_dpiY but m_dpiY can't be accessed... Do we need the m_dpiY attr then? |
Better! Proposed renames.
src/common/KReportPageSize.h | ||
---|---|---|
32 | List -> list | |
32 | don't | |
43 | -> pageFormatNames() (synced the term with the QPageSize API) | |
48 | -> pageFormatKeys() (synced the term with the QPageSize API) | |
52 | I propose change to: /**
QString pageFormatKey(QPageSize::PageSizeId pageSizeId); | |
58 | KREPORT_EXPORT QPageSize::PageSizeId pageFormatSize(const QString& key); |
src/common/KReportPageSize.h | ||
---|---|---|
27 | Are you sure? All the namespace does is convert between string and QPageSizeId's .... if you are sure we can change it, but be sure :) | |
src/wrtembed/KReportDpi.cpp | ||
75 ↗ | (On Diff #1308) | Ive left it in incase we do in the future need to support different X/Y dpi, but as this is only for screen rendering, we're probably ok to keep it simple. What do you think? |
Much better. I am annoying and picky, I know. Gold coins are waiting....
src/common/KReportPageOptions.cpp | ||
---|---|---|
207–208 | I propose to change both methods to more object-oriented and very simple: QSizeF KReportPageOptions::pixelSize() const | |
src/common/KReportPageSize.cpp | ||
87 | new line please | |
src/common/KReportPageSize.h | ||
44 | Really done? | |
48 | Not done... | |
58 | size -> key | |
63 | QPageSize::PageSizeId -> QPageSize::PageSizeId id | |
src/renderer/KReportPreRenderer.cpp | ||
420 | let's removed unused | |
472 | let's removed unused | |
472–473 | still needed line? | |
473–474 | let's removed unused | |
src/renderer/KReportPrintRenderer_p.cpp | ||
55 | let's removed unused | |
85–86 | Wouldn't more ideal to use scaleX, scaleY and use them respectively below? | |
100–101 | I mean here, etc. | |
src/wrtembed/KReportDesigner.cpp | ||
860–861 | let's remove unused | |
860–861 | let's remove unused | |
864 | let's remove unused | |
876–877 | let's remove unused | |
src/wrtembed/KReportDpi.cpp | ||
75 ↗ | (On Diff #1625) | It simplifies but also a bit surprised because e.g API close to that, QPaintDevice::logicalDpi*() has both X and Y. I propose to have X and Y then... Is there particular difficulty? Existence of setDpi() suggests dpiY() is independent. PS: when you change please also remove the comment in line 72 |
Added more const
Added back dpiY
All calculations of page sizes use x and y dpi
Removed unused code
Cool. Now you seem to have a few \t characters. Hunt for them using your
kreport/.git/hooks/pre-commit
It's executed on git commit)
(download here https://paste.kde.org/ple8f9nxm/xiypkc/raw)