Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show current system time on submission page #3508

Closed
wants to merge 7 commits into from

Conversation

@xprilion
Copy link
Contributor

xprilion commented Mar 29, 2019

Partially closes #1865 - it is in discussion over some finer points now.

Sent a quick PR since it was a long pending issue.

Screenshot from 2019-03-29 16-30-28

Copy link
Member

shailpatels left a comment

This looks like the current time is only ever sent on page load and never updated again.
At the very least we need the time to update to be current dynamically from the initial page call and ideally, we would like it to semi-frequently poll the server to ensure the display time matches server time.

@MasterOdin

This comment has been minimized.

Copy link
Member

MasterOdin commented Mar 30, 2019

@bmcutler, @inlinefan, how often have we gotten drift on the main machine in past year or so? I know we had issues a couple years ago, but it seems like ntp has been stable and preventing any real drift.

@xprilion

This comment has been minimized.

Copy link
Contributor Author

xprilion commented Mar 31, 2019

Hey @shailpatels I've implemented the following -

When the page loads, it could be at any second (seconds are universally the same) so a Time fetch from server occurs after the remaining seconds from the next complete minute.

Henceforth, at every minute, a time fetch from server occurs, effectively eliminating the chance of mismatched time on server and in the client UI, and the overhead of constant polling. Also, the format of time and date display remains consistent. Since the update interval is set irrespective of the response time of the fetch time call, the interval is maintained at every 60 seconds, strictly.

The time updates only when the call to server is successful, so the UI doesn't flicker nor does the time get cleared from the browser in case of a failure.

@inlinefan

This comment has been minimized.

Copy link
Contributor

inlinefan commented Mar 31, 2019

@xprilion

This comment has been minimized.

Copy link
Contributor Author

xprilion commented Apr 1, 2019

@inlinefan would you suggest simply using JavaScript based time update instead?

@MasterOdin

This comment has been minimized.

Copy link
Member

MasterOdin commented Apr 2, 2019

I would make it so that you fetch time from the server on page load (to ensure we're looking at the timezone set by the server, and not just what the user has set), and then use JS to increment it upwards.

@bmcutler

This comment has been minimized.

Copy link
Member

bmcutler commented Apr 3, 2019

We had a draft implementation of this issue months ago, but there were bugs in the time/timezone (production installations do not match vagrant in this area) and concerns about the expense of polling and concerns about the visuals being unnecessarily distracting.

This needs lots of testing, and a mini-survey of user input on the final chosen design.

@bmcutler

This comment has been minimized.

Copy link
Member

bmcutler commented May 14, 2019

@xprilion, what is your current status on this PR?

@xprilion

This comment has been minimized.

Copy link
Contributor Author

xprilion commented May 14, 2019

@bmcutler I'm awaiting some conversation over this issue for the design finalization. Shall I draw up some mockups and maybe some of us can vote over them? Or is there any other existing method of doing the design surveys at Submitty, we could definitely use that. Please let me know.

@bmcutler

This comment has been minimized.

Copy link
Member

bmcutler commented May 14, 2019

So this feature is an optional feature (hopefully helpful to students, but not strictly necessary) -- and as such we want to make sure it doesn't cause confusion or mis-information or stress or cause load on the server/network/student machine.

So we want to make sure the display is subtle -- it doesn't update every second, it's not visually distracting. Maybe it's not even shown if the deadline is more than 24 hours away. Maybe when the deadline is less than 1 hour away the color changes to red (or some other visual change)? But no blinking text or anything stressful. Yes, maybe you can post several options for the visual we can discuss/vote on.

It should work for all combinations of timezones -- if the student & the server are the same timezone, or if they are different timezones. We want to ALWAYS display the server timezone (a student doesn't get an extension if they set their local machine to a different timezone!). This will require lots of testing. If/when you've done this testing, please explain :)

If the student leaves the page open for a long time without refreshing, or if they put their machine to sleep and wake it back up -- we want to make sure the display doesn't get significantly out of sync (more than a minute or so). This will require lots of testing. If/when you've done this testing, please explain. I assume we need to test on the different OS & browsers as well. Please ask for help for whatever you don't have access to.

We don't want to poll the server very often, most of this time updating can be done on the client machine exclusively.

@bmcutler

This comment has been minimized.

Copy link
Member

bmcutler commented May 22, 2019

@xprilion What is the status of your work on this PR? Are you looking for any guidance on implementation?

@xprilion

This comment has been minimized.

Copy link
Contributor Author

xprilion commented May 24, 2019

@bmcutler the task was a bit complex on the head and after implementing it once, I realized there was a simpler implementation, thus the time taken. But finally, it is done in the following manner -

[1] At page load, a time out for fetching the server time when the number of seconds on the clock is 0 is set.
[2] the server time is fetched at clock 0 time, but the latency might cause a few seconds to shift. This is adjusted into the server time.
[3] a date variable with the server's time offset is set into the browser. This keeps updating on the page every 1 minute.
[4] After 10 minutes, the server's time is fetched again, synchronizing any time differences that might have crept up.

Worst time difference case:
[1] Right after time update, the person puts the machine to sleep. The next update is 59 seconds away.
[2] Person opens up the laptop exactly x minutes later.
[3] The page will display the wrong time for a maximum of 5 minutes from the time of resuming laptop operation.

This can be bettered. I shall think of another way, or maybe we can discuss this out! :)

@bmcutler bmcutler requested a review from MasterOdin May 25, 2019
@xprilion xprilion requested a review from shailpatels May 27, 2019
Copy link
Member

shailpatels left a comment

Overall the time does show in the correct place and isn't too distracting, as Barb said it would be better to maybe show this if its due within 24 hrs or a closer time period instead of always showing it.

Copy link
Member

shailpatels left a comment

You mentioned you're thinking of a better way, if so I recommend what Matt said and grab the server time onload and increment by JS is fine if we grab the current time every minute or two. If our precision is only up to a minute this should be fine and a lot simpler to implement.

Up to you if you think you have a better idea and want to start a new PR or work off of this one.

I'm not sure about this but did we decide to show military time to students or use AM/PM (I always forget @bmcutler )

}, (60 - localDate.getSeconds())*1000);
}
$.ajax({

This comment has been minimized.

Copy link
@shailpatels

shailpatels May 29, 2019

Member

Could you remove the console.logs and also the commented out ones and replace them with comments about the logic of the function, I think that would be more helpful

// console.log("Server time remaining: "+(60 - serverDate.getSeconds()));
setTimeout(function(){
// console.log("Setting interval");
serverDate = new Date((result.data.time + (60 - serverDate.getSeconds()))* 1000);

This comment has been minimized.

Copy link
@shailpatels

shailpatels May 29, 2019

Member

Why is there another nest setTimeout function when theres already one above the ajax call. If the ajax response is delayed for whatever reason I don't think this will correct time skew, it might make it worse if the ajax response is really out of sync and just overwrites the setTime function with an outdated time.

@xprilion

This comment has been minimized.

Copy link
Contributor Author

xprilion commented May 29, 2019

@shailpatels thanks for the review. Closing this in favor of a new PR, it has been around quite some time!

Will keep the display as is currently, will change the method of time fetching! :)

@xprilion xprilion closed this May 29, 2019
@shailpatels shailpatels mentioned this pull request May 31, 2019
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.