[PATCH] Remove useless members of DiveItem

[Dirk Hohndel]

On Thu, 2013-04-25 at 08:02 -0700, Thiago Macieira wrote:
> 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.

I'll be in the office in an hour (once I'm off this call I'll drive
out). Let's have coffee and I'll share some of the more brilliant
features of the Gtk TreeViewModel disaster with you :-)

> 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.

I totally agree. My excuse here is that I didn't even know what this
code was supposed to be doing. I guess dive(NULL) makes this much
clearer. Which of course also reminded me of the fact that we don't
check for NULL when we dereference it later...

> 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?

It should return Utf8 at all times. And I thought you needed to tell
QString which encoding is 'incoming' :-/

> 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.

So is this a complicated way of saying "if it's utf8 we don't need a
conversion? I'm still not clear on how QString actually works...

> > > -	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™ :-)

Yeah. Death to references. Less code I don't instinctively
understand :-)

> 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?

Oh, so we need this after all? Yes, location is user input (or
downloaded from a web site, or - in rare cases like the Uemis Zurich -
downloaded from a dive computer. It's expected to be Utf8.

But now you have me even more confused then before... 

When exactly do I need the fromUtf8 thingy???

/D