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

Alexgithublab: change time, 3.0 (supersedes #1737) #1748

Merged

Conversation

JonathonHall-Purism
Copy link
Collaborator

@JonathonHall-Purism JonathonHall-Purism commented Aug 6, 2024

Nice work on this under #1737 @alexgithublab and @tlaurion . I made some improvements:

UX

  • Improve prompt wording when entering fields
  • get_date: Complete input with enter, allow Backspace, don't require leading zeros
    • Not allowing backspace is frustrating if you make a typo, and worse that it actually inputs a character 😅
    • Moving on without requiring Enter is surprising, and if you press Enter quickly expecting it to be required, you skipped the next prompt inadvertently
    • Users with imperfect typing might double a digit unintentionally, it's really frustrating to have to complete the steps and set a wrong time since you couldn't edit. (TBH this happens to me, occasionally my hands twitch, this is important for accessibility)
  • get_date: Don't offer minimum field value by default
    • It's not useful very often, you're only on minute 0 for 1/60 of the time 😛
    • Pressing '0[enter]' is intuitive and no longer requires '00[enter]'
    • Eliminates a lot of wordiness that would have to be read (or realistically, ignored)
  • Ask whether to retry rather than forcing to repeat until it works
  • Improve wording of TOTP/HOTP mismatch prompt (show current time, suggest what to do, actually include a question since it's yes/no :) )

Scripting

  • Use named variables in get_date (clearer implementation and helps document usage)
  • Test exit status of date directly rather than trying to parse stdout/stderr
  • Infer digits from length of maximum value
  • Loop instead of recurse
  • Add .sh

Approved changes of UX, new prompt looks like
2024-08-06-142516

alexgithublab and others added 14 commits July 29, 2024 10:12
Signed-off-by: Thierry Laurion <insurgo@riseup.net>
Signed-off-by: Thierry Laurion <insurgo@riseup.net>
Signed-off-by: Thierry Laurion <insurgo@riseup.net>
Signed-off-by: Thierry Laurion <insurgo@riseup.net>

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
…ove unneeded duplicate menu options to change system time

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
Use local named variables instead of $1-$4 throughout the function.
This makes the implementation clearer and documents the usage.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
…e leading zeroes

Allow Backspace in input.  It's really frustrating otherwise if a typo
cannot be corrected, and worse, the backspace key actually produces a
character that becomes part of the input.

Complete input with Enter.  It is surprising when the script just
moves on right away once a fourth/second digit is entered, and worse,
users expecting to press Enter could reasonably press it before
realizing the script did not require it, which then skips the _next_
prompt inadvertently.  Users with imperfect typing might double a
digit unintentionally, do not force them to proceed with an incorrect
value.

Removing '-n $digits' from read does both of those.  Add '-r' so
backslashes do not have unexpected behavior.

Don't require leading zeroes, zero-pad automatically.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
There's no need to try to parse stdout/stderr to figure out if date
succeeded, just check if it was successful directly.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
There's little value to offering the minimum field value as a default
IMO:
* it's rarely accurate (e.g. minute 00 is only accuate 1/60 of the time)
* it's very obvious to just press '0'<enter> instead (and no longer
  needs to be '00')
* it eliminates a lot of wordiness you otherwise have to read (or more
  likely, ignore)

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
Infer digits from the length of the maximum value.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
…of recurse

Ask whether to retry instead of always retrying, so users can escape
if there is a problem setting the date instead of being forced to enter
values until it works.

Ask to press Enter instead of "any key".  "Any key" prompts are
generally misleading, because there are usually keys that won't
actually work (e.g. Ctrl, Caps Lock, Shift).

Loop to retry if setting the date fails instead of recursing.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
Adjust prompt wording when entering fields.  Technically the desired
value isn't always between min/max, because min and max are also
acceptable :)

No need to repeat an incorrect value, it is right there on the screen
and it dilutes the important point describing what value is needed.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
We're trying to move all shell scripts to including '.sh' to
differentiate them from functions.  While it's not 100% consistent yet,
do it for new scripts.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
This was a yes/no prompt but didn't actually have a question in it,
ask if the user wants to change the time.

Include the current time so the user can tell if it's correct.
Mention that if it's incorrect they should change the time and check
again.

The first line was too long for fbwhiptail by a few characters, trim it
a little.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

Will run and take screenshots in next hours (I think this is good habit to take when we change UX)

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

@JonathonHall-Purism unfortunately inconsistent !

2024-08-06-111215

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

@JonathonHall-Purism Also, current date should be given in expected format so user knows approximately which offset he needs to work agaist to resolve current time drift causing TOTP mismatch as said under #1737 (comment) for @alexgithublab to address.

@JonathonHall-Purism
Copy link
Collaborator Author

@JonathonHall-Purism unfortunately inconsistent !

Wow, nice catch on the octal shenanigans, will fix that 💯

@JonathonHall-Purism
Copy link
Collaborator Author

@JonathonHall-Purism Also, current date should be given in expected format so user knows approximately which offset he needs to work agaist to resolve current time drift causing TOTP mismatch as said under #1737 (comment) for @alexgithublab to address.

What would you suggest? The current prompt displays the numbers in the same order that we prompt for them.

E.g. it says: `The current UTC time is: 2024-08-06 16:33:16 UTC"

You would enter:

  • 2024
  • 08
  • 06
  • 16
  • 33
  • 16

I suppose there's a redundant "UTC" there that's not needed. Is there something else you'd suggest to change?

printf was interpreting these as invalid octal numbers, they're
decimal.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
The time zone in Heads is always UTC and we mentioned that in the text.
Don't repeat it.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

@JonathonHall-Purism Also, current date should be given in expected format so user knows approximately which offset he needs to work agaist to resolve current time drift causing TOTP mismatch as said under #1737 (comment) for @alexgithublab to address.

What would you suggest? The current prompt displays the numbers in the same order that we prompt for them.

E.g. it says: `The current UTC time is: 2024-08-06 16:33:16 UTC"

You would enter:

  • 2024
  • 08
  • 06
  • 16
  • 33
  • 16

I suppose there's a redundant "UTC" there that's not needed. Is there something else you'd suggest to change?

That would be perfect, nothing more needed, just a reminder of the time just prior of entering the time in UTC format (normally its minutes and seconds that are problematic with time drift if CMOS battery not garbage), which most end users will need to fix manually. So simple timestamp is fine. Testing last changes and outputting UX changes and report if ok.

If ok, will merge.

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

@JonathonHall-Purism just a timestamp reminder where %Z is already giving UTC would be fine, but missing

2024-08-06-134205

@JonathonHall-Purism
Copy link
Collaborator Author

JonathonHall-Purism commented Aug 6, 2024

@tlaurion You want that in The system date has been successfully set to <time> *UTC*, right?

Nevermind clarified below, we're adding a reminder of the system's configured time when setting the time

initrd/bin/gui-init Show resolved Hide resolved
initrd/bin/change-time.sh Outdated Show resolved Hide resolved
Show the system time when starting to change the time.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

Approved changes of UX, new prompt looks like
2024-08-06-142516

@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

@JonathonHall-Purism I took liberty to add UX screenshot in OP. Merging

@tlaurion tlaurion merged commit c5e449d into linuxboot:master Aug 6, 2024
2 of 4 checks passed
@tlaurion
Copy link
Collaborator

tlaurion commented Aug 6, 2024

@JonathonHall-Purism @alexgithublab thanks! This improves TOTP mismatch manual resolution a lot (setting date+time manually reducing friction)! Merged!

@alexgithublab
Copy link
Contributor

@JonathonHall-Purism @tlaurion No problem, I'm happy to help and thank you for improving my code.

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.

3 participants