Also QTextLine can crash due to its validity, Qt implementation incorporate a pointer that isn't checked exclusively but in isValid
I'm fine with this change
nah this is too aggressive.
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
I am more thinking of this place:
this shouldn't be set if we are doing a headerrow, so please try this:
if (cursor->row >= d->headerRows)
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
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:
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.
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?
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).
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?.
You see - to me - it's not at all clear what it is we are trying to accomplish
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:
master: Table disappears (expected, as layout code is skipped)
D15428: Table is displayed as it should be.
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...