pfSense

Phil,

I don't like the "automatic Apply" thing in print_info_box_np at all. When I started work on this project I would sometimes find myself editing text strings and replacing "apply" with "make" or "add" or something, just to get rid of an unwanted "Apply" button.

I would prefer to see the button created explicitly. Perhaps something like this:

// Make an info box with an "Apply" button
print_info_box_np(gettext("Time to apply the changes we need!"), "success");

// Make an info box with a custom button
print_info_box_np(gettext("Time to apply the changes we need!"), "success", gettext("Erase"), "erase", "yes");

// Make an info box with no button
print_info_box_np(gettext("IP addresses should never contain asterisks!"), "danger", false);

print_info_box_np(gettext($displaystring, class="alert", $btnlabel=gettext("Apply"), $btnname="apply", $btnval="") {
    if ($btnlabel) {
        . . .
    }

    . . .
}

By defaulting to gettext("Apply"), we would maintain some sort of backwards compatibility perhaps?

It would be a pretty pervasive change though.

I can modify print_info_box() so it takes:

function print_info_box($msg, $class="alert-warning", $btntoshow = "close") {
    switch ($btntoshow) {
        case "close":
            $showclose = true;
            $showapply = false;
            $name = "";
            break;
        case "apply":
            $showclose = false;
            $showapply = true;
            $name = "apply";
            break;
        default:
            $showclose = false;
            $showapply = false;
            $name = "";
            break;
    }
    print_info_box_np($msg, $name, null, $showapply, $class, $showclose);
}

That lets you give the message text, severity level (info, success, warning, danger) and which button you want to show (if any). It massages the input parameters to make the equivalent call to print_info_box_np. The default values end up the same as previously, so callers that used the defaults do not need to change.

Then it is easy to modify any existing print_info_box() or print_info_box_np() call to use the new print_info_box() parameters.

I think I can then get rid of all calls to print_info_box_np() - after that happens then we can sort out what is really wanted in the underlying routines and perhaps make a single simpler print_info_box() that has bunches of code from print_info_box_np() ...

I will have a little play with a few example calls and see if it seems to work.

Sleep time for me now!
But if someone wants to re-engineer print_info_box_np() to have more nicely laid-out parameters... and change print_info_box() and the few (only 1 maybe) remaining direct calls to print_info_box_np(),
or even work out how to get rid of print_info_box_np(), put the code nicely in print_info_box() (probably needing an optional parameter(s) on the end),
then go for it.

It should no longer have far-reaching implications - because 99% of real code now calls print_info_box() or print_apply_box().

https://github.com/pfsense/pfsense/commit/fd4803d2c1c778dc00bcb5b8338b29d6cb04efd8 got rid of another remaining call to print_info_box_np()

https://github.com/pfsense/pfsense/commit/0d711d380b719219a802b2b67db4ccce6cb645d3 removes print_info_box_np() completely and finishes the implementation of the re-engineered code in print_info_box().

All dependency on various text strings in the message is now removed. This should be happily language-agnostic and working.

Please test and report any problems.