D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

This revision was automatically updated to reflect the committed changes.
Closed by commit R8:76cfa3b654b2: [textlayout] Dont enter infinite loop 
when table is misfit (authored by anthonyfieroni).

REPOSITORY
  R8 Calligra

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15428?vs=41564=77492

REVISION DETAIL
  https://phabricator.kde.org/D15428

AFFECTED FILES
  libs/textlayout/KoTextLayoutNoteArea.cpp
  libs/textlayout/KoTextLayoutTableArea.cpp

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, cochise, 
vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders accepted this revision.
danders added a comment.
This revision is now accepted and ready to land.


  There are unit tests that fail without this, so if somebody doas not come up 
with code that shows this is wrong, please commit.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, cochise, 
vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  no

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  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...

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  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.F6552719: LastTest.log 


REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  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.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  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?

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  In D15428#378432 , @boemann wrote:
  
  > 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).

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


  @danders you can commandeer this revision to update diff.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  In D15428#378422 , @boemann wrote:
  
  > 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.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  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?

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  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.
  F6482937: y.diff 

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  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 :)

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  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:
  
  F6473096: x.diff 

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Dag Andersen]

danders added a comment.


  Would it be possible to make a unit test?

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


  Ping

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


  Let's make some fix about that.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 41564.
anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15428?vs=41418=41564

REVISION DETAIL
  https://phabricator.kde.org/D15428

AFFECTED FILES
  libs/textlayout/KoTextLayoutNoteArea.cpp
  libs/textlayout/KoTextLayoutTableArea.cpp

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


  It does not work, as you can see it overriding here 
https://phabricator.kde.org/source/calligra/browse/master/libs/textlayout/KoTextLayoutArea.cpp$571
 so that's why i want to set it true before loop.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D15428#324451 , @boemann wrote:
  
  > 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?

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added a comment.


  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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> boemann wrote in 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

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.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Camilla Boemann]

boemann added inline comments.

INLINE COMMENTS

> KoTextLayoutNoteArea.cpp:143
> +
> +if (blockLayoutLine.isValid()) {
> +d->labelYOffset += blockLayoutLine.ascent();

I'm fine with this change

> KoTextLayoutTableArea.cpp:464
> +//if we couldn't fit the header rows
> +//try again don't reset cursor->row or we enter infinite loop
> +//cursor->row = 0;

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

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 41418.
anthonyfieroni added a comment.


  Initialize labelYOffset even layout line is not valid

REPOSITORY
  R8 Calligra

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15428?vs=41409=41418

REVISION DETAIL
  https://phabricator.kde.org/D15428

AFFECTED FILES
  libs/textlayout/KoTextLayoutNoteArea.cpp
  libs/textlayout/KoTextLayoutTableArea.cpp

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

anthonyfieroni edited the summary of this revision.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever

D15428: [textlayout] Don't enter infinite loop when table is misfit

[Anthony Fieroni]

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".

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15428

To: anthonyfieroni, #calligra:_3.0, danders, boemann
Cc: Calligra-Devel-list, dcaliste, cochise, vandenoever