New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 734318 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

hterm: SCS: GR mappings are all broken

Project Member Reported by vapier@chromium.org, Jun 17 2017

Issue description

since the introduction of SCS support, GR maps have been broken:
  https://chromium-review.googlesource.com/25599

basically this bit of code:

hterm.VT.CharacterMap.prototype.reset = function(glmap) {
  // Set the the GL mapping.
  this.glmap = glmap;

  var glkeys = Object.keys(this.glmap).map(function(key) {
      return '\\x' + lib.f.zpad(key.charCodeAt(0).toString(16));
    });

  this.glre = new RegExp('[' + glkeys.join('') + ']', 'g');

  // Compute the GR mapping.
  // This is the same as GL except all keys have their MSB set.
  this.grmap = {};

  glkeys.forEach(function(glkey) {
      var grkey = String.fromCharCode(glkey.charCodeAt(0) & 0x80);
      this.grmap[grkey] = this.glmap[glkey];
    }.bind(this));

glmap looks like: {'a': 'A', 'b': 'B'}
glkeys looks like: '\\x61\\x62'  (i.e. it's 8 bytes -- those are literal \ inn there)

so the code building up grmap is supposed to produce:
  {'\xe1': 'A', '\xe2': 'B'}
but it ends up always producing {}

this is for two reasons:
(1) it's supposed to walk Object.keys(this.glmap) (e.g. ['a', 'b']), but it walks glkeys (e.g. ['\\x61', '\\x62']).  this means the glkey.charCodeAt(0) always returns '\\'.charCodeAt(0) == 92.
(2) it's supposed to set the eight bit (x|0x80), but instead it masks it (x&0x80).  and since we always do (92&0x80), grkey ends up always being 0.

i fixed this logic like so:
  Object.keys(this.glmap).forEach(function(glkey) {
      var grkey = String.fromCharCode(glkey.charCodeAt(0) | 0x80);
      this.grmap[grkey] = this.glmap[glkey];
    }.bind(this));

but this ended up causing some tests to fail.  this is because our translation logic makes a bad assumption -- namely that nothing in GL's output would ever be in GR's input.  this code:
hterm.VT.prototype.parseUnknown_ = function(parseState) {
  var self = this;

  function print(str) {
    if (self[self.GL].GL)
      str = self[self.GL].GL(str);

    if (self[self.GR].GR)
      str = self[self.GR].GR(str);

    self.terminal.print(str);
  };

in the case of Finnish, we see that this is not the case:
  glmap = {
      '\x5c': '\u00d6',  // \ -> 'O' umlaut
      '\x5e': '\u00dc',  // ~ -> 'U' umlaut
  }
  grmap = {
      '\xdc': '\u00d6',
      '\xde': '\u00dc',
  }

so GL('\x5e') -> '\xdc', and GR('\xdc') -> '\xd6'.  our charsets test actually caught this :).

fixing this is feasible, but would require a bit of rework for how we handle maps.  specifically, whenever hterm.VT.{GL,GR} get updated, we'd create a single regex that merges the GL.glre and the GR.grre, and then call that once in the print func.  something like:
  this.glgrre = new RegExp('[' + this.GL.glchars.join('') + this.GR.grchars.join('') + ']', 'g');
  self.GLGR = (str) =>
      str.replace(this.glgrre, (ch) => ch.charCodeAt(0) < 0x80 ? this.GL.GL(ch) : this.GR.GR(ch));
...
  function print(str) {
    if (self.GLGR)
      str = self.GLGR(str);
    self.terminal.print(str);
  };

obviously this is doable.  but, considering GR maps have been 100% non-functional since they were introduced ~5 years ago, and no one has noticed or complained, and we're a modern emulator that focuses on UTF8 (so mangling 8-bit data in transit which could be partial UTF8 sequences e.g. codeunits), i propose we simply say GR maps are unsupported and scrub the codebase of all their support.
 

Comment 1 by vapier@chromium.org, Jun 20 2017

Owner: vapier@chromium.org
Status: Fixed (was: Available)
dropped now for hterm-1.66+

Sign in to add a comment