Use suffix option from the new KProperty instead of units; implement units locally in KReport
ClosedPublic

Authored by piggz on Apr 13 2017, 10:07 PM.

Details

Summary

+ reuse constants/macros for defaults a lot more
+ add and use more unit conversion helpers (for size and pos in particular)
+ better name some unit conversion functions
+ better code reuse for reading of typical XML elements/attributes
+ switch from using const char* to QString for attribute names because the DOM API uses QString anyway

  • this will help soon to switch from QLatin1String to QStringLiteral

+ fix changing units
+ fix page margins handling

KReportExample and Kexi works

Test Plan

Run KReportExample and Kexi.

  • Test 1. Try to insert all report elements. Open existing reports in Kexi. Preview them. Expected: correct positions of elements in design view and preview.
  • Test 2. Change page units to all other units. Expected: correct property values and positions of elements in design view and preview -- values and suffixes (units) follow changes and effective positions/sizes are not not affected by current unit settings. Similarly, margin values update properly, custom page width/height update correctly (even if non-custom page size is in use).
  • Test 3. Change page size to custom, change page units to all other units. Expected Design view and preview should show correct size, report elements stay at their positions and have constant sizes regardless of the unit picked.

Diff Detail

Repository
R14 KReport
Branch
arcpatch-D5441_3
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

Buildfix from master

staniek updated this revision to Diff 13437.Apr 14 2017, 3:48 PM

Update conflict

staniek updated this revision to Diff 13438.Apr 14 2017, 4:29 PM
  • Fix unit conversions in the report line element
piggz accepted this revision.Apr 15 2017, 8:05 AM
This revision is now accepted and ready to land.Apr 15 2017, 8:05 AM
staniek planned changes to this revision.Apr 28 2017, 11:35 AM
staniek edited the summary of this revision. (Show Details)

There are 2 TODOs (see the description) before pushing it.

piggz commandeered this revision.Sep 5 2017, 7:36 PM
piggz edited reviewers, added: staniek; removed: piggz.
piggz updated this revision to Diff 19211.Sep 5 2017, 7:36 PM
piggz edited the summary of this revision. (Show Details)

rebased

piggz updated this revision to Diff 19219.Sep 6 2017, 7:18 AM
  • Fixup margins to get basic rendering working again
piggz updated this revision to Diff 19220.Sep 6 2017, 7:34 AM
  • When changing section unit, set the unit before setting the height to
piggz updated this revision to Diff 19234.Sep 6 2017, 3:03 PM
  • Fix crash when changing unit of a line
piggz updated this revision to Diff 19241.Sep 6 2017, 4:02 PM
  • Convert margins correctly when calculating page width
staniek requested changes to this revision.Sep 7 2017, 9:43 PM

1st step done: reviewed the rebase https://phabricator.kde.org/D5441?vs=13438&id=19211. Good job!

src/common/KReportDesign_p.cpp
64

typefor -> type for

This revision now requires changes to proceed.Sep 7 2017, 9:43 PM
staniek edited the test plan for this revision. (Show Details)Sep 11 2017, 9:20 AM
staniek edited the test plan for this revision. (Show Details)Sep 11 2017, 9:30 AM
staniek edited the test plan for this revision. (Show Details)Sep 11 2017, 9:42 AM
staniek added a comment.EditedSep 11 2017, 9:58 AM

Thanks Adam, that's gigantic step forward, we're almost there.

Please see the updated 3 tests on the top so we look at the same aspects.

  • Test #1

All seems to be OK

  • Test #2

Most things are OK.

OK: correct property values of element positions and sizes

OK: correct suffixes (units) of element positions and sizes properties (mm -> cm - in most cases, see below)

OK: correct property values of margins, as physical sizes on screen as expected - not affected by unit changes

OK: correct suffixes (units) of margins

  • Issue 2.1. Not OK: elements disappear after changing from mm to dm or (sometimes inches and even sometimes cm) (so also cannot verify correctness of suffixes and values), changing back to mm or cm restores the correct state and values

Fixed by 754f3292d79e.

  • Issue 2.2. Not OK: custom page width/height values do not update correctly after unit change
  • Issue 2.3. Not OK: custom page width/height units missing (we have probably just not implemented that yet and should do)
  • Issue 2.4. Not OK:

Step 1. Select report, ensure mm is the Page unit and Detail section's height is 20mm.
Step 2. Change Page unit to Cicero (cc)
Result: the section's height is now 5cc which is not equivalent of 20m but 22.5mm
Expected: equivalent in cc should be set, that is 4.42 (4.41530826634 before rounding)
Extra info #1: Possible regression, no such defect in kreportexample 3.0.2.
Extra info #2: No such defect wrt page margins or page size

  • Test #3: all is OK but

OK: Change page size to Custom from the default initially
Issue 3.1. Not OK: Change units afterwards: like in Issue 2.1, elements disappear in design view e.g. after changing unit from mm to inches but stay OK in preview. Preview can disappear entirely sometimes too. Changing back to mm restores the state and all is allright.

src/common/KReportItemBase.cpp
255

Minor

Move { to new line

piggz updated this revision to Diff 19850.Sep 24 2017, 8:38 AM
piggz edited the test plan for this revision. (Show Details)
  • Fix items dissapearing
piggz added a comment.Sep 24 2017, 8:44 PM

fixed 2.1 and 2.3

In D5441#148682, @piggz wrote:

fixed 2.1 and 2.3

Great, I understand it's not yet pushed.

Added new issue 2.4. Maybe it's fixed in the meantime by you but maybe not.

In D5441#148425, @piggz wrote:
  • Fix items dissapearing

Thanks, marked Issue 2.1 as resolved.

piggz updated this revision to Diff 19954.Sep 26 2017, 7:19 PM
  • Fix custom page size handling when changing units

Fixes 2.2

piggz updated this revision to Diff 19955.Sep 26 2017, 7:48 PM
  • Fix section changing height when untis changed

Fixes 2.4...pls check nothing else breaks

staniek updated this revision to Diff 20014.Sep 27 2017, 9:39 PM

Merge with master

staniek added a comment.EditedSep 27 2017, 10:15 PM

All OK @piggz! Works so well now.

Found more:

Test #4: Grids

  • 4.1. Change page size to custom. Issue: designer's Detail area is not fully repainted. Only changing height of the detail section repaints it eventually.

  • 4.2. Grid size changes when unit changes, so accuracy of snapping changes. But we would assume that user changes units for measuring but wants to keep acuracy. Expected: effective size of grid should not change too much. The only units that work together are mm, cm, in:
  • grid for mm is 10mm == 1cm, OK
  • grid for cm is 1cm, OK
  • grid for inches is 1in, OK

Then:

  • grid for dm is 1dm == 10cm, shall be 0.1dm
  • grid for pica and cicero is 10, that's big, shall be 5?
  • grid for points is 100, shall be 50?

Test #5: Margins

  • 5.1. Scroll over the Right margin value to increase it. Issue: the added value adds offset to the the grid. Expected: no such thing, grid should stay as margin is nonvisual in the designer area. The same happens when Left margin is changed this way.


  • 5.2. Missing maximums for margins. The maximums (the "max" property option) should be changed whenever the printable area changes, i.e. when
  • any margin is changing
  • page size type is changing
  • page size type is Custom and the custom size is changing

Example invalid view for too large margin:

  • 5.3. Missing fixups for page margins. The "fixups" means reducing respective margins as soon as this is needed to keep desired page size. Scenarios:

5.3.1. Left margin was enlarged so the printable area (PA) has zero width and left+right > page.width. Solution: fixup the left to page.width-right
5.3.2. Right margin was enlarged so the printable area (PA) has zero width and left+right > page.width. Solution: fixup the right to page.width-left
5.3.3. Top margin was enlarged so the printable area (PA) has zero height and top+bottom > page.height. Solution: fixup the top to page.height-bottom
5.3.4. Bottom margin was enlarged so the printable area (PA) has zero height and top+bottom > page.height. Solution: fixup the bottom to page.height-bottom

5.3.5. Page size was changed so the printable area has negative width. Solution: fixup the left and right equally by adding (page.width-left-right)*0.75 to them.

Example: Page width was 210mm with left margin 60mm and right margin 70mm. Page width changes to 110mm. Left margins are fixup by page.width-left-right)*0.75==(110-60-70)0.75==-20*0.75=-15mm. Now left margin is 45mm and right is 55mm. Page.width is thus 10mm.

5.3.6. Page size was changed so the printable area has negative height. Solution: fixup the top and bottom equally by adding (page.height-top-bottom)*0.75 to them.

Note for 5.3.*: Any fixups should use setValue(..., KProperty::ValueOption::IgnoreOld) so there is no step in undo buffer.

staniek updated this revision to Diff 20264.Oct 2 2017, 8:10 PM

Sync with master

staniek requested changes to this revision.Oct 2 2017, 8:12 PM
This revision now requires changes to proceed.Oct 2 2017, 8:12 PM
piggz added a comment.Oct 14 2017, 6:34 PM

these issues explain the call to slotResizeBarDragged(0) ... need to find a fix wthout introducing it ... calling update() doesnt cause a resize to happen

piggz updated this revision to Diff 21030.Oct 20 2017, 9:58 PM
  • Fix dynamic redraw of background. Resolved issue 4.1 and 5.1
staniek added a comment.EditedOct 20 2017, 10:22 PM
  • 6. If 0.00 value is entered it disappears in the prop. editor when moving focus to another editor. Data is saved though. Expected: 0.00 is visible. Example for margins:

This may or not be related to the KProperty libs and as such can belong to me :)

staniek added a comment.EditedOct 20 2017, 10:35 PM
  • 7.1. Grid Divisions property allows to enter <=0 valued. Expected: minimum value should be 1. (use property option for that)

For negative values we're loosing the grid. For 0 there's section height set to NaN and and it visually disappears.

  • 7.2. Grid Divisions property allows large integers. Example for 20, unit=cm:

In this case the grid visually disappears, not sure why. For unit=dm even 100 is OK. We need some display fix or setting some maximum.

  • 7.3. Set unit=dm, set Grid Divisions to 40, then mouse-wheel up to increase Grid Divisions. Expected: Grid Divisions increases and Grid Divisions editor still has focus.

Actual issue: proeprty set is changed to Details section and we're thus loosing the Grid Divisions context.

piggz updated this revision to Diff 21107.Oct 22 2017, 8:44 AM
  • Set min/max values for grid divisions
piggz added a comment.Oct 22 2017, 8:45 AM

Note, I cannot reproduce 7.3. Scrolling mouse over grididivsions works as expected.

piggz added a comment.Oct 22 2017, 8:53 AM
  1. If 0.00 value is entered it disappears in the prop. editor when moving focus to another editor. Data is saved though. Expected: 0.00 is visible. Example for margins:



    This may or not be related to the KProperty libs and as such can belong to me :)

This appears to be in kproperty ... the same happens in KPropertyExample, with the 'billions' property, which has a suffix. Doesnt happen in 'Dollars' property which only has a prefix.

staniek requested changes to this revision.Oct 23 2017, 8:29 PM

4.1, 5.1 fixed, good work. Added 5.2 and 5.3.

This revision now requires changes to proceed.Oct 23 2017, 8:29 PM
piggz marked an inline comment as done.Oct 23 2017, 8:39 PM

@piggz 7.x all fixed!

src/wrtembed/KReportDesigner.cpp
136

setValue(10, KProperty::DefaultValueOptions & KProperty::ValueOptions(KProperty::ValueOption::RememberOld)

Fixed in units makes 4.2 fixed!

piggz marked an inline comment as done.Oct 25 2017, 8:32 PM
piggz added a comment.Oct 25 2017, 8:52 PM
  1. If 0.00 value is entered it disappears in the prop. editor when moving focus to another editor. Data is saved though. Expected: 0.00 is visible. Example for margins:



    This may or not be related to the KProperty libs and as such can belong to me :)

it belongs to you ;)

piggz added a comment.Oct 25 2017, 8:58 PM

What do you think is a sensible max for margins? a % of the width/height? or some complex calculation involving calculating the size of the page and opposite margin?

In D5441#160067, @piggz wrote:

What do you think is a sensible max for margins? a % of the width/height? or some complex calculation involving calculating the size of the page and opposite margin?

In LO Writer the max is that the printable area is >= 5mm:

Maybe we can have the same.

In MS Word 2016 for a Letter format I was able to have printable area == 0.1" even. On error all MS Word does it raising error with no correction.

In D5441#160064, @piggz wrote:
  1. If 0.00 value is entered it disappears in the prop. editor when moving focus to another editor. Data is saved though. Expected: 0.00 is visible. Example for margins:



    This may or not be related to the KProperty libs and as such can belong to me :)

it belongs to you ;)

Fixed in kproperty.git af30f1805 :)

piggz updated this revision to Diff 21534.Oct 29 2017, 7:03 PM
  • Calculate sensible maximums for margin sizes
piggz added a comment.Oct 29 2017, 8:14 PM

could you confirm if 5.3.1 -> 5.3.4 are possible?

staniek updated this revision to Diff 21537.Oct 29 2017, 9:19 PM
  • Merge with kproperty API change 3.0.91
staniek added inline comments.Oct 29 2017, 9:20 PM
src/common/KReportDesign_p.cpp
137

Mistake: <%1> -> <%2>

src/common/KReportUtils_p.h
47

We never use this value directly so how about using consistently:

`const qreal SMALLEST_PAGE_SIZE_PT = MM_TO_POINT(5);

src/wrtembed/KReportDesigner.cpp
136

I've rebased master changes into this code: if you have unpublished fixes please do the same. Since kproperty 3.0.91 we're no longer using this complex math but instead: setValue(..., KProperty::ValueOption::IgnoreOld).

857

propertyName.startsWith("margin-") is not future-proof. What if we ever have properties like margin-background for example? ;)

1615

This is effectively a fix but usability of this a bit low: user needs to locate the longer margin first and reduce it, then increase the desired margin (left-right, top-bottom). It is a bit frustrating when "keyboard does not work" because of the the max limits.

How about setting max for left/right margins to page.width-smallest_page_size and max of top/bottom margins to page.height-smallest_page_size (e.g. for portrait A4: 205 and 205)? Then proceed with fixups from 5.3.[1-4].

For this solution we will need a KProperty parameter in this method so we know what property was changed and what property should be fixed. Please look that we never fix up value of property that is currently changed by the user, rather the other property.

In D5441#161577, @piggz wrote:

could you confirm if 5.3.1 -> 5.3.4 are possible?

The fix works great. I analyzed usability and my comments are here: https://phabricator.kde.org/D5441#inline-37061

@piggz I'd like to ask for review of D8518 ASAP so we have it in master otherwise number of conflicts increase! :)

staniek added inline comments.Oct 30 2017, 12:17 AM
src/common/KReportUtils.h
98

Do we need this specialized function in the public API?

staniek updated this revision to Diff 21578.Oct 30 2017, 4:06 PM
  • Merge branch 'master' into D5441
  • Make KReportUtils::readSizeAttributes() return default size also for invalid width or height; simplify readRectAttributes()
  • Use QRectF for bounding boxes/font metrics as long as we're using floating-point positions and sizes
  • Avoid double call of KReportDesignerItemRectBase::setSceneRect() what can lead to setting invalid size
  • Fix performance of sections resize, unit conversion, and allow to resize sections using property editor
staniek accepted this revision.Oct 30 2017, 4:11 PM

Please test relevant features after my last commit, I think we can then land this :)

Congrats Adam :)

This revision is now accepted and ready to land.Oct 30 2017, 4:11 PM
staniek edited the summary of this revision. (Show Details)Oct 30 2017, 4:14 PM
piggz added a comment.Oct 30 2017, 5:23 PM

You are happy with 5.x?

Maybe 5.x can be improved later.

piggz marked an inline comment as done.Oct 30 2017, 6:10 PM
This revision was automatically updated to reflect the committed changes.