Re: Review Request: Plasmoïd Notes: Solution for the bug 171795. Possibility to format the text.

[Aaron Seigo]

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



trunk/KDE/kdeplasma-addons/applets/notes/notes.h
<http://reviewboard.kde.org/r/567/#comment580>

    these members shouldn't be public (they should be private) and the should be prefixed with m_ like all the other members of the class.



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment577>

    ?



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment578>

    please arrange the #includes with the ones above, namely: sorted by Qt/KDE and alphabeticall. also, use the CamelCase includes, not the smallcase.h versions to be consistent with all the other #includes



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment579>

    this should be at the bottom of the file (convention)



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment573>

    this context menu is now huuuuuuuuuge. it needs some organization; consider perhaps putting all the formatting options into a i18n("Formatting") submenu



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment566>

    use KIconLoader; absolute paths to icons break with different install paths, KIconLoader finds them for you by name, e.g. KIcon("format-justify-fill")



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment567>

    i18n("Bold"), not "Gras" :)



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment568>

    missing i18n() around the user visible text



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment569>

    coding style:
    
    if () {
        foo;
    } else {
        bar;
    }
    
    note the {}s



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment570>

    if (font().bold()) ?



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment571>

    this already is the widget, you don't need to set it again.



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment572>

    why is this needed?



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment574>

    zero preferred size?



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment575>

    a feudal era french tax?



trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp
<http://reviewboard.kde.org/r/567/#comment576>

    this is going to break as soon as the widths change.
    
    rather, i'd suggest simply setting a minimum width on the note widget _or_ when it's below a certain width (e.g. (button->width() + spacing) * numberOfButtons) have the + icon pop up a menu showing the format actions


- Aaron


On 2009-04-12 04:35:57, Madlyn MERCK wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/567/
> -----------------------------------------------------------
> 
> (Updated 2009-04-12 04:35:57)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch offers the possibility to format the text of the note.
> It's a solution of the bug 171795 (https://bugs.kde.org/show_bug.cgi?id=171795).
> You can make your text bold, italic, strike-through... You can show format options with the button '+' or with the text's popupmenu.
> 
> If you notice any problem with this new option just tell me.
> 
> 
> This addresses bug 171795.
>     https://bugs.kde.org/show_bug.cgi?id=171795
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdeplasma-addons/applets/notes/notes.h 951942 
>   trunk/KDE/kdeplasma-addons/applets/notes/notes.cpp 951942 
> 
> Diff: http://reviewboard.kde.org/r/567/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Screenshot1
>   http://reviewboard.kde.org/r/567/s/104/
> Screenshot2
>   http://reviewboard.kde.org/r/567/s/105/
> Screenshot3
>   http://reviewboard.kde.org/r/567/s/106/
> 
> 
> Thanks,
> 
> Madlyn
> 
>