> Is this equivalent to the recursion that we had? I don't think so... You're right. I did not cope with the replacement of recursion. I'll try to fix later.
> Also, this is new behavior, should this be documented somewhere (like the > user manual)? Pardon. I was not aware that it is required. Regards, Matthew V. > Date: Wed, 18 Mar 2015 11:55:22 -0700 > From: d...@hohndel.org > To: matteoficht...@hotmail.com > CC: neolit...@gmail.com; subsurface@subsurface-divelog.org > Subject: Re: Patch for #847 > > On Wed, Mar 18, 2015 at 06:24:10PM +0000, Matthew Vepritskiy wrote: > > > The idea of the two patch rule is for you to demonstrate that you can > > > successfully contribute. > > I do not argue. I really want to be helpful. > > Cool > > > I made another patch. He corrects the error described in the ticket # 847. > > Plus, I have tried to remove the recursion from the function enableEdition > > (). > > Thanks. Please see my comments / questions below. > > /D > > > From 01220fed6cfc82645f7f38014e94b2f9b9accd60 Mon Sep 17 00:00:00 2001 > > From: Matvey Vepritskiy <matteoficht...@hotmail.com> > > Date: Wed, 18 Mar 2015 20:52:34 +0300 > > Subject: [PATCH] Fix for ticket #847 > > > > So where is the commit message that explains what this commit does? > > The title of the commit should be a brief description of the commit. > > "Allow editing profiles without editing a field" > > Then use > > Fixes #847 > > on its own line in the commit message to trigger the trac automation that > closes ticket #847 > > > diff --git a/qt-ui/maintab.cpp b/qt-ui/maintab.cpp > > index abf1382..39ffec1 100644 > > --- a/qt-ui/maintab.cpp > > +++ b/qt-ui/maintab.cpp > > @@ -285,19 +285,10 @@ void MainTab::enableEdition(EditMode newEditMode) > > modified = false; > > copyPaste = false; > > if ((newEditMode == DIVE || newEditMode == NONE) && > > - current_dive->dc.model && > > - strcmp(current_dive->dc.model, "manually added dive") == 0) { > > - // editCurrentDive will call enableEdition with newEditMode == > > MANUALLY_ADDED_DIVE > > - // so exit this function here after editCurrentDive() returns > > - > > - > > - > > - // FIXME : can we get rid of this recursive crap? > > - > > - > > - > > - MainWindow::instance()->editCurrentDive(); > > - return; > > + current_dive->dc.model && > > + strcmp(current_dive->dc.model, "manually added dive") == 0) { > > This is a whitespace change that I don't like - we try to lign up > continuation lines so it's easier to see the logic connection between > broken long lines... > > > + MainWindow::instance()->editCurrentDive(); > > + newEditMode = MANUALLY_ADDED_DIVE; > > Is this equivalent to the recursion that we had? I don't think so... > And why is this mixed into this commit? I can't tell without a commit > message if this is an integral part of the fix or if you just fixed this > "as well". > > > diff --git a/qt-ui/mainwindow.cpp b/qt-ui/mainwindow.cpp > > index b9e0f98..5d41b49 100644 > > --- a/qt-ui/mainwindow.cpp > > +++ b/qt-ui/mainwindow.cpp > > @@ -1467,19 +1467,16 @@ void MainWindow::editCurrentDive() > > struct dive *d = current_dive; > > QString defaultDC(d->dc.model); > > DivePlannerPointsModel::instance()->clear(); > > + disableShortcuts(); > > if (defaultDC == "manually added dive") { > > - disableShortcuts(); > > > > DivePlannerPointsModel::instance()->setPlanMode(DivePlannerPointsModel::ADD); > > graphics()->setAddState(); > > setApplicationState("EditDive"); > > DivePlannerPointsModel::instance()->loadFromDive(d); > > - information()->enableEdition(MainTab::MANUALLY_ADDED_DIVE); > > } else if (defaultDC == "planned dive") { > > - disableShortcuts(); > > > > DivePlannerPointsModel::instance()->setPlanMode(DivePlannerPointsModel::PLAN); > > setApplicationState("EditPlannedDive"); > > DivePlannerPointsModel::instance()->loadFromDive(d); > > - information()->enableEdition(MainTab::MANUALLY_ADDED_DIVE); > > } > > } > > So you move the disableShortcuts() before the if clause. Is that the right > thing to do? So far this was only called if the defaultDC was either > "manually added dive" or "planned dive". Why is it the right thing to > always call disableShortcuts()? > > And what about the enableEdition() call - why is that no longer needed? > I can see below that you added it to the mouseDoubleClickEvent()... but > again, without a solid commit message it is very hard for me from looking > at the patch to see if this is the right thing to do. > > > diff --git a/qt-ui/profile/profilewidget2.cpp > > b/qt-ui/profile/profilewidget2.cpp > > index 8e3641b..d9b9765 100644 > > --- a/qt-ui/profile/profilewidget2.cpp > > +++ b/qt-ui/profile/profilewidget2.cpp > > @@ -811,6 +811,8 @@ void ProfileWidget2::mouseDoubleClickEvent(QMouseEvent > > *event) > > int milimeters = rint(profileYAxis->valueAt(mappedPos) / > > M_OR_FT(1, 1)) * M_OR_FT(1, 1); > > plannerModel->addStop(milimeters, minutes * 60, 0, 0, true); > > } > > + if (currentState == PROFILE) > > + MainWindow::instance()->information()->enableEdition(); > > Would this be better as > } else if (currentState == PROFILE) { > ?? > > Also, this is new behavior, should this be documented somewhere (like the > user manual)? > > /D
fix_for_ticket_#847
Description: Binary data
_______________________________________________ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface