krita junior job

[Adam]

Hi,
i agree with 1) and 2), but isn't defining two macros for two conversions  
kind of an overkill? Imo a comment would be sufficient.
(new patch appended)

adam

On Sun, 13 Dec 2009 22:23:49 +0100, Dmitry Kazakov <dimula73 at gmail.com>  
wrote:

> Hi, Adam!
> Good work!
>
> I think some fixes could make code even better:
>
> 1)
>
> -    if(autosaveDelay>0) {
> -        m_autosaveCheckBox->setChecked(true);
> -    }
> -    else m_autosaveCheckBox->setChecked(false);
>
> +m_autosaveCheckBox->setChecked(autosaveDelay>0);
>
> 2) You name the methods and a parameter in a config "AutoSave". Maybe  
> change
> it to "AutoSaveInterval"? It's more self-explanatory =)
>
> 3) And the last suggestion: divisions and multiplications in the code are
> not very self-describing.
> I mean this:
> + m_autosaveSpinBox->setValue(autosaveDelay/60);
>
> Maybe add to the .cpp-file:
> #define MIN_TO_SEC(min) ((min)*60)
> #define SEC_TO_MIN(min) ((min)/60)
>
> ... like we do in koffice/libs/kobase/KoUnit.h
>
>
>
> On Sun, Dec 13, 2009 at 9:35 PM, Adam <nospam at xibo.at> wrote:
>
>> Hi,
>> here we go. Please don't hesitate to criticise my code, I'd like to  
>> write
>> good code and that's only possible, if somebody tells me the flaws..
>>
>> adam
>>
>>
>> On Sun, 13 Dec 2009 14:53:15 +0100, Boudewijn Rempt <boud at valdyas.org>
>> wrote:
>>
>>  On Sunday 13 December 2009, Adam wrote:
>>>
>>>> hi..
>>>> i've done one of the junior jobs
>>>> (http://wiki.koffice.org/index.php?title=Junior_Jobs#Krita) for krita.
>>>> it's working now, but i don't know how to submit the solution. here is
>>>> the
>>>> bugtracker entry: http://bugs.kde.org/159045
>>>> it's the first time, i code for an opensource project, so i've no  
>>>> idea,
>>>> how things are organized, where to post the patch, which format and  
>>>> how
>>>> to
>>>> generate it..
>>>> can anybody help?
>>>>
>>>> adam
>>>>
>>>>
>>> Wonderful! If you do
>>>
>>> 'svn diff > 159045.patch'
>>>
>>> in the koffice source directory, you will get a file that contains just
>>> your
>>> changes. You can mail that to this mailing list. We will look at it, we
>>> might
>>> have a couple of remarks and things that might have to be changed (but
>>> we'll
>>> be really nice about it), and then we'll decide it can go in, and I  
>>> will
>>> make
>>> sure it goes into trunk. And your name goes into the About Box --  
>>> eternal
>>> fame!
>>>
>>> Then, if you've fixed something else, we'll do it the same way. If the
>>> second
>>> contribution goes in smoothly as well, and you intend to go on working  
>>> on
>>> Krita, I'm going to help you get a KDE svn account, and we'll be really
>>> glad
>>> with another contributor :-).
>>>
>>> It's a lot of fun if you read this mailing list, participate in
>>> discussions,
>>> join us on irc, #krita on irc.freenode.net or post in forums on
>>> forums.kde.org, as well.
>>>
>>>
>> --
>> Using Opera's revolutionary e-mail client: http://www.opera.com/mail/
>>
>> _______________________________________________
>> kimageshop mailing list
>> kimageshop at kde.org
>> https://mail.kde.org/mailman/listinfo/kimageshop
>>
>>
>
>


-- 
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 159045.patch
Type: application/octet-stream
Size: 7786 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kimageshop/attachments/20091214/05605873/attachment.dll