[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