Project

General

Profile

Actions

Bug #5794

closed

Apply button can go missing when the UI is in a non-English language

Added by Phillip Davis almost 6 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Web Interface
Target version:
Start date:
01/22/2016
Due date:
% Done:

100%

Estimated time:
Plus Target Version:
Release Notes:
Affected Version:
2.3
Affected Architecture:

Description

guiconfig.inc print_info_box_np() has some code to help "guess" if the apply button needs to be shown. It does stuff like:
if (stristr($msg, gettext("apply")) != false) ...

If the particular $msg has a translation in the current language, and so does the string "apply" then (hopefully) all is good - the translated $msg string will (likely) contain the 1-word translation of "apply".

But if the particular $msg does not yet have a translation to the selected language, then it will remain in English and have the English string "apply" in it. Similar problem if $msg has a translation but the single word "apply" does not.

1) A "kludge" patch to what is already a kludge is to check $msg for both "apply" and gettext("apply") - I will make a PR for that which should work-around the issue.

2) IMHO the proper solution is that all callers that want the Apply button shown should specify it the call - there is already a $showapply parameter. Then we can get rid of the whole business of testing $msg against gettext("apply")...
I am happy to do (2) if people are happy that is the way to go for the "final solution".

Actions #1

Updated by Steve Beaver almost 6 years ago

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.

Actions #2

Updated by Steve Beaver almost 6 years ago

  • Status changed from New to Feedback
Actions #3

Updated by Phillip Davis almost 6 years ago

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.

Actions #4

Updated by Phillip Davis almost 6 years ago

  • % Done changed from 0 to 100
Actions #5

Updated by Phillip Davis almost 6 years ago

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().

Actions #6

Updated by Phillip Davis almost 6 years ago

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

Actions #7

Updated by Phillip Davis almost 6 years ago

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.

Actions #8

Updated by Steve Beaver over 5 years ago

  • Status changed from Feedback to Resolved
Actions

Also available in: Atom PDF