Now moves cursor vertically for sixel newlines even if there is no sixel

metainfo.xml

@@ -103,6 +103,9 @@
       <description>
         <ul>
           <li>Fixes a problem with oversized glyphs being wrongly cut off (#821).</li>
+          <li>Fixes vertical cursor movement for sixel graphics with only newlines (#822)</li>
+          <li>Fixes sixel rendering for images with aspect ratios other than 1:1</li>
+					<li>Removes `images.sixel_cursor_conformance` config option</li>
         </ul>
       </description>
     </release>

src/contour/Config.cpp

@@ -1705,7 +1705,6 @@ void loadConfigFromFile(Config& _config, FileSystem::path const& _fileName)
     }
 
     tryLoadValue(usedKeys, doc, "images.sixel_scrolling", _config.sixelScrolling);
-    tryLoadValue(usedKeys, doc, "images.sixel_cursor_conformance", _config.sixelCursorConformance);
     tryLoadValue(usedKeys, doc, "images.sixel_register_count", _config.maxImageColorRegisters);
     tryLoadValue(usedKeys, doc, "images.max_width", _config.maxImageSize.width);
     tryLoadValue(usedKeys, doc, "images.max_height", _config.maxImageSize.height);

src/contour/Config.h

@@ -283,7 +283,6 @@ struct Config
     std::shared_ptr<logstore::Sink> loggingSink;
 
     bool sixelScrolling = true;
-    bool sixelCursorConformance = true;
     terminal::ImageSize maxImageSize = {}; // default to runtime system screen size.
     unsigned maxImageColorRegisters = 4096;
 

src/contour/TerminalSession.cpp

@@ -110,7 +110,7 @@ TerminalSession::TerminalSession(unique_ptr<Pty> _pty, ContourGuiApp& _app):
                 config_.bypassMouseProtocolModifier, // TODO: you too
                 config_.maxImageSize,
                 config_.maxImageColorRegisters,
-                config_.sixelCursorConformance,
+                true,
                 profile_.colors,
                 50.0,
                 config_.reflowOnResize,
@@ -1088,7 +1088,7 @@ void TerminalSession::configureTerminal()
 
     SessionLog()("Setting terminal ID to {}.", profile_.terminalId);
     terminal_.setTerminalId(profile_.terminalId);
-    terminal_.setSixelCursorConformance(config_.sixelCursorConformance);
+    terminal_.setSixelCursorConformance(true);
     terminal_.setMaxImageColorRegisters(config_.maxImageColorRegisters);
     terminal_.setMaxImageSize(config_.maxImageSize);
     terminal_.setMode(terminal::DECMode::NoSixelScrolling, !config_.sixelScrolling);

src/contour/contour.yml

@@ -125,9 +125,6 @@ images:
     sixel_scrolling: true
     # Configures the maximum number of color registers available when rendering Sixel graphics.
     sixel_register_count: 4096
-    # If enabled, the ANSI text cursor is placed at the position of the sixel graphics cursor after
-    # image rendering, otherwise (if disabled) the cursor is placed underneath the image.
-    sixel_cursor_conformance: true
     # maximum width in pixels of an image to be accepted (0 defaults to system screen pixel width)
     max_width: 0
     # maximum height in pixels of an image to be accepted (0 defaults to system screen pixel height)

src/terminal/Screen.cpp

@@ -1621,9 +1621,8 @@ void Screen<Cell>::renderImage(shared_ptr<Image const> _image,
                                ImageResize _resizePolicy,
                                bool _autoScroll)
 {
-    // TODO: make use of _imageOffset and _imageSize
+    // TODO: make use of _imageOffset
     (void) _imageOffset;
-    (void) _imageSize;
 
     auto const linesAvailable = _state.pageSize.lines - _topLeft.line.as<LineCount>();
     auto const linesToBeRendered = min(_gridSize.lines, linesAvailable);
@@ -1634,7 +1633,21 @@ void Screen<Cell>::renderImage(shared_ptr<Image const> _image,
     // TODO: make use of _imageOffset and _imageSize
     auto const rasterizedImage = _state.imagePool.rasterize(
         move(_image), _alignmentPolicy, _resizePolicy, gapColor, _gridSize, _state.cellPixelSize);
-
+    const auto lastSixelBand = _imageSize.height.value % 6;
+    const LineOffset offset = [&]() {
+        auto offset =
+            LineOffset::cast_from(std::ceil(static_cast<float>(_imageSize.height.value - lastSixelBand)
+                                            / float(*_state.cellPixelSize.height)))
+            - 1 * (lastSixelBand == 0);
+        auto const h = _imageSize.height.value - 1;
+        // VT340 has this behavior where for some heights it text cursor is placed not
+        // at the final sixel line but a line above it.
+        // See
+        // https://github.com/hackerb9/vt340test/blob/main/glitches.md#text-cursor-is-left-one-row-too-high-for-certain-sixel-heights
+        if (h % 6 > h % _state.cellPixelSize.height.value)
+            return offset - 1;
+        return offset;
+    }();
     if (*linesToBeRendered)
     {
         for (GridSize::Offset const offset: GridSize { linesToBeRendered, columnsToBeRendered })
@@ -1643,7 +1656,7 @@ void Screen<Cell>::renderImage(shared_ptr<Image const> _image,
             cell.setImageFragment(rasterizedImage, CellLocation { offset.line, offset.column });
             cell.setHyperlink(_state.cursor.hyperlink);
         };
-        moveCursorTo(_topLeft.line + unbox<int>(linesToBeRendered) - 1, _topLeft.column);
+        moveCursorTo(_topLeft.line + offset, _topLeft.column);
     }
 
     // If there're lines to be rendered missing (because it didn't fit onto the screen just yet)
@@ -1667,7 +1680,7 @@ void Screen<Cell>::renderImage(shared_ptr<Image const> _image,
         }
     }
     // move ansi text cursor to position of the sixel cursor
-    moveCursorToColumn(_topLeft.column + _gridSize.columns.as<ColumnOffset>());
+    moveCursorToColumn(_topLeft.column);
 }
 
 template <typename Cell>

src/terminal/Screen_test.cpp

@@ -3372,8 +3372,8 @@ TEST_CASE("Sixel.simple", "[screen]")
 
     mock.writeToScreen(sixelData);
 
-    CHECK(mock.terminal.primaryScreen().cursor().position.column == ColumnOffset(8));
-    CHECK(mock.terminal.primaryScreen().cursor().position.line == LineOffset(4));
+    CHECK(mock.terminal.primaryScreen().cursor().position.column == ColumnOffset(0));
+    CHECK(mock.terminal.primaryScreen().cursor().position.line == LineOffset(5));
 
     for (auto line = LineOffset(0); line < boxed_cast<LineOffset>(pageSize.lines); ++line)
     {
@@ -3410,7 +3410,7 @@ TEST_CASE("Sixel.AutoScroll-1", "[screen]")
 
     mock.writeToScreen(sixelData);
 
-    CHECK(mock.terminal.primaryScreen().cursor().position.column == ColumnOffset(8));
+    CHECK(mock.terminal.primaryScreen().cursor().position.column == ColumnOffset(0));
     CHECK(mock.terminal.primaryScreen().cursor().position.line == LineOffset(3));
 
     for (auto line = LineOffset(-1); line < boxed_cast<LineOffset>(pageSize.lines); ++line)

src/terminal/SixelParser.cpp

@@ -412,13 +412,19 @@ void SixelImageBuilder::write(CellLocation const& _coord, RGBColor const& _value
                 size_.width = Width::cast_from(_coord.column + 1);
         }
 
-        auto const base =
-            unbox<unsigned>(_coord.line) * unbox<unsigned>((explicitSize_ ? size_.width : maxSize_.width)) * 4
-            + unbox<unsigned>(_coord.column) * 4;
-        buffer_[base + 0] = _value.red;
-        buffer_[base + 1] = _value.green;
-        buffer_[base + 2] = _value.blue;
-        buffer_[base + 3] = 0xFF;
+        for (auto i = 0; i < aspectRatio_.nominator; ++i)
+        {
+            auto const base = unbox<unsigned>(_coord.line + i)
+                                  * unbox<unsigned>((explicitSize_ ? size_.width : maxSize_.width)) * 4
+                              + unbox<unsigned>(_coord.column) * 4;
+            for (auto j = 0; j < aspectRatio_.denominator; ++j)
+            {
+                buffer_[base + 0 + 4 * static_cast<unsigned int>(j)] = _value.red;
+                buffer_[base + 1 + 4 * static_cast<unsigned int>(j)] = _value.green;
+                buffer_[base + 2 + 4 * static_cast<unsigned int>(j)] = _value.blue;
+                buffer_[base + 3 + 4 * static_cast<unsigned int>(j)] = 0xFF;
+            }
+        }
     }
 }
 
@@ -440,8 +446,7 @@ void SixelImageBuilder::rewind()
 void SixelImageBuilder::newline()
 {
     sixelCursor_.column = {};
-
-    if (unbox<int>(sixelCursor_.line) + 6 < unbox<int>(maxSize_.height))
+    if (unbox<int>(sixelCursor_.line) + 6 < unbox<int>(explicitSize_ ? size_.height : maxSize_.height))
         sixelCursor_.line.value += 6;
 }
 
@@ -452,7 +457,7 @@ void SixelImageBuilder::setRaster(int _pan, int _pad, ImageSize _imageSize)
     size_.width = clamp(_imageSize.width, Width(0), maxSize_.width);
     size_.height = clamp(_imageSize.height, Height(0), maxSize_.height);
 
-    buffer_.resize(size_.area() * 4);
+    buffer_.resize(size_.area() * static_cast<size_t>(_pad * _pan) * 4);
     explicitSize_ = true;
 }
 
@@ -477,6 +482,12 @@ void SixelImageBuilder::render(int8_t _sixel)
 
 void SixelImageBuilder::finalize()
 {
+    if (unbox<int>(size_.height) == 1)
+    {
+        size_.height = Height::cast_from(sixelCursor_.line * aspectRatio_.nominator);
+        buffer_.resize(size_.area() * 4);
+        return;
+    }
     if (!explicitSize_)
     {
         Buffer tempBuffer(static_cast<size_t>(size_.height.value * size_.width.value) * 4);

src/terminal/SixelParser_test.cpp

@@ -338,3 +338,17 @@ TEST_CASE("SixelParser.newline", "[sixel]")
         }
     }
 }
+
+TEST_CASE("SixelParser.vertical_cursor_advance", "[sixel]")
+{
+    auto constexpr defaultColor = RGBAColor { 0, 0, 0, 255 };
+    SixelImageBuilder ib(
+        { Width(5), Height(30) }, 1, 1, defaultColor, std::make_shared<SixelColorPalette>(16, 256));
+    auto sp = SixelParser { ib };
+
+    sp.parseFragment("$-$-$-$-");
+    sp.done();
+
+    REQUIRE(ib.size() == terminal::ImageSize { Width(1), Height(24) });
+    REQUIRE(ib.sixelCursor() == CellLocation { LineOffset(24), ColumnOffset { 0 } });
+}