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

> Q_PROPERTY(qreal horizontalSpacing READ horizontalSpacing WRITE setHorizontalSpacing NOTIFY horizontalSpacingChanged)
> [...]
> Q_PROPERTY(qreal delegateCreationBegin READ delegateCreationBegin WRITE setDelegateCreationBegin NOTIFY delegateCreationBeginChanged RESET resetDelegateCreationBegin)
> Q_PROPERTY(qreal delegateCreationEnd READ delegateCreationEnd WRITE setDelegateCreationEnd NOTIFY delegateCreationEndChanged RESET resetDelegateCreationEnd)

"Each line of text in your code should be at most 120 characters long. "

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

For test targets, the convention we use is "testFoo", not "testfoo".

Therefore I think the test should be called "testVerticalJournal", not "testverticaljournal".

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

I see that the vertical journal is not used anywhere yet. So there's no place to play with it. Would it be too much to ask for a "make tryVerticalJournal" target that brings up an interactive manual test so one could play with the VerticalJournal? I envision a window with a vertical journal on its left side and some controls on its right side. One could use those controls to manually add an item with a given size and see how the vertical journal places it. And click on a placed item to remove it and see now the vertical journal reorganizes itself. Similar to what we have with "make tryResponsiveGridView" and "make tryStage".

What do you think?

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

In VerticalJournal::setModel(QAbstractItemModel *model):

"""
#if (QT_VERSION < QT_VERSION_CHECK(5, 1, 0))
        m_delegateModel->setModel(QVariant::fromValue<QAbstractItemModel *>(model));
#else
        m_delegateModel->setModel(QVariant::fromValue<QAbstractItemModel *>(model));
#endif
"""

Played "spot the difference" here and lost.

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

void VerticalJournal::createDelegateModel()
{
#if (QT_VERSION < QT_VERSION_CHECK(5, 1, 0))
    m_delegateModel = new QQuickVisualDataModel(qmlContext(this), this);
    connect(m_delegateModel, SIGNAL(createdItem(int,QQuickItem*)), this, SLOT(itemCreated(int,QQuickItem*)));
#else
    m_delegateModel = new QQmlDelegateModel(qmlContext(this), this);
    connect(m_delegateModel, SIGNAL(createdItem(int,QObject*)), this, SLOT(itemCreated(int,QObject*)));
#endif

Could we use the QObject::connect version that checks signal-slot compatibility at compile time (as opposed to runtime)? Since we're not checking the connect() results here, which can fail with this runtime-check version, we are making room for sneaky bugs to slip through.

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

    const int nColumns = ceil((double)(width() - m_horizontalSpacing) / (m_columnWidth + m_horizontalSpacing));

How have you come to this formula? My question also comes from the fact that I don't know what's your definition of horizontalSpacing, which should probably documented.

For instance, I don't know is there's horizontalSpacing to the left of the left-most column and to the right of the right-most column.
Or if the spacing applies exclusively *between* two columns.

I would help if you used the same nomenclature as the existing Grid component. i.e., s/horizontalSpacing/columnSpacing and s/verticalSpacing/rowSpacing.

So, considering horizontalSpacing has the same meaning as Grid's columnSpacing, I would expect this expression to be:

nColumns = floor((width+columnSpacing) / (columnWidth+columnSpacing))

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

PS: Still going through the code