----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120624/#review68757 -----------------------------------------------------------
Found a bunch of issues around string handling, error handling and compiler definitions. misc/gtkbreeze/CMakeLists.txt <https://git.reviewboard.kde.org/r/120624/#comment48061> This (and a few other includes here) don't seem necessary. Could be cleaned up a tad. misc/gtkbreeze/CMakeLists.txt <https://git.reviewboard.kde.org/r/120624/#comment48049> Is this really needed here? I think we can just make the code work with these flags set, and be happier about it. Why did you add those? misc/gtkbreeze/CMakeLists.txt <https://git.reviewboard.kde.org/r/120624/#comment48062> Might aswell just put main.cpp here and remove the additional temporary var gtkbreeze_SRCS. It's only one source file, anyway. misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48050> foreach ( missing space misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48048> "/" should be a QDir::Separator, this will make it much faster since it can skip the more expensive QString ctor. Also, inconsistent spacing on that line. misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48051> QDir::separator() instead of "/" misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48052> QStringLiteral("oxygen-gtk") is faster misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48053> exists (grammar) misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48054> Use QStringLiteral here and on the following line for faster string creation misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48055> no spacing inside parentheses misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48056> Error handling when opening the file fails? misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48057> Wrapping all the strings on this and the following lines in QStringLiterals will make creation of all those strings much faster. (And removes the need for non-standard compiler flags.) misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48058> Superfluous, the QFile object goes out of scope here anyway, and will be closed in the dtor. misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48046> const misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48047> const misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48059> const misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48045> I don't understand this. Does our startkde set the Ubuntu font? (I'm pretty sure that it's not.) Can you clarify, or perhaps explain how the Oxygen font is set here? misc/gtkbreeze/main.cpp <https://git.reviewboard.kde.org/r/120624/#comment48060> Error handling here could be improved. I don't now exactly what makes sense here (return !0 if no settings have changed, if settings files aren't writable, perhaps?). - Sebastian Kügler On Oct. 19, 2014, 3:15 p.m., Jonathan Riddell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120624/ > ----------------------------------------------------------- > > (Updated Oct. 19, 2014, 3:15 p.m.) > > > Review request for Plasma and Hugo Pereira Da Costa. > > > Repository: breeze > > > Description > ------- > > add gtkbreeze, kconf_update tool to set gtk settings on first login > this checks if gtk settings are already set, if they are not or are set to > oxygen they update them, else it quits > it does this for both gtk 2 and 3 > it sets gtk to the orion theme because it's available for both gtk 2 and 3 > and it looks similar to breeze > it sets the icons to oxygen because I could not get breeze icons to work with > gtk 2 or 3 > I also could not get icons to show on buttons or in menus in gtk 3 > > > Diffs > ----- > > misc/CMakeLists.txt ff891a9 > misc/gtkbreeze/CMakeLists.txt PRE-CREATION > misc/gtkbreeze/gtkbreeze.upd PRE-CREATION > misc/gtkbreeze/main.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/120624/diff/ > > > Testing > ------- > > run it and run gtk-demo and gtk3-demo > > > Thanks, > > Jonathan Riddell > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel