Dive Merging Issues

[Linus Torvalds]

On Sat, Nov 16, 2013 at 2:22 PM, Tomaz Canabrava <tcanabrava at kde.org> wrote:
>
> I'v found one issue while trying to work on the list, to test if it was
> broke or anything, and I got stuck on it all day long.  But it's like this:
>
> open subsurface and load files dives/test23.xml && dives/test27.xml
> merge dive 22 with dive 28
>
> subsurface freezes.
> I'v tracked it down to 'scene()->itemsBoundingRect()' taking too long,
> and usually this means that we inserted a *lot* of crap in there,
>
> some time later, it came back to life, and I had a dive with more than 11000
> minutes.
> I'm attaching a screenshoot:
>
> So, what should I do on that regard? Dirk, Linus?

The attached patch - or something very much like it - is likely the
right thing to do.

It limits merging dives to dives that have at most half an hour of
surface time between them. That "half hour" is kind of a random thing
to pick, but it's not horribly horribly wrong.

It also changes the semantics of "merge selected dives" to something
that actually works pretty well: you can select a whole range of
dives, and it will merge only the ones that makes sense to merge. I
tested it, and it's reasonable. I could select all my dives from one
dive trip, and then do "Merge selected dives", and it did the right
thing (Dirk: I selected the florida trip, and it merged the aborted
"missed the trench" dive with the _actual_ "trench" dive).

I'm _slightly_ hesitant about this in the sense that maybe some crazy
person actually would want to merge dives with more than half an hour
of surface time between them, but it really doesn't seem to make much
sense.

So with that slight caveat:

  Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>

and it obviously fixes your test-case (by making "Merge selected
dives" just not do anything for it).

Comments?

                    Linus
-------------- next part --------------
 qt-ui/divelistview.cpp | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/qt-ui/divelistview.cpp b/qt-ui/divelistview.cpp
index 7689f06a9bec..e1083f7eaef6 100644
--- a/qt-ui/divelistview.cpp
+++ b/qt-ui/divelistview.cpp
@@ -408,6 +408,18 @@ void DiveListView::selectionChanged(const QItemSelection& selected, const QItemS
 	Q_EMIT currentDiveChanged(selected_dive);
 }
 
+static bool can_merge(const struct dive *a, const struct dive *b)
+{
+	if (!a || !b)
+		return false;
+	if (a->when > b->when)
+		return false;
+	/* Don't merge dives if there's more than half an hour between them */
+	if (a->when + a->duration.seconds + 30*60 < b->when)
+		return false;
+	return true;
+}
+
 void DiveListView::mergeDives()
 {
 	int i;
@@ -415,7 +427,7 @@ void DiveListView::mergeDives()
 
 	for_each_dive(i, dive) {
 		if (dive->selected) {
-			if (!maindive) {
+			if (!can_merge(maindive, dive)) {
 				maindive = dive;
 			} else {
 				maindive = merge_two_dives(maindive, dive);