[PATCH] Add dive list view to main window

Dirk Hohndel dirk at hohndel.org
Thu Apr 11 10:00:16 PDT 2013


"Lubomir I. Ivanov" <neolit123 at gmail.com> writes:
>
> hello amit, and thank you for this implementation and pushing the Qt port.

Hear! Hear!

(or was it "Here! Here!"? I can never remember)

> sorry to be too observant, but you could you revise usage of the
> following (coding style semantics follow):
> +        if( section == DIVE_NUMBER){
>
> and similar. spacing should be:
> +        if (section == DIVE_NUMBER) {

Yes, please


> from:
> +    DiveItem * itemForIndex(const QModelIndex &) const;
> to:
> +    DiveItem *itemForIndex(const QModelIndex &) const;
>
> perhaps sticking the * to function or variable name for consistency.

definitely. spaces on both sides if it is a binary operator
(multiplication), but connected to the element if it is unary
(pointer).

> something else is usage of C++ pointers:
> const QString &filename
> i would use C pointers here. other's opinions?

Ha... my C++ ignorance comes into play... what's the difference?

>> +DiveItem::DiveItem(int num, QString dt, float dur, float dep, QString loc, DiveItem *p):
>> +    m_number(num), m_dateTime(dt), m_duration(dur), m_depth(dep), m_location(loc), m_parent(p)
> in another thread i pointed out that m_name or m_Name for members is
> not preferable over mName. perhaps others could comment here as well.

We went a bit back and forth and then decided that members should follow
the Qt style of naming, i.e., C++ stuff goes the Qt naming (but
Subsurface whitespace and indentation), C stuff goes Subsurface way of
naming. And global variable names are based on "where they come from"
(boy, that vague definition is sure to bite me at some point).

> on this:
> +        DiveItem * item = static_cast<DiveItem*>(index.internalPointer());
> not sure on the final word on C++ casts as well, but at least we can
> try keeping the * with the following spacing:
> +        DiveItem *item = static_cast<DiveItem *>(index.internalPointer());

I hate seeing C++ casts. Why would I use them over standard C casts?

> pm this:
> +    switch( index.column())
> +    {
> we should rather have it as:
> +    switch(index.column()) {

Yep

> on this:
>> +    DiveItem *parent() const {return m_parent;}
> i think there should be spaces inside {}
>> +    DiveItem *parent() const { return m_parent; }

Also yes. And we should ONLY EVER have this "single line" format if
there is truly only one short instruction in there.... otherwise this
should be written like a regular, grown up function.

> i know that this is very annoying, but if we don't have a coding
> style, essentially more patches have to contribute later to SNR
> statistics until the codebase its cleaned up.

Yes, I completely agree with Lubomir. Let's establish a clear style
right from the beginning and all get used to it :-)

/D


More information about the subsurface mailing list