Review Request: Achievements 2: LongLongPropertyWidgetItem

[Laszlo Papp]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101908/#review4576
-----------------------------------------------------------



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h
<http://git.reviewboard.kde.org/r/101908/#comment3969>

    #include <creator/lib/widgets/propertywidgetitem.h> ?
    
    My concern is that any third-party application code on the top of a library can use the <...> schema since a library and application are not obligatory to have relative path connection in this sense.
    
    Also, most of the other propertywidgetitems do the same way.



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h
<http://git.reviewboard.kde.org/r/101908/#comment3970>

    Other items do not use virtual destructor either since it is not a library. It does not hurt, but consistency is always good and if it needs any change everything can be done in one step without in-between mixed state.



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h
<http://git.reviewboard.kde.org/r/101908/#comment3973>

    stringValueChanged according to the naming schema (*ValueChanged) in other files ? 



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp
<http://git.reviewboard.kde.org/r/101908/#comment3971>

    You do not need to change it since other classes use this way, but it is funny. :)
    
    parent is not abbreviated, flags is.



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp
<http://git.reviewboard.kde.org/r/101908/#comment3972>

    Uhhh...Otherwise might know my opinion about it :)
    
    1) It is really not a correct regexp for this case.
    0-255 would be: ([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-5][0-5])
    You can imagine the complexity of 0-18446744073709551616 :D
    Regexp has it's narrow use cases tho, but in general it's just more advisable to parse instead of regexp. I tried to make things smartly previously, but also my experience and others from #qt channel (w00t, thiago, etc) advised that to me the right decision for me is to avoid regexps as much as possible.
    
    2) Why not QDoubleValidator with decimal 0 property is the default for instance in case of the setRange method ?
    
    



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp
<http://git.reviewboard.kde.org/r/101908/#comment3974>

    stringValueChanged according to the header file ?



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp
<http://git.reviewboard.kde.org/r/101908/#comment3975>

    "When the value is out of the limit, it is set to 0." -> This is not a good idea in general. It is usually better to set to the maximum value.
    
    However in this case you do not need the if part of the snippet since the toLongLong method returns 0 if the conversion failed. (In theory with proper range settings, it cannot even fail :)
    
    Just leave out the if condition check.
    
    


- Laszlo


On July 10, 2011, 3:58 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101908/
> -----------------------------------------------------------
> 
> (Updated July 10, 2011, 3:58 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> Add a PropertyWidgetItem to edit qlonglong values in the creator. Needed by the Achievement class.
> 
> 
> Diffs
> -----
> 
>   creator/plugins/docks/propertiesdock/CMakeLists.txt 05c5242 
>   creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h PRE-CREATION 
>   creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101908/diff
> 
> 
> Testing
> -------
> 
> Tested with the Achievement and Statistic class. When the value is out of the limit, it is set to 0.
> 
> 
> Thanks,
> 
> Felix
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/gluon/attachments/20110710/f5cc4d6d/attachment-0001.htm