Divecomputer nickname handling..

[Dirk Hohndel]

On Thu, Sep 17, 2015 at 02:38:20PM -0700, Linus Torvalds wrote:
> 
> Ok, this is almost certainly entirely broken, because all I tested was 
> that it compiles for me. I'm not a huge fan of C++ ("really? Please don't 
> tell us again"),

Wait, you DON'T like C++? Tell me more...

> and I hate how I can't split up member functions into 
> static helper functions, so I got - once again - disgusted when I had to 
> add a "showchanges" member for no actual good reason just because I wanted 
> to split things up and make this more readable.

You can. I found out that you can indeed.

In a C++ file you can simply add a static function that is not part of the
class. It doesn't have access to class members - so it behaves just like a
standard C static function. I've use this somewhere already - forgot where
now. But yes, you absolutely can do that.

> I also didn't want to change the prototype fro the "AddDC" member, but I 
> wanted to change the use of the variables from the "constant pass by 
> reference" to actual local variables that I can change, so I did that 
> insane dance with variables there too.
> 
> In other words, you should not apply this. You should probably go "Linus 
> is insane, and this is not good, but the *right* way to do it is Xyz", and 
> just do Xyz instead.
> 
> For example, another alternative that I considered (and maybe should have 
> done) was to just create member functions to update the serial number, the 
> firmware version and the nickname. And then AddDC() would just do 
> something like
> 
>  (a) look up old entry with matching model/deviceid information.
>  (b) if no old entry exists, create a new entry with empty information 
>      and insert that
>  (c) just call the update functions for the passed-in firmware/serial/ 
>      nickname.
> 
> that would have been closer to what we used to do.

Yes, I think I would like that logic. It breaks things down to a
granularity that should be fairly easy to test. Because that's what I
want. I want TESTS that make sure we don't break things all the time any
more.

> This patch seemed closer to the new model, but tries to just make sure 
> that if we pass in empty firmware/serial/nickname information, we take the 
> values from whatever old entry we find instead.
> 
> And I *really* didn't test this. Yeah, I created a nickname for my EON, 
> and then I tried to download, and the nickname stayed around, but I didn't 
> actually test that that failed before, so the "test" is fundamentally 
> flawed crap.
> 
> Also, to be honest, I did a "force download all dives", and then clicked 
> "ok", and now I have a couple of dives that have *two* EON Steel cases, 
> apparently because we merged things and didn't notice we already had the 
> first. That seems to be a separate issue entirely, though - the ones that 
> merged correctly had the original EON Steel import as the first one.
> 
> I'll have to think more about that merging issue, but I'm pretty sure it 
> has nothing to do with this patch.

Merging is causing problems here and there. Again something that we don't
test enough. But a "force all" should merge every dive, shouldn't it?
Unless you changed dive times in your dive log or something...

> Anyway, despite the crap testing, this *looks* like it might work. That 
> must count for something? I mean, it did actually compile, so it must 
> be perfect ...

That's better than some of the things that I pushed upstream. Err, I
mean... really, Linus, you can do better than that, right?


> Not-Yet-Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> ---
> 
> You can remove the "Not-Yet-" part of my signoff if you actually review 
> this.  Or just consider it a failed attempt if you want to do the other 
> approach.

Review below. But I was hoping to motivate you to rewrite this with the
more granular API and utilizing cute little helper functions :-)

> diff --git a/divecomputer.cpp b/divecomputer.cpp
> index b45b74a449b9..a487578e7144 100644
> --- a/divecomputer.cpp
> +++ b/divecomputer.cpp
> @@ -58,26 +58,43 @@ const DiveComputerNode *DiveComputerList::get(const QString &m)
>  	return NULL;
>  }
>  
> -void DiveComputerList::addDC(const QString &m, uint32_t d, const QString &n, const QString &s, const QString &f)
> +void DiveComputerNode::showchanges(const QString &n, const QString &s, const QString &f) const
> +{
> +	if (nickName != n)
> +		qDebug("new nickname %s for DC model %s deviceId 0x%x", n.toUtf8().data(), model.toUtf8().data(), deviceId);
> +	if (serialNumber != s)
> +		qDebug("new serial number %s for DC model %s deviceId 0x%x", s.toUtf8().data(), model.toUtf8().data(), deviceId);
> +	if (firmware != f)
> +		qDebug("new firmware version %s for DC model %s deviceId 0x%x", f.toUtf8().data(), model.toUtf8().data(), deviceId);
> +}

So this does nothing when built in Release mode... which makes me wonder
why we would call it unconditionally below. But then again, we call so
much overhead code, it won't make a huge difference. And it's definitely a
useful level of debug information.

> +
> +void DiveComputerList::addDC(const QString &m, uint32_t d, const QString &_n, const QString &_s, const QString &_f)
>  {
>  	if (m.isEmpty() || d == 0)
>  		return;
> -	const DiveComputerNode *existNode = this->getExact(m, d);
> -	DiveComputerNode newNode(m, d, s, f, n);
> -	if (existNode) {
> -		if (newNode.changesValues(*existNode)) {
> -			if (n.size() && existNode->nickName != n)
> -				qDebug("new nickname %s for DC model %s deviceId 0x%x", n.toUtf8().data(), m.toUtf8().data(), d);
> -			if (f.size() && existNode->firmware != f)
> -				qDebug("new firmware version %s for DC model %s deviceId 0x%x", f.toUtf8().data(), m.toUtf8().data(), d);
> -			if (s.size() && existNode->serialNumber != s)
> -				qDebug("new serial number %s for DC model %s deviceId 0x%x", s.toUtf8().data(), m.toUtf8().data(), d);
> -		} else {
> +	const DiveComputerNode *Old = this->getExact(m, d);

while the existing code also used "this", it's not really needed and we
are trying to get rid of this pattern in the code.

Also, variables should start with a lower case letter... so 'old'.

> +	QString n = _n, s = _s, f = _f;

So you pass them in as reference but then copy them inside the function to
local variables. why?

> +	if (Old) {
> +		// Update any non-existent fields from the old entry
> +		if (!n.size())

I think n.isEmpty() is an easier to read way to implement this

> +			n = Old->nickName;
> +		if (!s.size())
> +			s = Old->serialNumber;
> +		if (!f.size())
> +			f = Old->firmware;
> +
> +		// Do all the old values match?
> +		if (n == Old->nickName && s == Old->serialNumber && f == Old->firmware)

Maybe you could turn "showChanges()" into a function that returns bool. It
shows the changes when compiled for debugging, but either way it returns
true it things are changed... so maybe isDifferent(...)

>  			return;
> -		}
> -		dcMap.remove(m, *existNode);
> +
> +		// debugging: show changes
> +		Old->showchanges(n, s, f);
> +		dcMap.remove(m, *Old);
>  	}
> -	dcMap.insert(m, newNode);
> +
> +	DiveComputerNode New(m, d, s, f, n);

see above, variables start with lower case letters and let's not use 'new'
as variable name... newNode wasn't entirely stupid. So maybe use oldNode
above instead of Old

> +	dcMap.insert(m, New);
>  }

The logic seems sound to me. I'm not sure why we remove the old node and
then add a new node instead of updating the old node, but we did the same
thing in the existing code, so maybe there's a reason to do so.

So conceptually I like what you said above about "if it exists, update,
otherwise add" better...

Thanks for working on this, Linus.

/D