Code review comment for lp:~aacid/unity8/verticalJournal

In VerticalJournal::setModel and VerticalJournal::setDelegate:

        if (!m_delegateModel) {
            [...]
        }
        [...]
        for (int i = 0; i < m_columnVisibleItems.count(); ++i)
        {
            [...]
        }

coding style: Let's be consistent with the placement of {}.

Seeing that you have the same code snippet in two functions makes me think that you could at least have a cleanupExistingItems() helper function with that chunk of code:

        for (int i = 0; i < m_columnVisibleItems.count(); ++i) {
            QList<ViewItem> &column = m_columnVisibleItems[i];
            Q_FOREACH(const ViewItem &item, column)
                releaseItem(item);
            column.clear();
        }
        m_indexColumnMap.clear();

---------------------------

"""
    // Check if we add it to the bottom of existing column items
    qreal columnToAddY = !m_columnVisibleItems[0].isEmpty() ? m_columnVisibleItems[0].last().y() + m_columnVisibleItems[0].last().height() : 0;
    int columnToAddTo = 0;
    for (int i = 1; i < m_columnVisibleItems.count(); ++i) {
        const qreal iY = !m_columnVisibleItems[i].isEmpty() ? m_columnVisibleItems[i].last().y() + m_columnVisibleItems[i].last().height() : 0;
        if (iY < columnToAddY) {
            columnToAddTo = i;
            columnToAddY = iY;
        }
    }
    if (m_columnVisibleItems[columnToAddTo].isEmpty() || m_columnVisibleItems[columnToAddTo].last().m_modelIndex < modelIndex) {
"""

Long lines ahoy!

----------------------------

Any particular reason why you're calling m_columnVisibleItems.resize(nColumns) in componentComplete() right before calling polish()? Because updatePolish() already does the very same thing. Looks like this is either a redundant leftover that should be removed or a workaround for some obscure behavior which should then be explained with a comment (for the unware developer reading this code, like myself).

----------------------------

"""
   * The first nColumns items are layouted at row 0 from column 0
   * to column nColumns-1 in order. After that every new item
   * is positioned in the column which provides the free topmost
   * position as possible. If more than one column tie in providing
   * the topmost free position the leftmost column will be used.
"""

Got some ASCII art for you!

+-----+ +-----+ +-----+
| | | 2 | | |
| | | | | |
| 1 | +-----+ | 3 |
| | +-----+ | |
| | | | +-----+
+-----+ | 4 | +-----+
+-----+ | | | 5 |
| 6 | +-----+ | |
| | +-----+
+-----+

----------------------------

void VerticalJournal::updatePolish()
{
[...]
    if (m_implicitHeightDirty) {
        [...]
    }
}

Could be refactored to

void VerticalJournal::updatePolish()
{
[...]
    if (m_implicitHeightDirty) {
        calculateImplicitHeight();
    }
}