@anonymous-piwik-user opened this Issue on January 28th 2010
The line "$this->rowsIndexByLabel[$label] = count($this->rows)-1;" in function

"public function addRow( Piwik_DataTable_Row $row )" doesn't work correctly.
For example:

$rows ( 1=>"AA", 2=>"BB", 3=>"CC", 4=>"DD", 5=>"EE", 6=>"FF");
note:1=>"AA": "1" is the index id of label "AA"

delete $rows[3], we get:

$rows ( 1=>"AA", 2=>"BB", 4=>"DD", 5=>"EE", 6=>"FF");

addRow($newrow),the label of $newrow is "GG"

The result should be:
$rows ( 1=>"AA", 2=>"BB", 4=>"DD", 5=>"EE", 6=>"FF", 7=>"GG");

while according to function addRow(), the index id of $newrow's label("GG") is

$this->rowsIndexByLabel[$label] = count($this->rows)-1 =5-1=4,

but the right index id of label "GG" is 7.

We find the function rebuildIndex() useless after we change the function addRow().

@anonymous-piwik-user commented on January 28th 2010

Attachment:
newdatatablebug.txt

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The patch file for DataTable.php
DataTable-diff.txt

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The file with change by jetuelle
DataTable.php

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The diff file of DataTable.php
DataTable.php.diff

@anonymous-piwik-user commented on February 22nd 2010

Attachment: The patch file for DataTable.php
DataTable.php.patch

@anonymous-piwik-user commented on January 28th 2010

The code change contains the following three points:

11original code(4 functions):
public function addRow( Piwik_DataTable_Row $row )
{
$this->rows[] = $row;

    if(!$this->indexNotUpToDate
        && $this->rebuildIndexContinuously)
    {
        $label = $row->getColumn('label');
        if($label !== false)
        {
            $this->rowsIndexByLabel[$label] = count($this->rows)-1;             
        }
        $this->indexNotUpToDate = false;
    }
}

static public function isEqual(Piwik_DataTable $table1, Piwik_DataTable $table2)
{
    $rows1 = $table1->getRows();
    $rows2 = $table2->getRows();

    $table1->rebuildIndex();
    $table2->rebuildIndex();

    if($table1->getRowsCount() != $table2->getRowsCount())
    {
        return false;
    }

    foreach($rows1 as $row1)
    {
        $row2 = $table2->getRowFromLabel($row1->getColumn('label'));
        if($row2 === false
            || !Piwik_DataTable_Row::isEqual($row1,$row2))
        {
            return false;
        }
    }       
    return true;
}

public function getRowFromLabel( $label )
{
    $this->rebuildIndexContinuously = true;
    if($this->indexNotUpToDate)
    {
        $this->rebuildIndex();
    }

    if($label === self::LABEL_SUMMARY_ROW
        && !is_null($this->summaryRow))
    {
        return $this->summaryRow;
    }

    $label = (string)$label;
    if(!isset($this->rowsIndexByLabel[$label]))
    {
        return false;
    }
    return $this->rows[$this->rowsIndexByLabel[$label]];
}

    public function sort( $functionCallback, $columnSortedBy )
{
    $this->indexNotUpToDate = true;
    $this->tableSortedBy = $columnSortedBy;
    usort( $this->rows, $functionCallback );

    if($this->enableRecursiveSort === true)
    {
        foreach($this->getRows() as $row)
        {
            if(($idSubtable = $row->getIdSubDataTable()) !== null)
            {
                $table = Piwik_DataTable_Manager::getInstance()->getTable($idSubtable);
                $table->enableRecursiveSort();
                $table->sort($functionCallback, $columnSortedBy);
            }
        }
    }
}

2changed code(4 functions):
public function addRow( Piwik_DataTable_Row $row )
{
    $this->rows[] = $row;

    $label = $row->getColumn('label');
    if($label !== false)
    {
        $this->rowsIndexByLabel[$label] = $this->nextRowId;
        $this->nextRowId++;
    }
}

static public function isEqual(Piwik_DataTable $table1, Piwik_DataTable $table2)
{
    $rows1 = $table1->getRows();
    $rows2 = $table2->getRows();

    if($table1->getRowsCount() != $table2->getRowsCount())
    {
        return false;
    }

    foreach($rows1 as $row1)
    {
        $row2 = $table2->getRowFromLabel($row1->getColumn('label'));
        if($row2 === false
            || !Piwik_DataTable_Row::isEqual($row1,$row2))
        {
            return false;
        }
    }       
    return true;
}

public function getRowFromLabel( $label )
{
    if($label === self::LABEL_SUMMARY_ROW
        && !is_null($this->summaryRow))
    {
        return $this->summaryRow;
    }

    $label = (string)$label;
    if(!isset($this->rowsIndexByLabel[$label]))
    {
        return false;
    }
    return $this->rows[$this->rowsIndexByLabel[$label]];
}

    public function sort( $functionCallback, $columnSortedBy )
{
    $this->tableSortedBy = $columnSortedBy;
    usort( $this->rows, $functionCallback );

    if($this->enableRecursiveSort === true)
    {
        foreach($this->getRows() as $row)
        {
            if(($idSubtable = $row->getIdSubDataTable()) !== null)
            {
                $table = Piwik_DataTable_Manager::getInstance()->getTable($idSubtable);
                $table->enableRecursiveSort();
                $table->sort($functionCallback, $columnSortedBy);
            }
        }
    }
}   

2added codes:
protected $nextRowId = 0;
3deleted code:
protected $indexNotUpToDate = true;
protected $rebuildIndexContinuously = false;
private function rebuildIndex()
{
foreach($this->rows as $id => $row)
{
$label = $row->getColumn('label');
if($label !== false)
{
$this->rowsIndexByLabel[$label] = $id;
}
}
$this->indexNotUpToDate = false;
}

@robocoder commented on January 28th 2010 Contributor

Can you upload this as a patch? (use "svn diff")

@mattab commented on January 28th 2010 Owner

Also if you can please submit a unit test to show that there is a bug somewhere? the unit test for the dataTable is: https://github.com/piwik/piwik/blob/master/tests/core/DataTable.test.php

@mattab commented on February 11th 2010 Owner

jetuelle, please post a patch, your code is very hard to read in its current form.

rebuildIndex is not useless, it is helpful to access quickly rows with a given label instead of looping through the hundreds of rows in the datatable.

@mattab commented on March 17th 2010 Owner

Marking as invalid. Using delete $rows[3] is not supported, you should use $dataTable->deleteRow( $idRow ); instead.

If you wish to prove a bug, please provide a new unit test to https://github.com/piwik/piwik/blob/master/tests/core/DataTable.test.php that would revel the bug. thx

This Issue was closed on March 19th 2010
Powered by GitHub Issue Mirror