----------------------------------------------------------- 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel