[textlayout] Don't enter infinite loop when table is misfit
Needs ReviewPublic

Authored by anthonyfieroni on Sep 11 2018, 1:43 PM.

Details

Reviewers
danders
boemann
Group Reviewers
Calligra: 3.0
Summary

Also QTextLine can crash due to its validity, Qt implementation incorporate a pointer that isn't checked exclusively but in isValid

CCBUG: 381341

Test Plan

Attached document is now open and pretty responsive due to its 243 pages :)

Diff Detail

Repository
R8 Calligra
Lint
Lint Skipped
Unit
Unit Tests Skipped
anthonyfieroni created this revision.Sep 11 2018, 1:43 PM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptSep 11 2018, 1:43 PM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald Transcript
anthonyfieroni requested review of this revision.Sep 11 2018, 1:43 PM
anthonyfieroni retitled this revision from [textlayout] Don't enter infinite loop when table are misfit to [textlayout] Don't enter infinite loop when table is misfit.
anthonyfieroni edited the summary of this revision. (Show Details)Sep 11 2018, 1:47 PM

Initialize labelYOffset even layout line is not valid

boemann added inline comments.Sep 11 2018, 8:08 PM
libs/textlayout/KoTextLayoutNoteArea.cpp
143

I'm fine with this change

libs/textlayout/KoTextLayoutTableArea.cpp
464

nah this is too aggressive.
There are definitely cases where setting to 0 is the correct thing to do.
There might be some times we enter an infinite loop yes, but we need to catch this is some other way

anthonyfieroni added inline comments.Sep 11 2018, 9:05 PM
libs/textlayout/KoTextLayoutTableArea.cpp
464

I should see how this would work, because if we reset it to 0 every time it takes same way to parse, as it can see cursor->row is force reset only here.

The idea is that if we are at the beginning of a new page we don't get in here (virginpage is true)
so we only reset if we are somewhere down on a page meaning we wil have more space to try on next time around
But we should reset if the only thing we fitted was the table header as a table headerrow should never be the only thing on a page

One reason why this might fail is if virginpage becomes false when we add the table headerrow (if we can fix this and that doesn't have other ill effect then I would prefer such a solution).
So find out where we set virginpage to false, and make sure it doesn't go from true to false when adding the header row

I hope this makes sense

One reason why this might fail is if virginpage becomes false when we add the table headerrow (if we can fix this and that doesn't have other ill effect then I would prefer such a solution).
So find out where we set virginpage to false, and make sure it doesn't go from true to false when adding the header row

I hope this makes sense

Only one i can see before https://phabricator.kde.org/source/calligra/browse/master/libs/textlayout/KoTextLayoutArea.cpp$540 to setted to true every time what you think?

I am more thinking of this place:

https://phabricator.kde.org/source/calligra/browse/master/libs/textlayout/KoTextLayoutTableArea.cpp$436

this shouldn't be set if we are doing a headerrow, so please try this:

if (cursor->row >= d->headerRows)

setVirginPage(false);

It may still not be enough to fix the problem but it's step in the right direction i think
There is still the possibility that the user has completely written more than can be fitted - I wonder what we should do in that case then

anthonyfieroni added a comment.EditedSep 12 2018, 12:45 PM

It does not work, the problem is that d->headerRows is 0

Something as

if (d->headerRows) {
    cursor->row = 0;
}

Another try:
Don't mark table as misfit when it does not have header rows

Any other ideas for special handling of headerRows == 0?

Let's make some fix about that.

Would it be possible to make a unit test?

Had a closer look at this. Afaics we get an infinit loop when a table is 'totally misfit',
Can't say I understand the table layout logic, but my assumption is that if a row is a total misfit it can't just be ignored,
so breaking off the row layout loop in this case seems to work:

@danders that was one of my previous approach. Let @boemann tolds us which is correct one.

Ok, think I'm on to something.
Testing with the 1.doc in bug 381341. It seems it fails on the table in approx page 222 (open in LO)
with text in 0,0: Экономический субъект
Stepping through the KoTextLayoutTableArea::layoutRow(), it seems *all* columns in row 0 (at least) spans rows and hence (I think) layouting does not work.

I probably don't have more time to spend on this before next week, so please have a look :)

I ended up in the same spot as you:
Since all columns in row 0 spans rows, totalMisFit will always be set to true and the whole table is layed out on next page, and next page again and again ...

I'm not 100% certain just adding noCellsFitted to the condition covers all bases:
We try to layout all headerrows plus 1, but afaics we have to considder spanned rows when first nonheaderrow spans rows.
See attached path for a suggested solution.
(Maybe also if last headerrow spans rows, but well, probably not a common case ;)

Also, if this minimum does not fit on a virgin page, I think we should just lay it out and hope for the best.
I added some code for this in the attached patch, but I'm not sure it is the right way, or complete.

Yep, it's look like same approach to mine, did you try mine or it's not correct in all cases?

Dan I like your diff better - I don't think it's completely there but it's a better starting point

On a more conceptual level, what should happen if the design of table is such that headers can't fit on a virgin page? What should we do.? One one hand it should be there but on the other hand we will never get to show the real part of the table ever.
What is the solution we should aim for?

Yep, it's look like same approach to mine, did you try mine or it's not correct in all cases?

Mine tries to take into account the case where the first row after header is spanning rows, I don't think yours do?

Dan I like your diff better - I don't think it's completely there but it's a better starting point

On a more conceptual level, what should happen if the design of table is such that headers can't fit on a virgin page? What should we do.? One one hand it should be there but on the other hand we will never get to show the real part of the table ever.
What is the solution we should aim for?

I thought I had covered that case by not setting totalMisFit (but I'm not certain). I thought that rows then would be layed out regrdless, but I may have missed a trick.

@danders you can commandeer this revision to update diff.

No you misunderstand. I wasn't talking about you diff - I want to know what it is we are trying to accomplish. in spoken words

No you misunderstand. I wasn't talking about you diff - I want to know what it is we are trying to accomplish. in spoken words

Ahhh. It's a guard against loop/crash in extreme cases.

In real life this should (almost) never happen, maybe when changing page size or if we do not have the correct font.
I think the only thing to do is to just layout the header rows over multiple pages (as we do with non-header rows).

In facts that's 2 crashes, first one in ascent, second one misfit.

Still not the description i was looking for. I want to know what the extreme case is and what the resulting document should look like when we give up:

A table without header rows and that doesn't fit will be layouted and shown in pieces

What I want to know is: What should we do?.

  1. Should we not show the header rows- it will leave the user without context but at least the user will get to see normal rows
  2. should we layout one line past the headerrows even though that will never be visible on the printed paper (and then nothing interesting will ever be visible) - Should we then show the table at all instead of 100s of pages with just headerrows until we have disposed of all the contents one line at a time.

You see - to me - it's not at all clear what it is we are trying to accomplish

And has anyone been able to produce a smaller 1 page example of the document - we are stumbling blindly here.
Do we have an odf snippet of the table that gives the problem?

Fidled a little more with this, and found several problems when table with merged cells is split over pages,
like unmerged cell painted on both pages (empty on first page), caret not shown in selected cell and sometimes shown in prev cell.

Tried to recreate problem with words, which is easy, both with Anthonies (D15428) patch and with master:

  • Create a 2x1 table then merge the two rows:

master: Table disappears (expected, as layout code is skipped)
D15428: Table is displayed as it should be.

  • Create a table larger than a page, then merge the first 2 rows:

master: Loops
D15428: Table is displayed as it should.

I encountered an assert in KoTextLayoutTableArea::selectionBoundingBox() line 342,
but this is not a new problem and unrelated to D15428.
To recreate: Add a table, merge two rows, add a row below the merged rows -> assert.

I also checked out how LO handles this. In imvvho they don't do a great job, but they don't freeze nor crash ;)
As you aptly point out Camilla, we need to define in more detail what to do in different cases, but as always
LO is probably the standard even when they are not good.

Based on all this, I retract all my suggestions/proposals/patches.
Imvvho Anthonies patch adresses two concrete problems (a loop and a crash) and afaics it does not introduce new problems,
so I propose it is accepted.

good investigative work, but I fear those tests are way too simple to dare apply the patch.

The table code handles a lot more cases than a simple 2x1 table merged will uncover. I'm a bit surprised of the problems you describe in master - I don't recall any such problems.

I think it is time that we create a lot of unittests. Unittesting table layout is long overdue anyway

Ran this patch against new unit tests and it fixes all the expected ones, namely those where all cells in first row is merged with cells in second row.
Test results without patch: https://build.kde.org/job/Calligra/job/calligra/job/kf5-qt5%20SUSEQt5.10/lastCompletedBuild/testReport/projectroot.libs.textlayout/tests/libs_kotextlayout_TestTableLayout
See uploaded file for result with patch.

Well, unit tests has been added that fails without this patch and passes with this patch.
If there are more test cases needed, please add them so that we can get this patch committed soon...

Can we get a conclussion to this?
@Camilla Have you come up with any more unit tests?

Let's commit this, it's fair small patch but it covers some unwanted crashes.