> On 2009-04-10 18:43:48.042762, Paul Wise wrote: > > Patch looks good, except for a few remaining comments from the previous > > version and adding copyright info and adding your name to AUTHORS. > > > > You might also want to add doxygen comments for the new public functions. > > > > I assume it compiles and you've tested it? Please mention that in the > > testing done section.
Added some text on testing. > On 2009-04-10 18:43:48.042762, Paul Wise wrote: > > synfig-studio/trunk/src/synfigapp/inputdevice.cpp, line 283 > > <http://patches.synfig.org/r/19/diff/2/?file=121#file121line283> > > > > This change belongs in a separate patch since it has nothing to do with > > device settings saving. Added separate patch for review. > On 2009-04-10 18:43:48.042762, Paul Wise wrote: > > synfig-studio/trunk/src/gtkmm/devicetracker.cpp, line 80 > > <http://patches.synfig.org/r/19/diff/2/?file=119#file119line80> > > > > Whitespace removal, please undo this. This appears to be a bug in the patch review software since this line is not in the original patch file. > On 2009-04-10 18:43:48.042762, Paul Wise wrote: > > synfig-studio/trunk/src/gtkmm/devicetracker.cpp, line 8 > > <http://patches.synfig.org/r/19/diff/2/?file=119#file119line8> > > > > You have added a fair bit of code, please add your copyright here like > > this: > > > > Copyright 2009 Gerco Ballintijn > > > > Please also add yourself to the AUTHORS file. You are already in > > about.cpp so that doesn't need changing. Done. > On 2009-04-10 18:43:48.042762, Paul Wise wrote: > > synfig-studio/trunk/src/gtkmm/devicetracker.cpp, line 33 > > <http://patches.synfig.org/r/19/diff/2/?file=119#file119line33> > > > > Please file a bug on GTKmm asking for the required features to be added > > and add a comment and link about it here. I confirmed this idea by looking at the actual wrapper code for Gtk::Device, and reading a TODO entry for the required functionality in there. So I guess the GTKmm already know some extra work is needed. > On 2009-04-10 18:43:48.042762, Paul Wise wrote: > > synfig-studio/trunk/src/gtkmm/devicetracker.cpp, line 59 > > <http://patches.synfig.org/r/19/diff/2/?file=119#file119line59> > > > > Should this comment be changed too? I think these comments should simple be removed. The commented-out code was basically wrong, so it should have been removed completely (not just commented out). With this patch initializing is done via the DeviceSettings class. The comment is no longer relevant. Should this also be a separate patch? - Gerco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://patches.synfig.org/r/19/#review34 ----------------------------------------------------------- On 2009-04-10 10:13:52.868668, Gerco Ballintijn wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://patches.synfig.org/r/19/ > ----------------------------------------------------------- > > (Updated 2009-04-10 10:13:52.868668) > > > Review request for Synfig. > > > Summary > ------- > > This a preliminary patch that adds the capability of saving and restoring the > input devices configuration (i.e., tablet configuration). > > Note: The code specifically doesn't use the GTKmm layer but uses GTK+ > directly since the GTKmm wrapper is incomplete. > > There are a couple of I don't like about the patch, but first I await your > comments... > > > Diffs > ----- > > synfig-studio/trunk/src/gtkmm/app.cpp 2357 > synfig-studio/trunk/src/gtkmm/devicetracker.h 2357 > synfig-studio/trunk/src/gtkmm/devicetracker.cpp 2357 > synfig-studio/trunk/src/synfigapp/inputdevice.h 2357 > synfig-studio/trunk/src/synfigapp/inputdevice.cpp 2357 > > Diff: http://patches.synfig.org/r/19/diff > > > Testing > ------- > > > Thanks, > > Gerco > > ------------------------------------------------------------------------------ This SF.net email is sponsored by: High Quality Requirements in a Collaborative Environment. Download a free trial of Rational Requirements Composer Now! http://p.sf.net/sfu/www-ibm-com _______________________________________________ Synfig-devl mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/synfig-devl
