[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Toolbox layout reviewal
From:       "Thomas Zander" <zander () kde ! org>
Date:       2010-03-21 20:04:37
Message-ID: 20100321200437.19540.2541 () localhost
[Download RAW message or body]


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



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4085>

    that semicolon should be removed.



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4086>

    Its 'sepArator'



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4087>

    Please use qCeil (which returns an int)



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4088>

    Lots of spaces on empty line here, maybe you can remove them prior to committing. \
(reviewboard highlighted 3 so far)



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4089>

    height for the x ? If thats correct please add a comment why.



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4090>

    Same here, width += height looks weird.



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4091>

    Please leave the curly brace on the same line as the if()



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4092>

    ditto



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4093>

    one more ditto :)



/trunk/koffice/libs/main/KoToolBox.cpp
<http://reviewboard.kde.org/r/3243/#comment4094>

    due to the update(), could you add a line that checks if the set orientation is \
really different from the current one and if its the same just return?



/trunk/koffice/libs/main/KoToolBox_p.h
<http://reviewboard.kde.org/r/3243/#comment4095>

    Can you call these 'set*' ? Using 'update*' sounds like its not a setter.



/trunk/koffice/libs/widgets/KoDockWidgetTitleBar.h
<http://reviewboard.kde.org/r/3243/#comment4096>

    These 3 setters are added on a public class, for completeness sake please \
consider adding API docs and getters (isCollapsable / textVisible are easy)  The next \
method is harder to mirror to a getter maybe rename the 'ignore' method to create the \
pair; 'setTitleClippingAllowed / titleClippingAllowed'


- Thomas


On 2010-03-18 09:58:41, Cyrille Berger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3243/
> -----------------------------------------------------------
> 
> (Updated 2010-03-18 09:58:41)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> This patch is a revival of http://reviewboard.kde.org/r/782/ . The reason is that I \
> (and from the review comments as well as list thread \
> http://lists.kde.org/?t=124395874500007&r=1&w=2 some other agrees) the current \
> layout code lead to messy. The main objection to the previous patch was that it \
> took more space, this patch address the issue and put some groups on the same line \
> when possible. 
> I have put screenshots of both layout with different size in \
> http://cyrille.diwi.org/tmp/koffice/toolboxlayout/, the proposed patch leads to a \
> less messy toolbox, as well as use less space. 
> Now to move forward, I had like all people to clearly state which layout they \
> prefer. And also if anything works incorrectly with the new layout code, I am \
> willing to work on it to fix any issues. 
> (I would also want to allow one column toolbox like we have one line horizontal \
> toolbox, but I think it is Qt that prevent that to happen) 
> 
> Diffs
> -----
> 
> /trunk/koffice/libs/main/KoMainWindow.cpp 1102761 
> /trunk/koffice/libs/main/KoToolBox.cpp 1102761 
> /trunk/koffice/libs/main/KoToolBox_p.h 1102761 
> /trunk/koffice/libs/widgets/KoDockWidgetTitleBar.h 1102761 
> /trunk/koffice/libs/widgets/KoDockWidgetTitleBar.cpp 1102761 
> /trunk/koffice/libs/widgets/KoDockWidgetTitleBarButton.h 1102761 
> 
> Diff: http://reviewboard.kde.org/r/3243/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cyrille
> 
> 

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel


[prev in list] [next in list] [prev in thread] [next in thread]