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

Revision history for this message
Albert Astals Cid (aacid) wrote :

> > > -------------------------------
> > >
> > > 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))
> >
> > You're right, needs to be floor and not ceil (as i was using it already in
> the
> > other part of the code).
> >
> > The formula is simple, think the other way around, if you only have 1
> column,
> > your "desired" with would be horizontalSpacing + columnWidth +
> > horizontalSpacing, if you have 2 columns, it'd be horizontalSpacing +
> > columnWidth + horizontalSpacing + columnWidth + horizontalSpacing, for 3
> > columns, horizontalSpacing + columnWidth + horizontalSpacing + columnWidth +
> > horizontalSpacing + columnWidth + horizontalSpacing, so
> > desiredWidth=nColumns * (horizontalSpacing + columnWidth) +
> horizontalSpacing
> > i.e
> > desiredWidth - horizontalSpacing = nColumns * (horizontalSpacing +
> > columnWidth)
> > i.e.
> > (desiredWidth - horizontalSpacing) / (horizontalSpacing + columnWidth) =
> > nColumns
> > Which is what my formula is using.
> >
> > Clearer now?
>
> Absolutely, thanks.
>
> But I would prefer s/horizontalSpacing/columnSpacing because that margin can
> be done via other means, yielding more flexibility to the layout. You can use
> Item's native margin properties to achieve that spacing between the edge
> columns (leftmost and rightmost columns) and the item's borders.
>
> And also mimicking an existing API (Qt's Grid) has the added benefit of having
> the overall API more uniform and fit better together. A developer used to work
> with Grid will have a much lower learning curve if his existing API
> expectations can be directly transfered when working with VerticalJournal.
>
> Same applies for verticalSpacing property.

Done.

« Back to merge proposal