[haiku-bugs] Re: [Haiku] #2412: [Time] Integrate NTP client

#2412: [Time] Integrate NTP client
---------------------------------------+----------------------------
   Reporter:  diver                    |      Owner:  leavengood
       Type:  enhancement              |     Status:  assigned
   Priority:  normal                   |  Milestone:  R1
  Component:  Preferences/Time & Date  |    Version:  R1/Development
 Resolution:                           |   Keywords:
 Blocked By:                           |   Blocking:
Has a Patch:  1                        |   Platform:  All
---------------------------------------+----------------------------

Comment (by leavengood):

 I applied the patch and tested it out, and the main problem is that you
 used an old version of ntp.cpp (though I see you also made changes to it.)
 I fixed a pretty big bug in that file in r40103 in the NetworkTime source
 in Haiku's repo. I think I can merge in my changes though so I don't think
 you need to worry about that.

 Overall though it looks like you did a really good job. I want to take a
 little more time to review the changes and check the coding style as I've
 gotten in trouble in the past committing patches without thoroughly
 checking the style. In general though the style seems OK.

 Another thing is there may be a few things in the new layout based GUI
 which I might want to tweak. I also think the time server settings from
 NetworkTimeView.cpp should be moved in with the regular time settings. I
 can do that too.

 Lastly in the future it might be nice to break up work like this into two
 sections and associated patches:

 1. Making the GUI layout friendly.
 2. Adding stuff, like the Network time tab.

 This is a really big patch and that makes it a bit harder to review.
 Though I know Subversion makes this harder than it needs to be (with Git
 you could do two local commits with diffs for each...but that is another
 battle.)

-- 
Ticket URL: <http://dev.haiku-os.org/ticket/2412#comment:18>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.

Other related posts: