Improve the traffic signal timing UI. There's a choice between fixed and

variable timing, but currently you have to remember to toggle it; the
two extra spinners get ignored otherwise. The new version is still
confusing, but I think it's an improvement.

game/src/debug/uber_turns.rs

@@ -243,7 +243,7 @@ impl SimpleState<App> for UberTurnViewer {
         &mut self,
         ctx: &mut EventCtx,
         app: &mut App,
-        panel: &Panel,
+        panel: &mut Panel,
     ) -> Option<Transition> {
         Some(Transition::Replace(UberTurnViewer::new(
             ctx,

game/src/edit/lanes.rs

@@ -169,7 +169,7 @@ impl SimpleState<App> for LaneEditor {
         &mut self,
         ctx: &mut EventCtx,
         app: &mut App,
-        panel: &Panel,
+        panel: &mut Panel,
     ) -> Option<Transition> {
         let mut edits = app.primary.map.get_edits().clone();
         edits.commands.push(app.primary.map.edit_road_cmd(

game/src/edit/traffic_signals/edits.rs

@@ -4,8 +4,8 @@ use map_model::{
     ControlStopSign, ControlTrafficSignal, EditCmd, EditIntersection, IntersectionID, StageType,
 };
 use widgetry::{
-    Choice, DrawBaselayer, EventCtx, Key, Line, Panel, SimpleState, Spinner, State, TextExt,
-    Toggle, Widget,
+    Choice, DrawBaselayer, EventCtx, Key, Line, Panel, SimpleState, Spinner, State, Text, TextExt,
+    Widget,
 };
 
 use crate::app::{App, Transition};
@@ -20,6 +20,7 @@ pub struct ChangeDuration {
 impl ChangeDuration {
     pub fn new(
         ctx: &mut EventCtx,
+        app: &App,
         signal: &ControlTrafficSignal,
         idx: usize,
     ) -> Box<dyn State<App>> {
@@ -45,55 +46,54 @@ impl ChangeDuration {
                 )
                 .named("duration"),
             ]),
-            Widget::row(vec![
-                "Type:".text_widget(ctx),
-                Toggle::choice(
-                    ctx,
-                    "stage type",
-                    "fixed",
-                    "variable",
-                    None,
-                    match signal.stages[idx].stage_type {
-                        StageType::Fixed(_) => true,
-                        StageType::Variable(_, _, _) => false,
-                    },
-                ),
-            ]),
-            Widget::row(vec![Line("Additional time this stage can last?")
-                .small_heading()
-                .into_widget(ctx)]),
-            Widget::row(vec![
-                "Seconds:".text_widget(ctx).centered_vert(),
-                Spinner::widget(
-                    ctx,
-                    (1, 300),
-                    match signal.stages[idx].stage_type {
-                        StageType::Fixed(_) => 0,
-                        StageType::Variable(_, _, additional) => {
-                            additional.inner_seconds() as isize
-                        }
-                    },
-                )
-                .named("additional"),
-            ]),
-            Widget::row(vec![Line("How long with no demand to end stage?")
-                .small_heading()
-                .into_widget(ctx)]),
-            Widget::row(vec![
-                "Seconds:".text_widget(ctx).centered_vert(),
-                Spinner::widget(
-                    ctx,
-                    (1, 300),
-                    match signal.stages[idx].stage_type {
-                        StageType::Fixed(_) => 0,
-                        StageType::Variable(_, delay, _) => delay.inner_seconds() as isize,
-                    },
-                )
-                .named("delay"),
-            ]),
             Line("Minimum time is set by the time required for crosswalk")
                 .secondary()
                 .into_widget(ctx),
+            Widget::col(vec![
+                Text::from_all(match signal.stages[idx].stage_type {
+                    StageType::Fixed(_) => vec![
+                        Line("Fixed timing").small_heading(),
+                        Line(" (Adjust both values below to enable variable timing)"),
+                    ],
+                    StageType::Variable(_, _, _) => vec![
+                        Line("Variable timing").small_heading(),
+                        Line(" (Set either values below to 0 to use fixed timing."),
+                    ],
+                })
+                .into_widget(ctx)
+                .named("timing type"),
+                "How much additional time can this stage last?".text_widget(ctx),
+                Widget::row(vec![
+                    "Seconds:".text_widget(ctx).centered_vert(),
+                    Spinner::widget(
+                        ctx,
+                        (0, 300),
+                        match signal.stages[idx].stage_type {
+                            StageType::Fixed(_) => 0,
+                            StageType::Variable(_, _, additional) => {
+                                additional.inner_seconds() as isize
+                            }
+                        },
+                    )
+                    .named("additional"),
+                ]),
+                "How long with no demand before the stage ends?".text_widget(ctx),
+                Widget::row(vec![
+                    "Seconds:".text_widget(ctx).centered_vert(),
+                    Spinner::widget(
+                        ctx,
+                        (0, 300),
+                        match signal.stages[idx].stage_type {
+                            StageType::Fixed(_) => 0,
+                            StageType::Variable(_, delay, _) => delay.inner_seconds() as isize,
+                        },
+                    )
+                    .named("delay"),
+                ]),
+            ])
+            .padding(10)
+            .bg(app.cs.inner_panel_bg)
+            .outline(ctx.style().section_outline),
             ctx.style()
                 .btn_solid_primary
                 .text("Apply")
@@ -111,11 +111,11 @@ impl SimpleState<App> for ChangeDuration {
             "close" => Transition::Pop,
             "Apply" => {
                 let dt = Duration::seconds(panel.spinner("duration") as f64);
-                let new_type = if panel.is_checked("stage type") {
+                let delay = Duration::seconds(panel.spinner("delay") as f64);
+                let additional = Duration::seconds(panel.spinner("additional") as f64);
+                let new_type = if delay == Duration::ZERO || additional == Duration::ZERO {
                     StageType::Fixed(dt)
                 } else {
-                    let delay = Duration::seconds(panel.spinner("delay") as f64);
-                    let additional = Duration::seconds(panel.spinner("additional") as f64);
                     StageType::Variable(dt, delay, additional)
                 };
                 let idx = self.idx;
@@ -133,6 +133,30 @@ impl SimpleState<App> for ChangeDuration {
         }
     }
 
+    fn panel_changed(
+        &mut self,
+        ctx: &mut EventCtx,
+        _: &mut App,
+        panel: &mut Panel,
+    ) -> Option<Transition> {
+        let new_label = Text::from_all(
+            if panel.spinner("delay") == 0 || panel.spinner("additional") == 0 {
+                vec![
+                    Line("Fixed timing").small_heading(),
+                    Line(" (Adjust both values below to enable variable timing)"),
+                ]
+            } else {
+                vec![
+                    Line("Variable timing").small_heading(),
+                    Line(" (Set either values below to 0 to use fixed timing."),
+                ]
+            },
+        )
+        .into_widget(ctx);
+        panel.replace(ctx, "timing type", new_label);
+        None
+    }
+
     fn other_event(&mut self, ctx: &mut EventCtx, _: &mut App) -> Transition {
         if ctx.normal_left_click() && ctx.canvas.get_cursor_in_screen_space().is_none() {
             return Transition::Pop;

game/src/edit/traffic_signals/mod.rs

@@ -223,6 +223,7 @@ impl State<App> for TrafficSignalEditor {
                     let idx = x.parse::<usize>().unwrap() - 1;
                     return Transition::Push(edits::ChangeDuration::new(
                         ctx,
+                        app,
                         &canonical_signal,
                         idx,
                     ));

widgetry/src/app_state.rs

@@ -221,7 +221,12 @@ pub trait SimpleState<A> {
     ) -> Transition<A>;
     /// Called when something on the panel has changed. If a transition is returned, stop handling
     /// the event and immediately apply the transition.
-    fn panel_changed(&mut self, _: &mut EventCtx, _: &mut A, _: &Panel) -> Option<Transition<A>> {
+    fn panel_changed(
+        &mut self,
+        _: &mut EventCtx,
+        _: &mut A,
+        _: &mut Panel,
+    ) -> Option<Transition<A>> {
         None
     }
     /// Called when the mouse has moved.
@@ -257,7 +262,7 @@ impl<A: 'static> State<A> for SimpleStateWrapper<A> {
             Outcome::Clicked(action) => self.inner.on_click(ctx, app, &action, &self.panel),
             Outcome::Changed => self
                 .inner
-                .panel_changed(ctx, app, &self.panel)
+                .panel_changed(ctx, app, &mut self.panel)
                 .unwrap_or_else(|| self.inner.other_event(ctx, app)),
             Outcome::Nothing => self.inner.other_event(ctx, app),
         }

widgetry/src/widgets/spinner.rs

@@ -71,11 +71,17 @@ impl Spinner {
             up.get_dims().height + down.get_dims().height + 1.0,
         );
         if current < low {
+            warn!(
+                "Spinner's initial value is out of bounds! {}, bounds ({}, {})",
+                current, low, high
+            );
             current = low;
-            warn!("Spinner current value is out of bounds!");
         } else if high < current {
+            warn!(
+                "Spinner's initial value is out of bounds! {}, bounds ({}, {})",
+                current, low, high
+            );
             current = high;
-            warn!("Spinner current value is out of bounds!");
         }
 
         let mut spinner = Spinner {