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

This revision was automatically updated to reflect the committed changes. Closed by commit R8:76cfa3b654b2: [textlayout] Don't enter infinite loop when table is misfit (authored by anthonyfieroni). REPOSITORY R8 Calligra CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15428?vs=41564&i

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

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://phabric

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

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, vandeno

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

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

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, vand

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

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/D1

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

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/lastCompleted

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

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

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

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 pro

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

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

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

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

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

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

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

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

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

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

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

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 suc

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

danders added a comment. In D15428#378420 , @anthonyfieroni wrote: > 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

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

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 b

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

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,

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

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 cov

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

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 (

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

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, co

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

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 cas

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

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

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

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

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

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&id=41564 REVISION DETAIL https://

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

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

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://phabrica

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

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

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

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 suc

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

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 th

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

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

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

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

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

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&id=41418 REVISION DETAIL https://phabricator.kde.org/D1

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

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

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,