[PATCH 2/3] MainTab: fix missing update for 'amount_selected'

[Dirk Hohndel]

On Tue, 2013-12-10 at 05:40 +0100, Dirk Hohndel wrote:
> On Tue, 2013-12-10 at 01:11 +0200, Lubomir I. Ivanov wrote:
> > On 10 December 2013 01:10, Lubomir I. Ivanov <neolit123 at gmail.com> wrote:
> > > On 10 December 2013 01:07, Tomaz Canabrava <tcanabrava at kde.org> wrote:
> > >> I think we should look in the code for every appearance of 'd->selected =
> > >> true' and change it for void select_dive(int idx)
> > >> sounds a bit safer.
> > 
> > but if you wan't me to use select_dive() instead i will modify the patch.
> 
> Let's look at a little more context here:
> 
> 
> 	if(editMode == ADD || editMode == MANUALLY_ADDED_DIVE){
> 		mainWindow()->dive_list()->unselectDives();
> 		struct dive *d = get_dive(dive_table.nr -1 );
> 		// HACK. this shouldn't be here. but apparently it's
> 		// how we can know what was the newly added dive.
> 		d->selected = true;
> 		sort_table(&dive_table);
> 		int i = 0;
> 		for_each_dive(i,d){
> 			if (d->selected) break;
> 		}
> 		editMode = NONE;
> 		mainWindow()->refreshDisplay();
> 		mainWindow()->dive_list()->selectDive( i, true );
> 
> So d->selected is abused here to mark the new dive. We can neither keep
> the pointer nor the idx around to mark it - both can change as we sort
> the table. So instead we call unselectDives() and mark the one dive that
> we want to remember. And then we immediately afterwards walk the dive
> table and then use the UI function to select that dive. So once
> dive_list()->selectDive(i,true) has been called, everything should once
> again be hooked up correctly.
> 
> Now the first thing I thought was "umm, maybe we should reset
> d->selected to 0 here before calling selectDive(i,true), maybe that's
> what's confusing things... but then I read the code for selectDive and I
> wonder how this ever worked - as it DOESN'T keep our internal state and
> the UI state in sync at all. And selectDives equally makes me squirm as
> it sets the selected flags of the dives in newDiveSelection, but doesn't
> keep amount_selected in sync, either.

Should have kept looking before jumping to conclusions. I blame the lack
of coffee and the early hour.

Tomaz is of course MUCH smarter than this. The synchronization with our
internal structures is done in one place only - in the Qt callback that
the selection has changed.

And that's why my code above was broken and that's why Lubomir's fix
happens to fix it. We do indeed correctly call select_dive on the dive
that was freshly selected in selectDive(i,true). But select_dive then
has this very sensible piece of code in it:

		if (!dive->selected) {
			dive->selected = 1;
			amount_selected++;
		}

And since I stupidly didn't clear the ->selected flag that I abused to
remember the dive, THAT is why amount_selected gets out of sync.
I will fix this differently, making sure that if in the future we add
anything else to select_dive, we don't end up messing things up here.


/D