Add wrappers around whiptail #273

lurch · 2025-02-18T13:36:37Z

Fairly big change, so I've tried to break it down into manageable commits. I've tested this, and it all seems to work okay, but please review it carefully.

…d reduced boilerplate

…reduced boilerplate

…petition

gordoste · 2025-02-27T06:55:58Z

Given the maintainer seems to have limited time, I would recommend keeping the PR to just "what it says on the tin". Create separate PRs for each different thing so that they can prioritise. The "don't display composite-video on Pi400/500" seems much more valuable than refactoring IMHO - separating them allows the maintainer to decide without having to selectively pull commits.

lurch · 2025-02-27T09:33:22Z

"The maintainer" sits just a couple of desks away from me 😂 This isn't a high priority PR, I'm happy to wait until he has time to review it.

The "don't display composite-video on Pi400/500" seems much more valuable than refactoring IMHO

Yeah, ironically that was the original motivation for this whole PR, but I didn't want to add more special-case branches to:

do_display_menu() {
  if is_pi ; then
    if is_wayland; then
      if is_pifour; then
        FUN=$(whiptail --title "Raspberry Pi Software Configuration Tool (raspi-config)" --menu "Display Options" $WT_HEIGHT $WT_WIDTH $WT_MENU_HEIGHT --cancel-button Back --ok-button Select \
          "D2 Screen Blanking" "Enable/disable screen blanking" \
          "D4 Composite" "Enable/disable composite output" \
          "D5 4Kp60 HDMI" "Enable 4Kp60 resolution on HDMI0" \
          "D6 Onscreen Keyboard" "Enable on-screen keyboard" \
          "D7 Keyboard Output" "Select monitor used for on-screen keyboard" \
          3>&1 1>&2 2>&3)
      else
        FUN=$(whiptail --title "Raspberry Pi Software Configuration Tool (raspi-config)" --menu "Display Options" $WT_HEIGHT $WT_WIDTH $WT_MENU_HEIGHT --cancel-button Back --ok-button Select \
          "D2 Screen Blanking" "Enable/disable screen blanking" \
          "D4 Composite" "Enable/disable composite output" \
          "D6 Onscreen Keyboard" "Enable on-screen keyboard" \
          "D7 Keyboard Output" "Select monitor used for on-screen keyboard" \
          3>&1 1>&2 2>&3)
      fi
    else
      if is_pifour; then
        FUN=$(whiptail --title "Raspberry Pi Software Configuration Tool (raspi-config)" --menu "Display Options" $WT_HEIGHT $WT_WIDTH $WT_MENU_HEIGHT --cancel-button Back --ok-button Select \
          "D1 Underscan" "Remove black border around screen" \
          "D2 Screen Blanking" "Enable/disable screen blanking" \
          "D3 VNC Resolution" "Set resolution for headless use" \
          "D4 Composite" "Enable/disable composite output" \
          "D5 4Kp60 HDMI" "Enable 4Kp60 resolution on HDMI0" \
          3>&1 1>&2 2>&3)
      else
        FUN=$(whiptail --title "Raspberry Pi Software Configuration Tool (raspi-config)" --menu "Display Options" $WT_HEIGHT $WT_WIDTH $WT_MENU_HEIGHT --cancel-button Back --ok-button Select \
          "D1 Underscan" "Remove black border around screen" \
          "D2 Screen Blanking" "Enable/disable screen blanking" \
          "D3 VNC Resolution" "Set resolution for headless use" \
          "D4 Composite" "Enable/disable composite output" \
          3>&1 1>&2 2>&3)
      fi
    fi
  else
    FUN=$(whiptail --title "Raspberry Pi Software Configuration Tool (raspi-config)" --menu "Display Options" $WT_HEIGHT $WT_WIDTH $WT_MENU_HEIGHT --cancel-button Back --ok-button Select \
      "D1 Underscan" "Remove black border around screen" \
      "D2 Screen Blanking" "Enable/disable screen blanking" \
      3>&1 1>&2 2>&3)
  fi
  RET=$?
  if [ $RET -eq 1 ]; then
    return 0
	@@ -3850,28 +3944,25 @@ do_display_menu() {
      D5\ *) do_pi4video ;;
      D6\ *) do_squeekboard ;;
      D7\ *) do_squeek_output ;;
      *) whiptail --msgbox "Programmer error: unrecognized option" 20 60 1 ;;
    esac || whiptail --msgbox "There was an error running option $FUN" 20 60 1
  fi
}

...which is what started me on this refactoring journey 😉

spl237 · 2025-03-06T09:29:41Z

raspi-config

  # Escape special characters for embedding in regex below
  ssid="$(echo "$SSID" \
-   | sed 's;\\;\\\\;g' \
-   | sed -e 's;\.;\\\.;g' \


Is this definitely functionally identical? I didn't write the original code here, but it looks as if the two sed statements do different things, and by splitting them like this, the first will definitely happen before the second. I don't know if combining them like this will ensure that that still happens?

I'd be very surprised if sed 's/a/b/' | sed 's/c/d/'acted differently to sed -e 's/a/b/' -e 's/c/d/', but out of an abundance of caution I've done some testing, and it indeed appears to work as I expected:

$ echo "ffbfbfbb" | sed 's;f;F;g' | sed 's;b;B;g' FFBFBFBB $ echo "ffbfbfbb" | sed -e 's;f;F;g' -e 's;b;B;g' FFBFBFBB $ echo "start \\bslash\\ .dot. *asterisk* +plus+ ?question? ^caret^ \$dollar\$ /fslash/ [ls[ ]rs] {lb{ }rb} (lp( )rp) \"dquote\" end" start \bslash\ .dot. *asterisk* +plus+ ?question? ^caret^ $dollar$ /fslash/ [ls[ ]rs] {lb{ }rb} (lp( )rp) "dquote" end $ echo "start \\bslash\\ .dot. *asterisk* +plus+ ?question? ^caret^ \$dollar\$ /fslash/ [ls[ ]rs] {lb{ }rb} (lp( )rp) \"dquote\" end" | sed 's;\\;\\\\;g' | sed -e 's;\.;\\\.;g' -e 's;\*;\\\*;g' -e 's;\+;\\\+;g' -e 's;\?;\\\?;g' -e 's;\^;\\\^;g' -e 's;\$;\\\$;g' -e 's;\/;\\\/;g' -e 's;\[;\\\[;g' -e 's;\];\\\];g' -e 's;{;\\{;g' -e 's;};\\};g' -e 's;(;\$;g' -e 's;);\$;g' -e 's;";\\\\\";g' start \\bslash\\ \.dot\. \*asterisk\* \+plus\+ \?question\? \^caret\^ \$dollar\$ \/fslash\/ \[ls\[ \]rs\] \{lb\{ \}rb\} $lp\( $rp\) \\"dquote\\" end $ echo "start \\bslash\\ .dot. *asterisk* +plus+ ?question? ^caret^ \$dollar\$ /fslash/ [ls[ ]rs] {lb{ }rb} (lp( )rp) \"dquote\" end" | sed -e 's;\\;\\\\;g' -e 's;\.;\\\.;g' -e 's;\*;\\\*;g' -e 's;\+;\\\+;g' -e 's;\?;\\\?;g' -e 's;\^;\\\^;g' -e 's;\$;\\\$;g' -e 's;\/;\\\/;g' -e 's;\[;\\\[;g' -e 's;\];\\\];g' -e 's;{;\\{;g' -e 's;};\\};g' -e 's;(;\$;g' -e 's;);\$;g' -e 's;";\\\\\";g' start \\bslash\\ \.dot\. \*asterisk\* \+plus\+ \?question\? \^caret\^ \$dollar\$ \/fslash\/ \[ls\[ \]rs\] \{lb\{ \}rb\} $lp\( $rp\) \\"dquote\\" end

raspi-config

spl237 · 2025-03-06T09:42:45Z

raspi-config


 do_advanced_menu() {
-  if gpu_has_mmu ; then
+  if is_pifour ; then


I don't think gpu_has_mmu is the same as is_pifour - gpu_has_mmu should select both Pi 4 and Pi 5, IIRC - we introduced that test name to keep the existence of Pi 5 secret prior to release. I don't think changing this test is correct.

I split it into separate tests for Pi 4 and Pi 5 (rather than showing the same menu for both), because AFAIK the "Set PCIe x1 port speed" option is only applicable for Pi 5, and doesn't do anything on Pi 4?

spl237 · 2025-03-06T09:59:21Z

raspi-config

  WT_WIDTH=$(tput cols)
+  WT_BOX_HEIGHT=20
+  WT_BOX_WIDTH=60
+  WT_BOX_WIDTH_WIDE=70


Is it actually necessary to have two different widths, given they differ by so little? Might be tidier to just have one?

I only did that for backwards-feature-parity with the existing code - happy to unify it to just a single width, if you let me know which one you'd like me to use 🙂 (I think when I tested this, some of the text in the "wider" dialogs wrapped weirdly when given a width of 60, which is why I added the "wide" variant.)

I'd suggest using whichever is least risky - presumably the wider one - and testing to make sure it is still ok with the other messages. I'd assume the narrower one was historically the default but at some point it was found to cause problems with one or two particular messages.

spl237 · 2025-03-06T10:13:20Z

OK, I've now read through all of this (I think...) and noted the specific issues as commented above.

Overall, this is a really big set of changes to the script, albeit ones which have a negligible change on the actual functionality. That sort of thing makes me instinctively wary, as I am reluctant to change things which are working - that said, I can see the sense in a lot of this, but it is difficult to be able to be sure that everything is correct when it all hangs on a single merge.

There are parts of this which are completely uncontentious, like the whiptail wrapper functions - those to me look low-risk and a good improvement to the code. There are other parts, like the menu generation code, which are going to require some very careful review to make sure nothing has broken in the process.

As a result, can I please suggest that this one PR is broken up into a few separate ones which can be reviewed and merged independently? As a suggestion, these should be at a minimum (and not necessarily in this order):

Whiptail wrappers
Menu generation
Composite video change
Any other changes to functionality

And possibly even further subdivided if it makes sense to do so. If we are going to do this - and as above, I can see the sense of a lot of it - I'd like to have the riskier stuff separated out from the non-contentious stuff to make sure that we can break this into bite-size chunks rather than having to accept a single monolithic merge. Many thanks.

lurch · 2025-03-06T10:35:09Z

I'd like to have the riskier stuff separated out from the non-contentious stuff to make sure that we can break this into bite-size chunks rather than having to accept a single monolithic merge.

Makes sense, I'll attempt to do that 👍 (need to put my "advanced git" hat back on again 🧙‍♂️ )

albeit ones which have a negligible change on the actual functionality.

That was deliberate - I didn't want to break/change existing functionality 🙂

lurch added 10 commits February 13, 2025 16:08

Whitespace tidyups

e191c72

Cache values which can't change, for more performant menus

6a2aa44

Logic tweaks

afe7bcc

Wrap whiptail --msgbox in a wt_msgbox function for consistency an…

b5a1b4c

…d reduced boilerplate

Wrap whiptail --yesno in a wt_yesno function for consistency and …

11acddc

…reduced boilerplate

Add wt_inputbox and wt_passwordbox wrapper functions

ff7a252

Add wt_menu, wt_sub_menu and wt_top_menu wrapper functions

31cf244

Some small shellcheck fixes

862464e

Build some of the menus dynamically, for more flexibility and less re…

5e92a7a

…petition

Don't display composite-video option on Pi 400 or Pi 500

dc784b1

lurch changed the title ~~Wrappers~~ Add wrappers around whiptail Feb 18, 2025

spl237 reviewed Mar 6, 2025

View reviewed changes

raspi-config Show resolved Hide resolved

spl237 reviewed Mar 6, 2025

View reviewed changes

raspi-config Show resolved Hide resolved

spl237 reviewed Mar 6, 2025

View reviewed changes

Add wrappers around whiptail #273

Are you sure you want to change the base?

Add wrappers around whiptail #273

Uh oh!

Conversation

lurch commented Feb 18, 2025

Uh oh!

gordoste commented Feb 27, 2025

Uh oh!

lurch commented Feb 27, 2025

Uh oh!

spl237 Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

lurch Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spl237 Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

lurch Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

spl237 Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

lurch Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

spl237 Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

spl237 commented Mar 6, 2025

Uh oh!

lurch commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants