Skip to content

Add "No more jams" indication#287

Merged
brian-brazil merged 7 commits intorollerderby:devfrom
frank-weinberg:feature/noMoreJam
May 29, 2019
Merged

Add "No more jams" indication#287
brian-brazil merged 7 commits intorollerderby:devfrom
frank-weinberg:feature/noMoreJam

Conversation

@frank-weinberg
Copy link
Copy Markdown
Contributor

When there won't be another regular jam in this period without
intervention (TTO/OR), mark the headers of running clocks in red on the
operator screen and display the lineup/timeout clock with a red
background on the main view.

Closes #218

Includes #283 as I needed those stats to reliably detect if there has been a timeout that gives another jam.

@frank-weinberg frank-weinberg changed the title Feature/no more jam Add "No more jams" indication May 24, 2019
Copy link
Copy Markdown
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave the review until the timeout stuff is in.

REVIEWS_PER_PERIOD(new BooleanRule("Team.OfficialReviewsPer", "Are official reviews granted per period or per game?", true, "Period", "Game")),
NUMBER_RETAINS(new IntegerRule("Team.MaxRetains", "How many times per game or period a team can retain an official review", 1)),

MANDATORY_OVERTIME(new BooleanRule("Overtime.Mandatory", "Will a tied game always go to overtime?", true, "True", "False")),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used

protected Object computeValue(PermanentProperty prop, Object value, Object last, Flag flag) {
if (prop == Value.UPCOMING_JAM && !(value instanceof Jam)) {
value = new JamImpl(this, getCurrentPeriod().getCurrentJam());
} else if (prop == Value.NO_MORE_JAM) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic isn't quite right here, if I stop on 30 it goes red rather than allowing another jam.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are exactly 30s between the moment when the display flips to 30 and the moment when the display flips to 0. So if you stop when the 30 is already showing, there are only 29.x seconds remaining and there should not be another Jam.

But I've changed the code to give you another Jam when you hit exactly 30.0s.

@brian-brazil
Copy link
Copy Markdown
Contributor

Can you rebase this off dev?

When there won't be another regular jam in this period without
intervention (TTO/OR), mark the headers of running clocks in red on the
operator screen and display the lineup/timeout clock with a red
background on the main view.

Closes rollerderby#218
@frank-weinberg
Copy link
Copy Markdown
Contributor Author

Done.

Copy link
Copy Markdown
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new logic should have a unittest, covering OTO+TO cases preferably too.

Comment thread html/controls/operator.js Outdated
if (WS.state[prefix + '.WalltimeEnd'] == 0 && WS.state[prefix + '.WalltimeStart'] > 0) {
row.find("td.PC").text("running");
} else {
row.find("td.PC").text(_timeConversions.msToMinSec(v));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be working, it's always empty.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is showing elapsed time, we probably want whatever the operator is seeing in terms of inversion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Changed. (Also unit tests in.)

Comment thread html/controls/operator.js Outdated
$("<a>").text("Duration").addClass("Title")
.appendTo(headers.find("td:eq(2)").addClass("Title"));
$("<a>").text("PC at end").addClass("Title")
.appendTo(headers.find("td:eq(3)").addClass("Title"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer children to find when you can, it's faster.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was at it I did some other straightforward optimizations as well, like using k.Period etc.

@brian-brazil brian-brazil merged commit cc30abf into rollerderby:dev May 29, 2019
@brian-brazil
Copy link
Copy Markdown
Contributor

Hmm, points isn't showing on the Jams popup but I presume that's due to something else.

@frank-weinberg
Copy link
Copy Markdown
Contributor Author

Ah, damn, that was one children too much.

@brian-brazil
Copy link
Copy Markdown
Contributor

Changing those is always fun ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make it clear when <30 seconds lineup time at end of period

2 participants