#2412 closed enhancement (fixed)
[Time] Integrate NTP client
| Reported by: | diver | Owned by: | leavengood |
|---|---|---|---|
| Priority: | normal | Milestone: | R1/beta1 |
| Component: | Preferences/Time & Date | Version: | R1/Development |
| Keywords: | Cc: | rossi@…, mdisreali@…, hamish@… | |
| Blocked By: | Blocking: | ||
| Platform: | All |
Description
Integrate NTP client in Time preflet. Something like this: http://www.bebits.com/app/4044
Attachments (4)
Change History (33)
comment:1 by , 16 years ago
| Cc: | added |
|---|
comment:2 by , 14 years ago
| Cc: | added |
|---|---|
| Version: | R1/pre-alpha1 → R1/Development |
comment:3 by , 14 years ago
If someone has the time to look into it, I've attached the NetworkTime sources. Released under MIT license.
comment:4 by , 14 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
Hamish Morrison did most of the work to get NetworkTime going in Haiku as part of a GCI task and I committed it in hrev40088. I think it could still use some work and maybe integration with the Time preflet so I'll leave this ticket open, but will take ownership.
comment:5 by , 14 years ago
Not sure what he did, as NetworkTime should have built and worked on Haiku out of the box (it definitely did over here). In any case, I see no point in adding it to Haiku as is -- it should be integrated into the system, and not be kept as a standalone application.
comment:6 by , 14 years ago
| Cc: | added |
|---|
Indeed, the code built and ran without any changes. I wasn't sure what was meant by "integrate into Haiku" in the GCI task, so I merely added a Jamfile and an rdef.
I'd be happy to work on this further; perhaps integrate it into the time preflet?
follow-up: 8 comment:7 by , 14 years ago
Hey Hamish,
If you want to work on that, that would be great. In fact I was going to ask you in an email to try it as a GCI task, but did not have the time to prepare the task before the end of the GCI. Plus this is a fairly complex task and it was the last week of GCI when I was going to create the task.
Anyhow, here is the email:
I appreciate your work in adding NetworkTime to Haiku but I think the other developers actually wanted it integrated into the Time prelet. I don't think that was properly explained in the original GCI task.
So what I'd like to propose is another GCI task to integrate NetworkTime as a new tab, called (wait for it): Network Time. While you are at it you could make Time use the layout kit (which we could make another GCI task so you get more points.)
My thoughts on the UI is checkboxes for the options currently in the NetworkTime menu, a menu field for the server, and the same Edit Server List window (opened by a button) and the Update button and progress bar currently in the NetworkTime window. All that should fit fine with the current size of the Time prelet.
Now the other aspect of this is the option to have the time updated periodically. For this I really think we need some sort of scheduled task runner in Haiku, and that is definitely outside the scope of this task. So for now I would not worry about that.
follow-up: 9 comment:8 by , 14 years ago
Replying to leavengood:
Now the other aspect of this is the option to have the time updated periodically. For this I really think we need some sort of scheduled task runner in Haiku, and that is definitely outside the scope of this task. So for now I would not worry about that.
I disagree - while the periodical execution is indeed out of the scope, we still need a standalone mechanism for it to update the time. We could then update the time once after booting, for now at least. I think the easiest way to accomplish that would be to add a command line option to Time that only updates the time without showing an UI. However, this will bring the Time entry to the Deskbar -- so I guess the best option would be to have a separate CLI tool that actually does the work (and that can read the settings, too). It could be loaded by Time as an add-on, or just started via system().
Looking forward to your work, Hamish!
follow-up: 10 comment:9 by , 14 years ago
Replying to axeld:
I disagree - while the periodical execution is indeed out of the scope, we still need a standalone mechanism for it to update the time. We could then update the time once after booting, for now at least. I think the easiest way to accomplish that would be to add a command line option to Time that only updates the time without showing an UI. However, this will bring the Time entry to the Deskbar -- so I guess the best option would be to have a separate CLI tool that actually does the work (and that can read the settings, too). It could be loaded by Time as an add-on, or just started via system().
OK, that is a reasonable suggestion. I guess the easiest thing would be to just extract out the GUI of NetworkTime as I suggested before, but leave the command-line code as a new bin program, network_time. The Time preflet could write out settings which the network_time command line program could read (they could share the same settings class with the right options in the Jamfile), while there could still be the option to override those settings with command-line options. When the "Update" button is pressed in the Time preflet it will just use system() to run network_time. Then network_time could be called at the end of the Bootscript to set the time as Axel says.
Then if one day we have a scheduled tasks system network_time can also just be called from that in whatever frequency the user wants.
The only problem I see with this approach is that it will be harder to use a progress bar in the Time preflet if the network_time command is actually doing all the work. Though in my experience the operation was always so fast the progress bar wasn't really needed.
comment:10 by , 14 years ago
Replying to leavengood:
The only problem I see with this approach is that it will be harder to use a progress bar in the Time preflet if the network_time command is actually doing all the work. Though in my experience the operation was always so fast the progress bar wasn't really needed.
I don't think the progress bar is needed, indeed, and I haven't seen it anywhere else either yet. That's not a loss :-)
follow-up: 12 comment:11 by , 14 years ago
Should the automatic setting every bootup be optional, i.e. via a checkbox in the Time preflet? There may be circumstances someone deliberately wants to work with an off clock. Also, seeing this checked option there may be comforting to other users, as they are then aware that their clock is always taken care of. Just a thought...
comment:12 by , 14 years ago
Replying to humdinger:
Should the automatic setting every bootup be optional, i.e. via a checkbox in the Time preflet?
Sure, it should be optional. In addition to the check box ("Synchronize time at system boot"), I would also like to see a button "Synchronize now".
comment:13 by , 14 years ago
Pardon the lack of progress; I've been quite busy with other things.
The custom controls in the other tabs aren't playing so nicely with the layout manager, so that's taking a little time. Don't fear, though: I haven't abandoned this.
by , 14 years ago
| Attachment: | time_pref_net.patch added |
|---|
networktime integrated into time preflet, and time preflet converted to layout manager
comment:14 by , 14 years ago
| patch: | 0 → 1 |
|---|
Everything seems to work fine.
I consolidated the clock into a single class, because I couldn't get the previous setup to work nicely with the layout manager.
You can do a silent NTP update with --silent-update. If synchronizing at boot is disabled, this returns immediately. The icon doesn't appear in the deskbar because a TimeApplication isn't created.
Didn't bother including the progress bar. The "Synchronize now" button simply changes to "Stop" while the update is going on.
by , 14 years ago
| Attachment: | time_pref_net2.patch added |
|---|
comment:15 by , 14 years ago
Apologies, I forgot to svn add some files in the last patch.
Additionally, I removed the NTP server list window entirely and integrated it into the tab page. Did some other miscellaneous refactoring, and also hooked the network time view up to the revert button.
comment:16 by , 14 years ago
All of what you last commented is or is not in patch 2 attached here? It is a big patch but I will try to review, apply and commit it this weekend.
comment:18 by , 14 years ago
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 hrev40103 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:
- Making the GUI layout friendly.
- 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.)
comment:19 by , 14 years ago
Indeed, I would have liked to separate the patches, but in this case the whole thing had to be converted to the layout manager before I could use the layout manager to construct the network time tab.
Also, this appears to fix #7067. I'm not exactly sure how, but I no longer observe that behaviour in the time preflet.
comment:20 by , 14 years ago
Hi Hamish,
Sorry for the silence, I have not had time for Haiku development lately. I will try to find some time this weekend to finish this ticket and will also test to see if #7067 is fixed too.
comment:23 by , 14 years ago
| Status: | assigned → in-progress |
|---|
OK this patch has been sitting in my local repo for a while. I will work on it now and maybe it could still make it into alpha3.
comment:24 by , 14 years ago
| Resolution: | → fixed |
|---|---|
| Status: | in-progress → closed |
I applied Hamish's patch in hrev41930 with a few small changes, so I think we can finally close this ticket! Thanks for your work Hamish and sorry for the delay on applying your patch. Your efforts are much appreciated! When you have time I'd love to see more patches!
comment:25 by , 14 years ago
It could be a good idea to add a comment to if (region == B_TRANSLATE("Etc")) that "Etc" is a timezone area, and not an abbreviation.
I suggest a comment like "Timezone area, eg Etc/UTC".
comment:27 by , 14 years ago
There's a patch by mt in #7642 (Add more localization for Time & Date preflet).
comment:28 by , 14 years ago
Sorry, I was working on a patch for the localisation before I realised there already was one. In the latest revision the error message boxes and the error strings in ntp.cpp are missing localisation. This patch should cover that. (I assume it's all right to B_TRANSLATE a string with sprintf format specifiers?)
comment:29 by , 14 years ago
Thanks Hamish, I applied your patch in hrev41955, though I changed back the button saying "Synchronize again" after doing a synchronization.
There is also http://www.bebits.com/app/624 which has source available, however it is GPL. I'd rather see Axel's NTP client used, or new code under MIT, as that would make things easier.