Re: Patch for when rowspan or colspan equals 0

Dear list,
        I've been working on my patch for implementing proper colspan and rowspan
behaviour.
        Forget almost completely my previous mail, here's a new way of doing it, way
better than the old one, which has no known corner-cases, it even handles
properly when both colspan and rowspan equals 0 in the same cell, whereas the
last one did but I couldn't catch them till recently.
        In essence the method now is:
        ->If we found a colspan = 0 we insert the cell and take ownership of all the
cells to the right that exist so far, update the colspan properly and insert
it in the QMap as such QMap<nextColWhenUpdateIsNeeded, cell>.
        ->Similarly for rowspan = 0, except that since the rows are sequential
whenever we find a row with such rowspan it has to be from that point on, it's
inserted similarly: QMap< nextRowWhenUpdateIsNeeded, cell>.
        ->On the next pass we check whether we're in an unhandled column for any of
the colspan=0 cells, if so, update properly (update its colspan and mark as
used the cell). We check the same for rowspans and store the column in which
no other cell should be placed.
        ->Process cell as usual, except if we're in a column in which no cell should
be inserted then advance to the next column.
The only corner case for both colspan and rowspan cells happens when it's a
new column in which we are dealing, that is we are at a position in which no
column has been inserted so far. Of course it is properly handled inserting a
new column.

So the algorithm seems to work quite well actually, except for one problem,
for rowspan > 2 for some reason it stops growing the cell even though we set
the rowspan properly! (I believe but my believings have been proven wrong in
the past ;).
        So we're this time _really_ nearly there. I'm attaching you the bunch of test
cases I've covered, and passed, so far (the previous ones had some
misspellings and I didn't realize till after sending the previous mail so it
lead me to the wrong conclusion that the previous algorithm worked, now I
believe they're actually right), also the test in
http://bugsfiles.kde.org/attachment.cgi?id=8334 is passed too except for the
fact that rowspan doesn't seem to want to grow greater than 2.
        I'll let you know if I could solve the rowspan > 2 problem, if so then the
patch wouldn't have any known problems, and would be waiting just for the
regression test to be commited.
--
Carlos

cell 0,0
cell 1,0	cell 1,1	cell 1,2
cell 2,1	cell 2,2	cell 2,3
cell 3,1	cell 3,2	cell 3,3
cell 4,1	cell 4,2	cell 4,3

cell 0,0
cell 1,0
cell 2,0	cell 1,1	cell 1,2
cell 3,1	cell 3,2	cell 3,3

cell 0,0
cell 1,0	cell 1,1	cell 1,2
cell 2,1	cell 2,2

cell 0,0
cell 1,0	cell 1,1	cell 1,2
cell 2,1
cell 3,1

cell 0,0	cell 0,1	cell 0,2
cell 1,0	cell 1,1
cell 2,0

cell 0,0	cell 0,1
cell 1,0	cell 1,1	cell 1,2	cell 1,3
cell 2,0
cell 3,0	cell 3,1	cell 3,2
cell 4,0

[tables.diff]

---

Index: html/html_tableimpl.cpp
===================================================================
--- html/html_tableimpl.cpp (revisión: 948444)
+++ html/html_tableimpl.cpp (copia de trabajo)
@@ -210,7 +210,7 @@
     lastSection = section;
     if ( index < 0 ) //append/last mode
         return 0;
-    
+
     long rows   = section->numRows();
     if ( index >= rows ) {
         section =  0;
@@ -428,7 +428,7 @@
 
     return cellChanged;
 }
-
+
 void HTMLTableElementImpl::parseAttribute(AttributeImpl *attr)
 {
     // ### to CSS!!
@@ -910,14 +910,14 @@
         // limit this to something not causing an overflow with short int
         if(rSpan < 0 || rSpan > 1024) rSpan = 1;
         if (renderer())
-            renderer()->updateFromElement();                    
+            renderer()->updateFromElement();
         break;
     case ATTR_COLSPAN:
         cSpan = attr->val() ? attr->val()->toInt() : 1;
         // limit this to something not causing an overflow with short int
         if(cSpan < 0 || cSpan > 1024) cSpan = 1;
         if (renderer())
-            renderer()->updateFromElement();        
+            renderer()->updateFromElement();
         break;
     case ATTR_NOWRAP:
         if (attr->val() != 0)
@@ -964,7 +964,7 @@
     {
     case ATTR_SPAN:
         _span = attr->val() ? attr->val()->toInt() : 1;
-        if (_span < 1) _span = 1;
+        if (_span < 0) _span = 1;
         break;
     case ATTR_WIDTH:
         if (!attr->value().isEmpty())
Index: rendering/render_table.cpp
===================================================================
--- rendering/render_table.cpp (revisión: 948444)
+++ rendering/render_table.cpp (copia de trabajo)
@@ -182,7 +182,7 @@
                     head = static_cast<RenderTableSection *>(child);
                 } else {
                     resetSectionPointerIfNotBefore(firstBody, beforeChild);
-                    if (!firstBody)
+                    if (!firstBody)
                         firstBody = static_cast<RenderTableSection*>(child);
                 }
             }
@@ -761,10 +761,10 @@
                 maxCols = sectionCols;
         }
     }
-    
+
     columns.resize(maxCols);
     columnPos.resize(maxCols+1);
-    
+
     needSectionRecalc = false;
     setNeedsLayout(true);
 }
@@ -1190,44 +1190,115 @@
  }
     }
 
+    //What:Postion the cell
+    //How: we take care of special case when colspan or rowspan equals 0
+    //after that position the cell, that is just to tell where is it and tell
+    //other cells where they can't be located (marking the cells as -1)
+    //later taking the span into account (and in other function) the cell is
+    //then painted (that's why we need to set the colspan and rowspan properly
+    //when any of them is cero
+
     // make sure we have enough rows
-    ensureRows( cRow + rSpan );
+    ensureRows( cRow + (rSpan)? rSpan : 1 );
 
     grid[cRow].rowRenderer = row;
 
     int col = cCol;
     // tell the cell where it is
     RenderTableCell *set = cell;
-    while ( cSpan ) {
- int currentSpan;
- if ( cCol >= nCols ) {
-    table()->appendColumn( cSpan );
-    currentSpan = cSpan;
- } else {
-    if ( cSpan < columns[cCol].span )
- table()->splitColumn( cCol, cSpan );
-    currentSpan = columns[cCol].span;
- }
- int r = 0;
- while ( r < rSpan ) {
-    if ( !cellAt( cRow + r, cCol ) ) {
-// qDebug("    adding cell at %d, %d",  cRow + r, cCol );
- cellAt( cRow + r, cCol ) = set;
-    }
-    r++;
- }
- cCol++;
- cSpan -= currentSpan;
- set = (RenderTableCell *)-1;
+    kDebug()<<"row"<<cRow<<"col"<<cCol;
+    kDebug()<<"cSpan"<<cSpan<<"rSpan"<<rSpan;
+    //check wether we are in a column or in a row with span = 0
+    int actualCurrentCol = table()->effColToCol(cCol);
+    if( cellsWithColSpanZero.contains( actualCurrentCol ) ) {
+        while ( RenderTableCell* cell = cellsWithColSpanZero.take( actualCurrentCol ) ) {
+            int cellRow = cell->row();
+            if( !cellAt( cellRow, actualCurrentCol ) ) {
+                cellAt( cellRow, actualCurrentCol ) = (RenderTableCell *)-1;
+            }
+            cell->setColSpan( cell->colSpan() + 1 );
+            cellsWithColSpanZero.insertMulti(actualCurrentCol + 1, cell );
+        }
     }
+    QList< int > columnsToAvoid;
+    if( cellsWithRowSpanZero.contains( cRow ) ) {
+        while( RenderTableCell* cell = cellsWithRowSpanZero.take( cRow ) ) {
+            int cellCol = cell->col();
+            if( !cellAt( cRow, cellCol ) ) {
+                cellAt( cRow, cellCol ) = (RenderTableCell *)-1;
+            }
+            cell->setRowSpan( cell->rowSpan() + 1 );
+            columnsToAvoid << cellCol;
+            cellsWithRowSpanZero.insertMulti( cRow + 1, cell );
+            kDebug()<<"final rowspan"<<cell->rowSpan();
+        }
+    }
+
+    //Save the column if the its span is 0
+    if( cSpan == 0 ) {
+        //mark it to be inserted in the next column
+        if( cCol >= nCols ) {
+            table()->appendColumn( cCol - nCols );
+            nCols = columns.size();
+        }
+        int finalSpan = 0;
+        for( int i = cCol; i < nCols; ++i) {
+            if ( !cellAt( cRow, cCol ) ) {
+                cellAt( cRow, cCol ) = cell;
+            }
+            ++finalSpan;
+        }
+        cellsWithColSpanZero.insertMulti( table()->effColToCol(cCol) + finalSpan, cell );
+        cell->setColSpan( finalSpan );
+    }
+
+    if( rSpan == 0 ) {
+        if( cCol >= nCols ) {
+            table()->appendColumn( cCol - nCols );
+            nCols = columns.size();
+        }
+        //mark is as inserted in next row
+        cellsWithRowSpanZero.insertMulti( cRow + 1, cell );
+        if ( !cellAt( cRow, cCol ) ) {
+            cellAt( cRow, cCol ) = cell;
+        }
+        cell->setRowSpan( 1 );
+        kDebug()<<"Adding rowspan0";
+    }
+
+     if( rSpan ) {
+        while ( cSpan ) {
+            int currentSpan;
+            if ( cCol >= nCols ) {
+                table()->appendColumn( cSpan );
+                currentSpan = cSpan;
+            } else {
+                if ( cSpan < columns[cCol].span ) {
+                    table()->splitColumn( cCol, cSpan );
+                }
+                currentSpan = columns[cCol].span;
+            }
+            int r = 0;
+            if( columnsToAvoid.contains(cCol) ) ++cCol;
+            while ( r < rSpan ) {
+                if ( !cellAt( cRow + r, cCol ) ) {
+    // qDebug("    adding cell at %d, %d",  cRow + r, cCol );
+                    cellAt( cRow + r, cCol ) = set;
+                }
+                ++r;
+            }
+            ++cCol;
+            cSpan -= currentSpan;
+            set = (RenderTableCell *)-1;
+        }
+     }
+
     if ( cell ) {
- cell->setRow( cRow );
- cell->setCol( table()->effColToCol( col ) );
+        cell->setRow( cRow );
+        cell->setCol( table()->effColToCol( col ) );
     }
 }
 
-
-
 void RenderTableSection::setCellWidths()
 {
 #ifdef DEBUG_LAYOUT
@@ -1781,7 +1852,7 @@
  if ( !cell || cell == (RenderTableCell *)-1 || nextrow && (*nextrow)[c] == cell )
     continue;
                 RenderObject* rowr = cell->parent();
-                int rtx = tx+rowr->xPos();
+                int rtx = tx+rowr->xPos();
                 int rty = ty+rowr->yPos();
 #ifdef TABLE_PRINT
  kDebug( 6040 ) << "painting cell " << r << "/" << c;
@@ -1826,17 +1897,17 @@
 int RenderTableSection::numColumns() const
 {
     int result = 0;
-    
+
     for (int r = 0; r < numRows(); ++r) {
         for (int c = result; c < table()->numEffCols(); ++c) {
             if (cellAt(r, c))
                 result = c;
         }
     }
-    
+
     return result + 1;
 }
-
+
 void RenderTableSection::recalcCells()
 {
     cCol = 0;
@@ -1854,6 +1925,7 @@
             for (RenderObject *cell = row->firstChild(); cell; cell = cell->nextSibling())
                 if (cell->isTableCell())
                     addCell( static_cast<RenderTableCell *>(cell), static_cast<RenderTableRow *>(row) );
+
  }
     }
     needCellRecalc = false;
@@ -2563,7 +2635,7 @@
     RenderTableCol* colElt = table()->colElement(col() + (rtl ? colSpan() - 1 : 0), &startColEdge, &endColEdge);
     if (colElt && (!rtl ? startColEdge : endColEdge)) {
         result = compareBorders(result, CollapsedBorderValue(&colElt->style()->borderLeft(), BCOL));
-        if (!result.exists())
+        if (!result.exists())
             return result;
         if (colElt->parent()->isTableCol() && (!rtl ? !colElt->previousSibling() : !colElt->nextSibling())) {
             result = compareBorders(result, CollapsedBorderValue(&colElt->parent()->style()->borderLeft(), BCOLGROUP));
Index: rendering/render_table.h
===================================================================
--- rendering/render_table.h (revisión: 948444)
+++ rendering/render_table.h (copia de trabajo)
@@ -273,6 +273,11 @@
 
     int numColumns() const;
 
+    //QMap< nextColumnToInsert, cell >
+    QMap< int, RenderTableCell* > cellsWithColSpanZero;
+    //QMap< column, cell >
+    QMap< int, RenderTableCell* > cellsWithRowSpanZero;
+
     signed short cRow;
     ushort cCol;
     bool needCellRecalc;

---