From koffice-devel  Sun Mar 21 20:04:37 2010
From: "Thomas Zander" <zander () kde ! org>
Date: Sun, 21 Mar 2010 20:04:37 +0000
To: koffice-devel
Subject: Re: Review Request: Toolbox layout reviewal
Message-Id: <20100321200437.19540.2541 () localhost>
X-MARC-Message: https://marc.info/?l=koffice-devel&m=126920193103946


-----------------------------------------------------------
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