Also QTextLine can crash due to its validity, Qt implementation incorporate a pointer that isn't checked exclusively but in isValid
CCBUG: 381341
danders | |
boemann |
Calligra: 3.0 |
Also QTextLine can crash due to its validity, Qt implementation incorporate a pointer that isn't checked exclusively but in isValid
CCBUG: 381341
Attached document is now open and pretty responsive due to its 243 pages :)
Lint Skipped |
Unit Tests Skipped |
libs/textlayout/KoTextLayoutNoteArea.cpp | ||
---|---|---|
143 | I'm fine with this change | |
libs/textlayout/KoTextLayoutTableArea.cpp | ||
464 | nah this is too aggressive. |
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
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:
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
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.
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?
Mine tries to take into account the case where the first row after header is spanning rows, I don't think yours do?
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.
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).
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
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:
master: Table disappears (expected, as layout code is skipped)
D15428: Table is displayed as it should be.
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?
There are unit tests that fail without this, so if somebody doas not come up with code that shows this is wrong, please commit.