> 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 ...
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. > On Nov. 4, 2011, 11:29 a.m., Aaron J. Seigo wrote: > > libs/taskmanager/launcherproperties.cpp, line 159 > > <http://git.reviewboard.kde.org/r/103007/diff/3/?file=40210#file40210line159> > > > > this should be the correct values, not "-1U". NET::WM2WindowClass for > > the third and 0 for the first parameter.. OK, will do. Must admit this part was taken from the oxygen window decoration config. - Craig ----------------------------------------------------------- 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
