[PATCH] Remove useless members of DiveItem

Thiago Macieira thiago at macieira.org
Thu Apr 25 08:02:50 PDT 2013


TL;DR: looks good, just one question about encoding amid the ramblings

On quinta-feira, 25 de abril de 2013 07.19.38, Dirk Hohndel wrote:
> From the way it looks Qt's way of handling this is orders of magnitude
> more sane - and at least from what I can see there is no need at all to
> keep anything more than a pointer to the dive in the model. I may be
> missing something but as I've said frequently in the past 12 hours... it
> compiles, therefore it must be perfect(tm) :-)

Hmm... the model-view framework on Qt is one of the API we're least proud 
of... It's considered overly complicated. It was rushed into Qt 4.0 when it 
was released. I don't know what that says about Gtk.

Anyway, what you say makes perfect sense: a model (as in a class derived from 
QAbstractItemModel) is supposed to be the view into your data, which exists 
outside of the model.

> > +	explicit DiveItem(): dive() { parentItem = 0; }

This is one of those things that are correct but could be better. So in the 
sake of readability I recommend you explicitly write:

	dive(0)

or dive(NULL), as I thought we had agreed to use NULL for pointers, not 
zeroes. I had to look at the class member to see if it was a pointer or not. 
Then I had to think for a second about what the "default constructor" for a 
pointer does -- yes, it does a zero-initialisation, but it leads to head-
scratching.

parentItem is also a pointer, so this can be:

	explicit DiveItem() : dive(NULL), parentItem(NULL) {}

> > +	const QString diveDateTime() const { return
> > QString::fromUtf8(get_dive_date_string(dive->when)); } 

This is the diveDateTime() from the other thread. Does anyone know how this 
could return badly encoded data?

Given the solution that was proposed there, I would say that the problem has 
been _fixed_. Can someone verify?

Here's why: the proposed workaround was to do fromUtf8(string.toLatin1()), 
which indicates that the string contained the opposite: UTF-8 data interpreted 
via fromLatin1(). Since this function is now doing fromUtf8(), it can't be 
wrong any more.


> > -	const QString& diveLocation() const { return location; }
> > -	const QString& diveSuit() const { return suit; }
> > +	const QString diveLocation() const { return dive->location; }
> > +	const QString diveSuit() const { return dive->suit; }

Removing the reference is TheRightThing™ :-)

Now you see exactly why Qt doesn't return references: because you can't change 
your internal fields without affecting your API.

What's the encoding for the dive location? Are we missing QString::fromUtf8() 
here?

> >  private:
> > -	int number;
> > -	timestamp_t when;
> > -	duration_t duration;
> > -	depth_t maxdepth;
> > -	int rating;
> > -	temperature_t temperature;
> > -	weight_t totalweight;
> > -	QString suit;
> > -	int sac;
> > -	int otu;
> > -	int maxcns;
> > -	QString location;
> > +	struct dive *dive;
> > 
> >  	DiveItem *parentItem;
> >  	QList <DiveItem*> childlist;

Yay! Struct reduced from 8 ints + 1 gint64 + 4 pointers to 3 pointers in size!

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20130425/9b047b82/attachment.sig>


More information about the subsurface mailing list