----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127918/#review95519 -----------------------------------------------------------
kcms/componentchooser/componentchooserbrowser.cpp (line 101) <https://git.reviewboard.kde.org/r/127918/#comment64736> This KConfigGroup name is still confusing IMHO. This is NOT the kde4 config. It's the config group we are about to copy to the kde4 config. Rename to simpleConfigGroup ? Clear names will prevent more bugs in this code next time someone touches it. If this patch already changes all users of syncConfigGroup, then maybe it would actually be cleaner to change that to syncKdeglobals, which would take care of simpleConfig + the KConfigGroup. Less duplication + easier to read (one place for the comment that explains why this has to be SimpleConfig). kcms/migrationlib/kdelibs4config.h (line 28) <https://git.reviewboard.kde.org/r/127918/#comment64737> I even wonder why this is a separate method, it's only used in the sync... method, right? I would merge it there, making it a single method that takes care of opening both source and kde4 dest, and doing the copyTo. But if what I mean is unclear to you, just ship it and I'll do the refactoring I have in mind. - David Faure On May 16, 2016, 8:52 p.m., Hrvoje Senjan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127918/ > ----------------------------------------------------------- > > (Updated May 16, 2016, 8:52 p.m.) > > > Review request for Plasma and David Faure. > > > Repository: plasma-desktop > > > Description > ------- > > Address David's issues in previous rr's regarding the code. > > > Diffs > ----- > > kcms/componentchooser/componentchooserbrowser.cpp 5795c2b > kcms/componentchooser/componentchooserterminal.cpp 36f1296 > kcms/input/mouse.cpp f7d030f > kcms/migrationlib/kdelibs4config.h bb2dca2 > > Diff: https://git.reviewboard.kde.org/r/127918/diff/ > > > Testing > ------- > > Default browser is correctly written in both ~/.kde4/share/config/kdeglobals > and ~/.config/kdeglobals > > > Thanks, > > Hrvoje Senjan > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
