Patch

[Dirk Hohndel]

On Sun, 2014-03-09 at 13:02 +0800, Rishubh Jain wrote:
> Hi Dirk and Tomaz
> I Have attachedd the patch. I guess this is at the satisfactory level
> both in terms of the commit message and the requirements.

We are making progress.

But there are still a few things left that need to be addressed

a) formatting of the commit message.

please make sure that your commit message is wrapped at 74 characters.
Your lines were much longer than that which means that I need to
reformat the message.
Yes, this is a subtlety, but as I said, this is about learning
something. In normal patches I grumble and just do it for you... we have
one contributor here who is super prolific and a great friend of mine
and after 600+ commits still forces me to reformat his commit messages
quite frequently. I blame myself for not pushing back harder on him in
the beginning :-)

b) 4 character indentation and other whitespace damage

this is a good way to make me grumpy. Again, to the beginning developer
this will feel ridiculous, but trust me, consistent indentation and
whitespace matters. A huge deal. Please look at the CodingStyle file for
details. It even contains settings for some editors.

Look at this:

		if(reply==QMessageBox::Discard){
		    event->accept();
		    QApplication::quit();
		}
		else if(reply==QMessageBox::Ok){
		    event->ignore();
		}

it should read

		if (reply==QMessageBox::Discard) {
			event->accept();
		   	QApplication::quit();
		} else if (reply==QMessageBox::Ok) {
			event->ignore();
		}

Pay attention to indentation and whitespace around '{', '}' and language
keywords like "if" and "else".

I really don't want to discourage students eager to work on the project.
But the goal here is for students to learn how to be successful working
in an open source project. And the two points a) and b) above matter a
lot.

c) let's talk about the code.

when the user picks 'Discard' you simply quit right there. If you look
at the flow of the function before your change that seems like the wrong
thing to do. The code should simply fall through and continue execution
of the function if the user picks 'Discard' - that way writeSettings()
is called. On the flip side, after calling event->ignore() you really
need to return from the function, otherwise the event->ignore() will be
canceled out by the later event->accept() in the function.

d) did you take a look at the suggested wording that I sent yesterday,
earlier in this thread? You did not take that into account.

So please resubmit.

/D