> On Nov. 4, 2011, 11:29 a.m., Aaron J. Seigo wrote: > > lots of coding style issues, though substance-wise it looks ok. a big > > concern i'm left with right now is that we end up with all of these patches > > in libtaskmanager and while it has more features, the overall quality of > > the code goes down due to the massive nature of the patches going in with > > very little testing combined with a decrease in readability due to less > > consistent coding style. > > > > it also seems that in this patch we lose the ability to associate an > > executable with a launcher. that is a regression that needs addressing. > > > > i also need to look at the config UI still, as well ... > > Craig Drummond wrote: > Sorry for the coding issues - I thought I had run 'astyle', but perhaps I > am mistaken. I *promise* to run 'astyle' when all the changes are checked in, > and go through the files by hand to check the spacing. I agree that > consistent style is a good idea. > > As to not associating an executable with the launcher, this *was* > mentioned even before I submitted the first diff. But anyway, yes this needs > to be fixed. In fact the issue is already resolved, I just have not updated > the patch - I dont have access to my git checkout at the moment. > > I dont agree with the "very little testing", these changes are used in > IconTasks - and that has been out for about couple of months.
the code added has been tested a fair amount, but nowhere near the testing that it will get now that it is in a mainline Plasma Workspaces release. your code is about to be exposed by orders (plural) of magnitude more people. but more importantly: it has not been tested in the form it is in git right now, which is merged with the upstread libtaskmanager. we're making changes to the patches and it will be used with more than just the icon tasks widget now. so, yes, testing is currently an issue. not a fatal one, and one that will repidly be addressed once this is merged and the first betas go out ;) i just want us to be aware of this and ready to address problems that may arise that we're unaware of right now. nothing we (e.g. you and i) can't or won't handle, but awareness == readiness. - Aaron J. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103007/#review7940 ----------------------------------------------------------- On Nov. 3, 2011, 9:21 p.m., Craig Drummond wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103007/ > ----------------------------------------------------------- > > (Updated Nov. 3, 2011, 9:21 p.m.) > > > Review request for Plasma. > > > Description > ------- > > 1. If fail to automatically find launcher, then prompt user to select from > installed applications. > 2. Add a config page, so that manualy set launchers may be adjusted. > > (Part of IconTasks' taskmanager changes) > > > Diffs > ----- > > libs/taskmanager/CMakeLists.txt 6ae36dc > libs/taskmanager/groupmanager.h acaa142 > libs/taskmanager/groupmanager.cpp 6e7ffa7 > libs/taskmanager/launcherconfig.h PRE-CREATION > libs/taskmanager/launcherconfig.cpp PRE-CREATION > libs/taskmanager/launcherconfig.ui PRE-CREATION > libs/taskmanager/launcherproperties.h PRE-CREATION > libs/taskmanager/launcherproperties.cpp PRE-CREATION > libs/taskmanager/launcherproperties.ui PRE-CREATION > libs/taskmanager/taskactions.cpp 011c565 > libs/taskmanager/taskactions_p.h 407b2c9 > libs/taskmanager/taskitem.h 3c84678 > libs/taskmanager/taskitem.cpp e2c07d8 > > Diff: http://git.reviewboard.kde.org/r/103007/diff/diff > > > Testing > ------- > > > Thanks, > > Craig Drummond > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
