KTimer, Patch for wish 195647

[Eike Krumbacher]

Hi Again!


Raphael Kubo da Costa schrieb:
> The patch looks quite big. I'd rather have at least two separate ones
-- one
> for the tabs/spaces stuff, and another one implementing the new
functionality.
>  

OK, removed the Tab/Saces thing as good as I can.


> Other points:
>
> * ktimer.h: QString formatTime(const int seconds) const;
>
> The parameter does not need to be const, as it is a primitive type.
> Furthermore, it should have spaces between the brackets and the
parameters
>  

done

> * ktimer.cpp:
>
> You seem to do the same arithmetic for hours, minutes and seconds in
at least
> three different places in the file. Isn't it better to have a QTime
object in
> the class for that?
>  

QTime does not work for me, as I wanted to have 99 hours, which seems to
be a good mixture between the requested day and not providing a
day-QSpinBox. QTime is limited to 24 hours.

> You don't use the int parameter in changeDelay -- you can simply
remove it and
> connect changeDelay() to a SIGNAL(valueChanged()). And since it is a
slot,
> it'd be good to follow the naming convention and call it
delayChanged() or
> something like that.
>  
I haven't found a QSpinBox::valueChanged() Signal without parameter at
http://doc.trolltech.com/4.6/qspinbox.html


Thank you for reviewing my patches :-)


Eike



-------------- next part --------------
A non-text attachment was scrubbed...
Name: ktimer_ek2_195647.diff.gz
Type: application/gzip
Size: 3517 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kde-utils-devel/attachments/20100416/07fc76ae/attachment.bin