Patch for #847

[Matthew Vepritskiy]

> 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: dirk at hohndel.org
> To: matteofichtner at hotmail.com
> CC: neolit123 at gmail.com; subsurface at 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 <matteofichtner at 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
 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150318/300e4fce/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_for_ticket_#847
Type: application/octet-stream
Size: 1025 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150318/300e4fce/attachment.obj>