ballaswag / guppyscreen

A native Touch UI for 3D Printers running Klipper/Moonraker.
GNU General Public License v3.0
167 stars 14 forks source link

Add 5 min display timeout #79

Closed kevdliu closed 1 month ago

kevdliu commented 1 month ago

This PR adds a 5 minute option to the list of display timeout durations. 10 minutes is a bit long (in my opinion) since most of the time I'm only making short adjustments.

ballaswag commented 1 month ago

I think you actually want to add new options at the very end, with the next index instead of reordering them. The options text can be ordered, but not the mapped indexes. Existing settings will be surprised if their sleep values are changed.

kevdliu commented 1 month ago

Thanks for the feedback. I'm actually having trouble understanding why the current ordering wouldn't work. Looking at this code

lv_dropdown_set_options(display_sleep_dd,
            "Never\n"
                        "5 Minutes\n"
            "10 Minutes\n"
            "30 Minutes\n"
            "1 Hour\n"
            "5 Hours");

if (!v.is_null()) {
    auto sleep_sec = v.template get<int32_t>();
    const auto &el = sleepsec_to_dd_idx.find(sleep_sec);
    if (el != sleepsec_to_dd_idx.end()) {
      lv_dropdown_set_selected(display_sleep_dd, el->second);
    }
  }

The menu looks up the index of the selected option using sleepsec_to_dd_idx, so as long as the mapping in sleepsec_to_dd_idx is consistent with the list passed to lv_dropdown_set_options then everything should still work right?

ballaswag commented 1 month ago

You are right! I had the key value reversed in my head. The store value is actually the second and not the index.

Thanks for taking the time to adjust the nonsense.

kevdliu commented 1 month ago

You are right! I had the key value reversed in my head. The store value is actually the second and not the index.

Thanks for taking the time to adjust the nonsense.

No worries! Glad I'm not just dumb hahah.