-
Notifications
You must be signed in to change notification settings - Fork 24
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
I1027 Add Contest Clock To WTI #1034
base: develop
Are you sure you want to change the base?
I1027 Add Contest Clock To WTI #1034
Conversation
the abstract and concrete versions)
can be used to transform Date objects into strings displaying day/hours/minutes/seconds.
- a count of the number of elapsed seconds (planned for removal), and - a string showing days plus hh:mm:ss (the real plan).
added documentation
apply renaming of updateContestClock() to updateLocalContestClockFromServer().
Conflicts: projects/WTI-UI/src/app/modules/core/services/contest.service.ts projects/WTI-UI/src/app/modules/login/components/login-page/login-page.component.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not run it on my machine yet, but these are my opinions. Looks like it would work well!
projects/WTI-UI/src/app/modules/core/services/contest.service.ts
Outdated
Show resolved
Hide resolved
projects/WTI-UI/src/app/modules/core/services/contest.service.ts
Outdated
Show resolved
Hide resolved
projects/WTI-UI/src/app/modules/core/services/contest.service.ts
Outdated
Show resolved
Hide resolved
console.error ("LoginPageComponent.onSubmit() ContestClock subscription callback: unable to get ContestClock from PC2 API via ContestService!"); | ||
} else { | ||
//copy the data fields received from the PC2 Server (via the WTI-API) into the local ContestClock object | ||
this.contestClock.isRunning = data.running ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of nested conditionals here, maybe we can use break to break out of conditions. This would improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow this comment. As far as I can tell there are no nested conditionals which would allow using break
. What there ARE (or at least, WERE) are a lot of "debug if's". I will remove all those debug statements; hopefully the logic will be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant return sorry. Maybe the method can be terminated early if it is not going to lead anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the large number of "debug" statements, which makes the nested conditionals pretty easy to follow (as JohnB has noted elsewhere, the traditional JavaScript usage of "lamba functions" does complicate it a little bit). As it now stands I don't really see where there's any place that the addition of "return" statements would be particularly helpful...
Let me know if you have a specific suggestion (short of rewriting the whole thing avoiding lambda expressions :) for improving it further. If not, please mark the conversation "Resolved".
* and if wallClockStartTime is greater than zero it fetches the current problem list from the server and displays it in the | ||
* component; otherwise it sets the list of displayed problems to an empty list (array). | ||
*/ | ||
private loadProblems(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woah. Is it possible to simplify this method through having multiple methods breaks etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the previous comment, I don't see any way to use break
s to address this. As before, I think the problem was the large number of "debug" if
s in the code, making it hard to see the structure. I've eliminated all those debug statements; hopefully that makes the code more readable. I'll welcome any suggestion for how to use break
s to simplify it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the in-line lambda function is what clutters it up. Folks love doing that in javascript too, and it makes it very hard to follow and read. I guess it's the nature of the beast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant return sorry. Maybe the method can be terminated early if it is not going to lead anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted, I've removed the large number of "debug" statements; hopefully this makes the code clearer. Also, as @johnbrvc noted, the use of JavaScript "lambda functions" makes it a little hard to read. (I didn't write that code originally, and I'm reluctant to change it as part of this PR. If you think it still needs to be changed, I suggest submitting a separate Issue for cleaning that up...)
Again, let me know if you have a specific suggestion (short of rewriting the whole thing avoiding lambda expressions :) for improving it further. If not, please mark the conversation "Resolved".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More opinions!
if (this.intervalId) { | ||
console.error("ContestTimerService.startTimer(): call to startTimer() when timer is already running"); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tabbing doesn't seem to match on my screen here and other places. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is just on my screen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are talking about the way indentation appears when you look at the code in Eclipse, that's a function of your Eclipse settings -- both global settings and the setting for the particular editor you're viewing it with. At one point in time we (the early members of the PC2 project) had agreed upon a common set of indentation settings; perhaps yours don't match?
(If it's NOT in Eclipse that you're talking about, then where is it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that there is an actual "TAB" character on the lines in question. As @clevengr pointed out, if your Eclipse settings are "right" (by "right" I mean that they match ours), it will line up properly. What IS interesting is, the code indenting appears to violate the indenting rules that we use in the java code. The particular line in @kkarakas example has a TAB SPACE SPACE. It seems to me, if TAB is 4 spaces, then the next level of indenting should be a total of 8 spaces (however you want to arrive at that: TAB TAB, 8x SPACE, etc.) Not sure where the "+2" spaces comes from, unless that's standard for TS? I attached a screenshot of my general editor properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% positive, but I think the "default standard" for TypeScript IS "2 space indents". I say this only because almost all the TS code we inherited from the students who did the original WTI project was indented by 2 spaces (on each indent). I know that has caused some aggravation for me... Not sure how we "fix" this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's fine, but what they did initially, it seems, is they "know" that TAB is 4 spaces, which is 2 "TS" indents, one keypress. Ugh. So, I guess just set your TAB to 4 spaces, and you should be ok. The problem is, adding new code, you have to be aware of this and increase levels by 2 spaces...
padding-left: 8px; | ||
padding-right: 8px; | ||
} | ||
|
||
/* Set the container holding the contest clock value(s) to medium, white font */ | ||
.clock-container { | ||
color: white; | ||
font-size: 1.0rem; | ||
padding-left: 8px; | ||
padding-right: 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some concerns about how these pixel paddings would look in different window sizes. Did you play around with different window size and different pixel counts? Perhaps we could use relative units such as em/ rem, vw, just like you use it for font-size. But this is not necessarily problematic, there are cases where pixels are better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try it using various window sizes, and I didn't spot any problems. Do you have a specific example of a platform and window size which shows a problem?
Regarding the "pixel paddings", I didn't create those; they came from a CSS file I got from @johnbrvc . But I haven't observed any issues with it (I tested it in both Chrome and Edge, using various window sizes). Again, do you have an example of a specific situation where a problem is demonstrated?
projects/WTI-UI/src/app/modules/shared/components/app-header/app-header.component.ts
Show resolved
Hide resolved
projects/WTI-UI/src/app/modules/shared/components/app-header/app-header.component.ts
Outdated
Show resolved
Hide resolved
- use concrete data types rather than "any". - remove debug statements. - use "const" instead of "let" in variable declarations where applicable.
Description of what the PR does
Adds "Elapsed Time" and "Remaining Time" counters to the WTI header bar whenever a team is logged in.
The counters are initialized with values read from the PC2 Server (via the WTI-API Server and its PC2 API connection) each time a team logs in. The counters respond appropriately to changes in the "PC2 contest clock" state -- that is, when the Admin "Starts" the contest the counters begin counting (up/down respectively), and the counters stop (freeze) whenever the Admin "Stops" (pauses) the contest. The counters also respond correctly to changes in the contest times; for example, if the Admin extends the contest (adds more time), then the "Remaining Time" counter automatically adjusts as appropriate.
Times are displayed in HH:MM:SS format, preceded by a number of days if the total time is greater than 24 hours.
The counters are driven by a JavaScript setInterval() object which ticks at a rate of once per second. To handle potential clock drift, the code automatically "resynchronizes" the on-screen counters with the current PC2 Server time, at a frequency defined by a constant in the WTI-UI
constants.ts
file (currently every 15 minutes).A screenshot showing the clocks for a contest that was scheduled to run for 30 hours and has been running for just over an hour is shown here:
Issue which the PR addresses:
Fixes #1027, #1029
Environment in which the PR was developed (OS,IDE, Java version, etc.)
Precise steps for testing the PR :
./bin/pc2wti
.length of the contest (for the default values in the "sumithello" contest, that should be "05:00:00")
Note: if you change the Contest Length in the contest.yaml file BEFORE STARTING THE SERVER, and the value you set for
contest length is greater than 24 hours, the "Remaining Time" field should also include a count of the number of DAYS remaining.
a separate issue will be filed for this.)