Patch for #847

[Dirk Hohndel]

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