[PATCH] Make sure TUI is readable for non-latin languages (#1182562)

Martin Kolman mkolman at redhat.com
Wed Jul 22 11:51:59 UTC 2015


On Mon, 2015-07-20 at 09:28 +0200, Vratislav Podzimek wrote:
> On Fri, 2015-07-17 at 17:05 +0200, Martin Kolman wrote:
> > Make sure the currently selected TUI language is displayable
> > in the Linux console by checking its scripts, as reported by
> > langtable.list_scripts()).
> > (Currently supported scripts: 'Latn', 'Cyrl', 'Grek')
> > 
> > If the language is not displayable tehn fall back to English.
> > 
> > We also need to handle some edge cases such as Anaconda falling 
> > back
> > to TUI after failed GUI startup and missing or corrupted console 
> > fonts.
> > 
> > Resolves: rhbz#1182562
> > Signed-off-by: Martin Kolman <mkolman at redhat.com>
> > ---
> >  anaconda                            |  13 +++-
> >  anaconda.spec.in                    |   2 +-
> >  pyanaconda/anaconda.py              |   1 +
> >  pyanaconda/localization.py          | 142 
> > ++++++++++++++++++++++++++++++++++--
> >  pyanaconda/ui/gui/spokes/welcome.py |   4 +-
> >  5 files changed, 151 insertions(+), 11 deletions(-)
> Generally looks good to me, but I have a few (inline) comments below. 
> In
> particular, I'd "degrade" all log messages.
> 
> > 
> > diff --git a/anaconda b/anaconda
> > index 7b5d386..b868ac3 100755
> > --- a/anaconda
> > +++ b/anaconda
> > @@ -681,6 +681,9 @@ def setupDisplay(anaconda, options, 
> > addons=None):
> >  
> >      anaconda.instClass.configure(anaconda)
> >  
> > +    # report if starting the GUI failed
> > +    anaconda.gui_startup_failed = bool(graphical_failed)
> > +
> >  def prompt_for_ssh():
> >      # Do some work here to get the ip addr / hostname to pass
> >      # to the user.
> > @@ -1122,7 +1125,7 @@ if __name__ == "__main__":
> >      # first, try to load firmware language if nothing is already 
> > set in
> >      # the environment
> >      if "LANG" not in os.environ:
> > -        localization.load_firmware_language(ksdata.lang)
> > +        localization.load_firmware_language(ksdata.lang, 
> > anaconda.displayMode != "g")
> >  
> >      # If command line options or kickstart set LANG, override the 
> > environment
> >      if opts.lang:
> > @@ -1145,7 +1148,7 @@ if __name__ == "__main__":
> >          log.error("Invalid locale '%s' given on command line or in 
> > kickstart", os.environ["LANG"])
> >          os.environ["LANG"] = constants.DEFAULT_LANG # pylint: 
> > disable=environment-modify
> >  
> > -    localization.setup_locale(os.environ["LANG"], ksdata.lang)
> > +    localization.setup_locale(os.environ["LANG"], ksdata.lang, 
> > anaconda.displayMode != "g")
> >  
> >      import blivet
> >      blivet.enable_installer_mode()
> > @@ -1158,6 +1161,12 @@ if __name__ == "__main__":
> >  
> >      # now start the interface
> >      setupDisplay(anaconda, opts, addon_paths)
> > +    if anaconda.gui_startup_failed:
> > +        # we need to reinitialize the locale if GUI startup 
> > failed,
> > +        # as we might now be in text mode, which might not be able 
> > to display
> > +        # the characters from our current locale
> > +        log.warning("reinitializing locale due to failed attempt 
> > to start the GUI")
> > +        localization.setup_locale(os.environ["LANG"], ksdata.lang, 
> > anaconda.displayMode != "g")
> >  
> >      # we now know in which mode we are going to run so store the 
> > information
> >      from pykickstart.constants import DISPLAY_MODE_GRAPHICAL, 
> > DISPLAY_MODE_CMDLINE, DISPLAY_MODE_TEXT
> > diff --git a/anaconda.spec.in b/anaconda.spec.in
> > index a698c24..37a46dd 100644
> > --- a/anaconda.spec.in
> > +++ b/anaconda.spec.in
> > @@ -38,7 +38,7 @@ Source0: %{name}-%{version}.tar.bz2
> >  %define iscsiver 6.2.0.870-3
> >  %define rpmver 4.10.0
> >  %define libarchivever 3.0.4
> > -%define langtablever 0.0.18-1
> > +%define langtablever 0.0.31-3
> >  %define libxklavierver 5.4
> >  %define libtimezonemapver 0.4.1-2
> >  
> > diff --git a/pyanaconda/anaconda.py b/pyanaconda/anaconda.py
> > index 6baf48a..8e9e777 100644
> > --- a/pyanaconda/anaconda.py
> > +++ b/pyanaconda/anaconda.py
> > @@ -51,6 +51,7 @@ class Anaconda(object):
> >          self.desktop = desktop.Desktop()
> >          self.dir = None
> >          self.displayMode = None
> > +        self.gui_startup_failed = False
> >          self.id = None
> >          self._instClass = None
> >          self._intf = None
> > diff --git a/pyanaconda/localization.py 
> > b/pyanaconda/localization.py
> > index 30112f9..5a4659f 100644
> > --- a/pyanaconda/localization.py
> > +++ b/pyanaconda/localization.py
> > @@ -29,13 +29,15 @@ import glob
> >  from collections import namedtuple
> >  
> >  from pyanaconda import constants
> > -from pyanaconda.iutil import upcase_first_letter, setenv
> > +from pyanaconda.iutil import upcase_first_letter, setenv, 
> > execWithRedirect
> >  
> >  import logging
> >  log = logging.getLogger("anaconda")
> >  
> >  LOCALE_CONF_FILE_PATH = "/etc/locale.conf"
> >  
> > +SCRIPTS_SUPPORTED_BY_CONSOLE = {'Latn', 'Cyrl', 'Grek'}
> > +
> >  #e.g. 'SR_RS.UTF-8 at latin'
> >  LANGCODE_RE = re.compile(r'(?P<language>[A-Za-z]+)'
> >                           r'(_(?P<territory>[A-Za-z]+))?'
> > @@ -92,6 +94,24 @@ def is_supported_locale(locale):
> >      en_name = get_english_name(locale)
> >      return bool(en_name)
> >  
> > +def locale_supported_in_console(locale):
> > +    """
> > +    Function that tells if the given locale can be displayed by 
> > the Linux console.
> > +
> > +    The Linux console can display Latin, Cyrillic and Greek 
> > characters reliably,
> > +    but others such as Japanese, can't be correctly installed.
> > +
> > +    :param locale: locale to test
> > +    :type locale: str
> The type could be moved to the :param... line.
OK. :)
> 
> > +    :return: whether the given locale is supported by the console 
> > or not
> > +    :rtype: bool
> > +    :raise InvalidLocaleSpec: if an invalid locale is given (see 
> > LANGCODE_RE)
> > +
> > +    """
> > +
> > +    locale_scripts = get_locale_scripts(locale)
> > +    return 
> > bool(set(locale_scripts).issubset(SCRIPTS_SUPPORTED_BY_CONSOLE))
> I'd expect issubset() to return a boolean value. Is that really not 
> the
> case? If it is, please drop the bool() call.
Yep, you are of course right - dropping the bool() call.
> 
> > +
> >  def langcode_matches_locale(langcode, locale):
> >      """
> >      Function that tells if the given langcode matches the given 
> > locale. I.e. if
> > @@ -181,7 +201,7 @@ def find_best_locale_match(locale, langcodes):
> >      else:
> >          return None
> >  
> > -def setup_locale(locale, lang=None):
> > +def setup_locale(locale, lang=None, text_mode=False):
> >      """
> >      Procedure setting the system to use the given locale and store 
> > it in to the
> >      ksdata.lang object (if given). DOES NOT PERFORM ANY CHECKS OF 
> > THE GIVEN
> > @@ -190,9 +210,14 @@ def setup_locale(locale, lang=None):
> >      $LANG must be set by the caller in order to set the language 
> > used by gettext.
> >      Doing this in a thread-safe way is up to the caller.
> >  
> > +    We also try to set a proper console font for the locale in 
> > textmode.
> ^ text mode (two words)
Fixing.
> 
> > +    If the font for the locale can't be displayed in the Linux 
> > console,
> > +    we fall back to the English locale.
> > +
> >      :param locale: locale to setup
> >      :type locale: str
> >      :param lang: ksdata.lang object or None
> > +    :param bool text_mode: if the locale is being setup for 
> > textmode
> >      :return: None
> >      :rtype: None
> >  
> > @@ -201,8 +226,55 @@ def setup_locale(locale, lang=None):
> >      if lang:
> >          lang.lang = locale
> >  
> > -    setenv("LANG", locale)
> > -    locale_mod.setlocale(locale_mod.LC_ALL, locale)
> > +    # not all locales might be displayable in text mode
> > +    if text_mode:
> > +        # check if the script corresponding to the locale/language
> > +        # can be displayed by the Linux console
> > +        # * all scripts for the given locale/language need to be
> > +        #   supported by the linux console
> > +        # * otherwise users might get a screen full of white 
> > rectangles
> > +        #   (also known as "tofu") in text mode
> > +        # then we also need to check if we have information about 
> > what
> > +        # font to use for correctly displaying the given 
> > language/locale
> > +
> > +        script_supported = locale_supported_in_console(locale)
> > +        log.info("scripts found for locale %s: %s", locale, 
> > get_locale_scripts(locale))
> > +
> > +        console_fonts = get_locale_console_fonts(locale)
> > +        log.info("console fonts found for locale %s: %s", locale, 
> > console_fonts)
> These should be turned into debug messages, I think.
OK!
> 
> > +
> > +        font_set = False
> > +        if script_supported and console_fonts:
> > +            # set the locale
> > +            setenv("LANG", locale)
> > +            locale_mod.setlocale(locale_mod.LC_ALL, locale)
> This change may be overridden by the 'if not font_set' branch below. 
> See
> the comment at the end of this function for the suggestion. 
> 
> > +            # try to set console font
> > +            for font in console_fonts:
> > +                if set_console_font(font):
> > +                    # console font set successfully, skip the rest
> > +                    font_set = True
> > +                    break
> > +
> > +        if not font_set:
> > +            log.error("can't set console font for locale %s", 
> > locale)
> > +            # report what exactly went wrong
> > +            if not(script_supported):
> > +                log.error("script not supported by console for 
> > locale %s", locale)
> > +            if not(console_fonts):  # no fonts known for locale
> > +                log.error("no console font found for locale %s", 
> > locale)
> > +            if script_supported and console_fonts:
> > +                log.error("none of the suggested fonts can be set 
> > for locale %s", locale)
> > +            log.error("falling back to the English locale")
> I think the above are more like warnings than errors.
Yeah, even though the issues can be quite serious (you have requested
a language, but we have found out it can't be displayed), it is less of
na issue due to the fallback to English. So I'll demote those errors to
warnings. :)

> 
> > +            default_locale = constants.DEFAULT_LANG
> > +            os.environ["LANG"] = default_locale
> > +            setenv("LANG", default_locale)
> > +            locale_mod.setlocale(locale_mod.LC_ALL, 
> > default_locale)
> > +
> > +    # we can use any locale supported by langtable in the GUI
> > +    else:
> > +        # set the locale
> > +        setenv("LANG", locale)
> > +        locale_mod.setlocale(locale_mod.LC_ALL, locale)
> Generally I'm quite a bit nervous about the amount of setenv() and
> setlocale() calls in this function, especially when thinking about 
> the
> issues we had with the *env() functions. Could you please set the
> 'locale' variable accordingly in the code branches and then do the
> setenv(), setlocale() and os.environ modifications at the very end in
> one place?
Sure, that should also make it easier to easier to comprehend that:
1) locale/env gets set only once per call
2) what gets actually set

> 
> >  
> >  def get_english_name(locale):
> >      """
> > @@ -377,6 +449,46 @@ def get_locale_territory(locale):
> >  
> >      return parts.get("territory", None)
> >  
> > +def get_locale_console_fonts(locale):
> > +    """
> > +    Function returning preferred console fonts for the given 
> > locale.
> > +
> > +    :param locale: locale string (see LANGCODE_RE)
> > +    :type locale: str
> Again, this could be reduced to ':param str locale:...'
Fixing.
> 
> > +    :return: list of preferred console fonts
> > +    :rtype: list of strings
> > +    :raise InvalidLocaleSpec: if an invalid locale is given (see 
> > LANGCODE_RE)
> > +
> > +    """
> > +
> > +    parts = parse_langcode(locale)
> > +    if "language" not in parts:
> > +        raise InvalidLocaleSpec("'%s' is not a valid locale" % 
> > locale)
> > +
> > +    return 
> > langtable.list_consolefonts(languageId=parts["language"],
> > +                                      
> >  territoryId=parts.get("territory", ""),
> > +                                      
> >  scriptId=parts.get("script", ""))
> > +
> > +def get_locale_scripts(locale):
> > +    """
> > +    Function returning preferred scripts (writing systems) for the 
> > given locale.
> > +
> > +    :param locale: locale string (see LANGCODE_RE)
> > +    :type locale: str
> Same here.
Also fixing.
> 
> > +    :return: list of preferred scripts
> > +    :rtype: list of strings
> > +    :raise InvalidLocaleSpec: if an invalid locale is given (see 
> > LANGCODE_RE)
> > +
> > +    """
> > +
> > +    parts = parse_langcode(locale)
> > +    if "language" not in parts:
> > +        raise InvalidLocaleSpec("'%s' is not a valid locale" % 
> > locale)
> > +
> > +    return langtable.list_scripts(languageId=parts["language"],
> > +                                 
> >  territoryId=parts.get("territory", ""),
> > +                                  scriptId=parts.get("script", 
> > ""))
> > +
> >  def get_xlated_timezone(tz_spec_part):
> >      """
> >      Function returning translated name of a region, city or 
> > complete timezone
> > @@ -417,7 +529,7 @@ def write_language_configuration(lang, root):
> >          msg = "Cannot write language configuration file: %s" % 
> > ioerr.strerror
> >          raise LocalizationConfigError(msg)
> >  
> > -def load_firmware_language(lang):
> > +def load_firmware_language(lang, text_mode=False):
> >      """
> >      Procedure that loads firmware language information (if any). 
> > It stores the
> >      information in the given ksdata.lang object and sets the $LANG 
> > environment
> > @@ -474,7 +586,7 @@ def load_firmware_language(lang):
> >          return
> >  
> >      log.debug("Using UEFI PlatformLang '%s' ('%s') as our 
> > language.", d, locales[0])
> > -    setup_locale(locales[0], lang)
> > +    setup_locale(locales[0], lang, text_mode)
> >  
> >      os.environ["LANG"] = locales[0] # pylint: disable=environment
> > -modify
> >  
> > @@ -563,3 +675,21 @@ def resolve_date_format(year, month, day, 
> > fail_safe=True):
> >              # if this call fails too, something is going terribly 
> > wrong and we
> >              # should be informed about it
> >              return order_terms_formats(FAIL_SAFE_DEFAULT)
> > +
> > +def set_console_font(font):
> > +    """
> > +    Try to set console font to the given value.
> > +
> > +    :param str font: console font name
> > +    :returns: True on success, False on failure
> > +    :rtype: Bool
> > +
> > +    """
> > +    log.info("setting console font to %s", font)
> > +    rc = execWithRedirect("setfont", [font])
> > +    if rc == 0:
> > +        log.info("console font set successfully to %s", font)
> These two log.info() calls should either be dropped (we can see the
> action in the program.log) or at least changed to log.debug() calls 
> as
> these are debug messages.
I opt for debug calls, so that it can be more easily correlated with
the other console font related messages in the main anaconda log.
> 
> > +        return True
> > +    else:
> > +        log.error("setting console font to %s failed", font)
> > +        return False
> > diff --git a/pyanaconda/ui/gui/spokes/welcome.py 
> > b/pyanaconda/ui/gui/spokes/welcome.py
> > index 1dbe527..91b2b3a 100644
> > --- a/pyanaconda/ui/gui/spokes/welcome.py
> > +++ b/pyanaconda/ui/gui/spokes/welcome.py
> > @@ -67,7 +67,7 @@ class WelcomeLanguageSpoke(LangLocaleHandler, 
> > StandaloneSpoke):
> >  
> >          locale = store[itr][1]
> >          self._set_lang(locale)
> > -        localization.setup_locale(locale, self.data.lang)
> > +        localization.setup_locale(locale, self.data.lang, 
> > text_mode=False)
> >  
> >          # Skip timezone and keyboard default setting for kickstart 
> > installs.
> >          # The user may have provided these values via kickstart 
> > and if not, we
> > @@ -197,7 +197,7 @@ class WelcomeLanguageSpoke(LangLocaleHandler, 
> > StandaloneSpoke):
> >          # use default
> >          if not langs_with_translations:
> >              self._set_lang(DEFAULT_LANG)
> > -            localization.setup_locale(DEFAULT_LANG, 
> > self.data.lang)
> > +            localization.setup_locale(DEFAULT_LANG, 
> > self.data.lang, text_mode=False)
> >              lang_itr, _locale_itr = 
> > self._select_locale(self.data.lang.lang)
> >              langs_with_translations[DEFAULT_LANG] = lang_itr
> >              locales = [DEFAULT_LANG]
> 


More information about the anaconda-patches mailing list