Project

General

Profile

Bug #5794

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

Added by Phillip Davis over 3 years ago. Updated over 3 years ago.

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

100%

Estimated time:
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".

Associated revisions

Revision 3b3a95e5 (diff)
Added by Phillip Davis over 3 years ago

Fix #5794 remove print_info_box_np chackes for gettext("apply")

1) Get rid of the stristr() checks to "guess" if an apply button should
be used.
2) Change print_info_box() so it can take a button name of "close"
, "apply" or none to decide which button to show.
3) Delete function print_info_box_np_undo() - nothing calls it.
4) Add new function print_apply_box() to provide an easy wrapper for
print_info_box() with the parameters to be 'warning' level and 'apply'
button.
5) Change print_info_box_np() calls to just print_info_box() or
print_apply_box() as appropriate.
There is 1 direct call to print_info_box_np() from vpn_ipsec_mobile.php
remaining. That tries to make a "create" button. It was not working
before this change. It needs to be sorted out and fixed separately.

After this change there is no dependency on a string containing text
like "apply" to make the apply button appear.

Then we can work on re-engineering the internal code of
print_info_box_np() print_info_box() and print_apply_box() to fit
together however we like. It should be easy to preserving the current
API to print_info_box() and print_apply_box().

History

#1 Updated by Steve Beaver over 3 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.

#2 Updated by Steve Beaver over 3 years ago

  • Status changed from New to Feedback

#3 Updated by Phillip Davis over 3 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.

#4 Updated by Phillip Davis over 3 years ago

  • % Done changed from 0 to 100

#5 Updated by Phillip Davis over 3 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().

#6 Updated by Phillip Davis over 3 years ago

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

#7 Updated by Phillip Davis over 3 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.

#8 Updated by Steve Beaver over 3 years ago

  • Status changed from Feedback to Resolved

Also available in: Atom PDF