> 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? >
First of a series. > > > 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? > Copied from the code just above that one. > > > 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 > MEEH > > return; > > + } > > And why add the curly braces, anyway? > I was debugging and forgot to remove them. > > > - 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] > Is that possible? > > > + 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 > MEEEH > > > 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(¤t_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 > Oh, sorry. > > > 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(¤t_dive->dc, &displayed_dive.dc); > > See above - leaks memory > Oh sorry2 > > > + 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? > I was trying to find out why when moving a node for a few seconds while adding a new dive used 100% of the CPU in release mode. turned out it was because of too many replots that used the *very* expensive calls to PainterPath. while I was trying to fix that I got carried away. I think I'll just generate the pixmap of the Glyphs and reuse them in the future for the PainterPath issue. > 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 >
_______________________________________________ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface