[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