Re: Patch for when rowspan or colspan equals 0

On Lunes 06 Abril 2009 21:55:33 Germain Garand escribió: uncovered, though.

> > 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 ;).
>
> errm I'm getting Qt assert failures (index out of range) in QVector's []
> operator with all your rowspan testcases, so some logic is indeed wrong.
>
You were right at IRC, I missed the case when you already found the cell which
has a colspan = 0 and you want to insert it in a new column, if I'm correct it
just happens when the colspan=0 is in the first row.

> it sounds like this could all stem from:
>
> +    ensureRows( cRow + (rSpan)? rSpan : 1 );
>
> ? surely, you meant:
>
> +    ensureRows( cRow + (rSpan? rSpan : 1) );
>
Changed, though... is there a real difference? I didn't get any compiling
errors.

> - could you setup a boolean flag for when no zero rowspan/colspan has been
> encountered yet, and then skip entirely the map lookup logic and
> effColToCol calculation as an optimization?
>
Done.

> - you should clear your two maps on recalculation (up in recalcCells()
> probably)
>
Done. Though for it to work properly we must make sure that the previous cells
aren't somehow used, otherwise we would have a span different than 0 and we
would have a 'normal' cell instead of a special case cell.
--
Carlos



[tables.diff]

---

Index: html/html_tableimpl.cpp
===================================================================
--- html/html_tableimpl.cpp (revisión: 950904)
+++ 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)
Index: rendering/render_table.cpp
===================================================================
--- rendering/render_table.cpp (revisión: 950904)
+++ 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);
 }
@@ -1028,6 +1028,7 @@
 
 RenderTableSection::RenderTableSection(DOM::NodeImpl* node)
     : RenderBox(node)
+    , containsSpansZero(false)
 {
     // init RenderObject attributes
     setInline(false);   // our object is not Inline
@@ -1190,44 +1191,130 @@
  }
     }
 
+    //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
+    QList< int > columnsToAvoid;
+    if( containsSpansZero ) {
+        int actualCurrentCol = table()->effColToCol(cCol);
+        if( cellsWithColSpanZero.contains( actualCurrentCol ) ) {
+            //No need to check if we have enough column, we already found the first cell
+            //(when colspan="0", and as such, we've already inserted it
+            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 );
+            }
+        }
+
+        if( cellsWithRowSpanZero.contains( cRow ) ) {
+            //make sure we have enough columns
+            if( cCol >= nCols ) {
+                table()->appendColumn( cCol - nCols );
+                nCols = columns.size();
+            }
+            while( RenderTableCell* cell = cellsWithRowSpanZero.take( cRow ) ) {
+                int cellCol = cell->col();
+                if( !cellAt( cRow, cellCol ) ) {
+                    cellAt( cRow, cellCol ) = cell;
+                }
+                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 );
+        containsSpansZero = true;
+    }
+
+    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 );
+        containsSpansZero = true;
+    }
+
+     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 +1868,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,23 +1913,25 @@
 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;
     cRow = -1;
     clearGrid();
     grid.resize( 0 );
+    cellsWithColSpanZero.clear();
+    cellsWithRowSpanZero.clear();
 
     for (RenderObject *row = firstChild(); row; row = row->nextSibling()) {
         if (row->isTableRow())  {
@@ -1854,6 +1943,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 +2653,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: 950904)
+++ rendering/render_table.h (copia de trabajo)
@@ -273,6 +273,12 @@
 
     int numColumns() const;
 
+    //QMap< nextColumnToInsert, cell >
+    QMap< int, RenderTableCell* > cellsWithColSpanZero;
+    //QMap< nextRowToInsert, cell >
+    QMap< int, RenderTableCell* > cellsWithRowSpanZero;
+    bool containsSpansZero;
+
     signed short cRow;
     ushort cCol;
     bool needCellRecalc;

---