First, thank you for code submission.
However like Greg, I have a few points to improve your patch before I can apply it. >Quick review of the case change: > > a large comment is removed, and there is no replacement comment > explainining the plan To be fair, the original comment explained how the code could probably be better, so removing the old stuff is fine. > in paricular, the missing new comment doesn't explain why the new way > causes no regressions Agreed, it still needs some comment about the casing, and that it *may* need to be 'Title Case' for certain devices to work properly: eg for Garmin Oregon 450 Some devices don't care - I've tested the change with my Garmin Etrex Legend HCx and it still works. > new procedures don't have comments explaining their purposes (even > though it's fairly easy to guess) Again to be fair most functions don't have comments, let alone a doxyen/gnome-doc friendly format. It would be nice, but not critical. > commit message does not describe the change first, but rather is a > history of why. Agreed the commit could explain a little bit more about how (although the why is more important). > The huge list of symbols is a) confusing - I'm not sure what "couldn't > be checked" means and b) too detailed for the commit message a. Agreed - Just remove. b. Agreed - something simpler like - "Casing of all symbols apart from 'custom placeholder' modified into Title Case'" If there are 'right capitalization rules', please provide some kind of reference - I have not been able to find any. > The commit message, or the mail sent to the list announcing it, does > not say what you have regression tested. Unfortunately this is probably the biggest drawback, we have some code which: 1. Is currently in use. - Given the original authors do not seem active any more, we have to try and understand both the old and new behaviour/implications. - It appears the original code was done in a lower case way due to misguided efficiency attempt (it's only done on GPX loading, so hardly a code path ran all the time!) 2. Don't really know how the various different devices handle this casing, obviously we don't want to break existing use. - Saying that, it looks like it's always been in 'Title Case' in GPSBabel. It would be great if we can get this tested with a couple more Garmin devices. [I think we should hold a list of what devices prominent Viking users have in the wiki...] >thanks for putting this up on a public git repo - that made looking at >it easy. For others, the URL is > > http://git.open-mesh.org/?p=ecsv/viking.git;a=summary NB I think one symbol needs further tweaking: Line 137 : "Us hwy" ---> "US hwy" (this matches GPSBabel casing) So in summary please add some appropriate comments in the code, especially in garminsymbols.c, add the above tweak and reword the commit message as suggested. Hopefully we'll have some more positive test feedback, so we can confidently apply it in time for the next release. Thanks for your support. ------------------------------------------------------------------------------ Colocation vs. Managed Hosting A question and answer guide to determining the best fit for your organization - today and in the future. http://p.sf.net/sfu/internap-sfd2d _______________________________________________ Viking-devel mailing list Viking-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/viking-devel Viking home page: http://viking.sf.net/
