Patch

[Rishubh Jain]

On Sunday, 9 March 2014 3:50 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
 

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


Hello Dirk,

I have attached two screenshots..one is of the code..I guess I have covered on all the indentation problems and also about that writesetting function, i was not sure what to do exactly so I would like to discuss that with you.

The second screenshot is of the subsurface when you click cancel..I have used your wordings exactly without any of my inputs.

I hope this message is not top posted

Thanking You
Rishubh
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140316/aa5a1ce2/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: codesnipet.png
Type: image/png
Size: 177476 bytes
Desc: not available
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140316/aa5a1ce2/attachment-0002.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: subsurfacesnippet.png
Type: image/png
Size: 215997 bytes
Desc: not available
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140316/aa5a1ce2/attachment-0003.png>