two patches

[Tomaz Canabrava]

> 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(&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
>

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(&current_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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20141212/55bb35ab/attachment.html>