two patches

[Dirk Hohndel]

On Fri, Dec 12, 2014 at 03:53:28PM -0200, Tomaz Canabrava wrote:
> From cc64959903353c1d1cb4b182d50246d7fd1cd701 Mon Sep 17 00:00:00 2001
> From: Tomaz Canabrava <tomaz.canabrava at intel.com>
> Date: Fri, 12 Dec 2014 14:06:04 -0200
> Subject: [PATCH 2/2] Fixes the Bookmark Add/Rename/Remove witout replotting
>  everything.
> 
> This seems to work. I'v reworked the Event positioning code so
> we don't need to recreate the full DisplayedDive when a new
> bookmark is added, just figure out where it should be added
> and position it at the right spot.
> 
> This is a series of patches to start using less replots
> on the Profile.

So I'm puzzled how often we are adding bookmark events and how that
affects performance... or is this really just a first of a series and the
others are more performance relevant?

> diff --git a/dive.c b/dive.c
> index b318c4b..4fb6717 100644
> --- a/dive.c
> +++ b/dive.c
> @@ -150,6 +150,18 @@ void update_event_name(struct dive *d, struct event *event, char *name)
>  	free(remove);
>  }
>  
> +struct event *get_event_by_time(struct dive *d, uint32_t time){
> +	if (!d)
> +		return NULL;
> +	struct divecomputer *dc = get_dive_dc(d, dc_number);
> +	if (!dc)
> +		return NULL;
> +	struct event **events = &dc->events;
> +	while((*events)->next && (*events)->time.seconds != time)
> +		events = &(*events)->next;
> +	return (*events);
> +}

Dear C expert... I am curious. Why are you doing this with a struct event **
instead of just a struct event *... you need the ** if you want to be able
to modify the list in an elegant way, but all you do here is walk the
list, so this could drop one level of indirection... or am I missing
something?

> diff --git a/qt-ui/profile/diveeventitem.cpp b/qt-ui/profile/diveeventitem.cpp
> index a9c3c3f..14cb0a3 100644
> --- a/qt-ui/profile/diveeventitem.cpp
> +++ b/qt-ui/profile/diveeventitem.cpp
> @@ -9,6 +9,7 @@
>  #include <QDebug>
>  #include "gettextfromc.h"
>  #include "metrics.h"
> +#include <profile.h>
>  
>  extern struct ev_select *ev_namelist;
>  extern int evn_used;
> @@ -152,18 +153,27 @@ bool DiveEventItem::shouldBeHidden()
>  
>  void DiveEventItem::recalculatePos(bool instant)
>  {
> -	if (!vAxis || !hAxis || !internalEvent || !dataModel)
> +	if (!vAxis || !hAxis || !internalEvent || !dataModel){

WHITESPACE
>  		return;
> +	}

And why add the curly braces, anyway?

> -	QModelIndexList result = dataModel->match(dataModel->index(0, DivePlotDataModel::TIME), Qt::DisplayRole, internalEvent->time.seconds);
> -	if (result.isEmpty()) {
> -		Q_ASSERT("can't find a spot in the dataModel");
> -		hide();
> -		return;
> +	// find the correct time or interpolate between two points.
> +	int depth = -1;
> +	plot_data *entry = dataModel->data().entry;
> +	for(int i = 0; i < dataModel->data().nr; i++){
> +		if (entry[i].sec == internalEvent->time.seconds){
> +			depth = entry[i].depth;
> +			break;
> +		} else if (entry[i].sec > internalEvent->time.seconds) {
> +			int min = entry[i-1].depth;

This could do something really bad if for some reason
internalEvent->time.seconds is negative, right? then entry[0].sec (which
should always be 0) could already be bigger and we access element entry[-1]

> +			int max = entry[i].depth;
> +			depth = min + ((max - min)/2);
> +			break;
> +		}
>  	}
> +
>  	if (!isVisible() && !shouldBeHidden())
>  		show();
> -	int depth = dataModel->data(dataModel->index(result.first().row(), DivePlotDataModel::DEPTH)).toInt();
>  	qreal x = hAxis->posAtValue(internalEvent->time.seconds);
>  	qreal y = vAxis->posAtValue(depth);
>  	if (!instant)
> @@ -172,4 +182,5 @@ void DiveEventItem::recalculatePos(bool instant)
>  		setPos(x, y);
>  	if (isVisible() && shouldBeHidden())
>  		hide();
> +	qDebug() << pos();

Grrrrrrrmbl

> diff --git a/qt-ui/profile/profilewidget2.cpp b/qt-ui/profile/profilewidget2.cpp
> index 1970561..1181fc5 100644
> --- a/qt-ui/profile/profilewidget2.cpp
> +++ b/qt-ui/profile/profilewidget2.cpp
> @@ -1277,8 +1277,11 @@ void ProfileWidget2::removeEvent()
>  								  tr("%1 @ %2:%3").arg(event->name).arg(event->time.seconds / 60).arg(event->time.seconds % 60, 2, 10, QChar('0'))),
>  				  QMessageBox::Ok | QMessageBox::Cancel) == QMessageBox::Ok) {
>  		remove_event(event);
> +		copy_events(&current_dive->dc, &displayed_dive.dc);

this leaks memory, the events in displayed_dive are overwritten without
being freed. Call free_events() on the existing events in displayed_dive

>  		mark_divelist_changed(true);
> -		replot();
> +		scene()->removeItem(item);
> +		eventItems.removeOne(item);
> +		item->deleteLater();

You are the expert on that code :-)

> @@ -1287,8 +1290,20 @@ void ProfileWidget2::addBookmark()
>  	QAction *action = qobject_cast<QAction *>(sender());
>  	QPointF scenePos = mapToScene(mapFromGlobal(action->data().toPoint()));
>  	add_event(current_dc, timeAxis->valueAt(scenePos), SAMPLE_EVENT_BOOKMARK, 0, 0, "bookmark");
> +	copy_events(&current_dive->dc, &displayed_dive.dc);

See above - leaks memory

> +	struct event *ev = get_event_by_time(&displayed_dive, timeAxis->valueAt(scenePos));
> +	qDebug() << "time = " << timeAxis->valueAt(scenePos) << "Event time: " << ev->time.seconds;

Grrrrrmbl

>  	mark_divelist_changed(true);
> -	replot();
> +	DiveEventItem *item = new DiveEventItem();
> +	item->setHorizontalAxis(timeAxis);
> +	item->setVerticalAxis(profileYAxis);
> +	item->setModel(dataModel);
> +	item->setEvent(ev);
> +	item->setZValue(2);
> +	item->recalculatePos(true);
> +	item->setVisible(true);
> +	eventItems.push_back(item);
> +	scene()->addItem(item);
>  }
>  
>  void ProfileWidget2::addSetpointChange()
> @@ -1377,10 +1392,12 @@ void ProfileWidget2::editName()
>  		// order is important! first update the current dive (by matching the unchanged event),
>  		// then update the displayed dive (as event is part of the events on displayed dive
>  		// and will be freed as part of changing the name!
> +		int time = event->time.seconds;
>  		update_event_name(current_dive, event, newName.toUtf8().data());
>  		update_event_name(&displayed_dive, event, newName.toUtf8().data());
> +		struct event *ev = get_event_by_time(&displayed_dive, time);
>  		mark_divelist_changed(true);
> -		replot();
> +		item->setEvent(ev);
>  	}
>  }


So this is pretty invasive and the patch has a few issues.

Why should this go in before 4.3? Is there a specific bug this addresses?

Just trying to understand the what and the why... if this should go in,
can I get a cleaned up patch (I have taken the other one).

/D