Skip to content

NCurses: Fix Colors Being Incorrect - #2375

Open
alecStewart1 wants to merge 1 commit into
lem-project:mainfrom
alecStewart1:fix/ncurses-colors
Open

alecStewart1 wants to merge 1 commit into
lem-project:mainfrom
alecStewart1:fix/ncurses-colors

Conversation

@alecStewart1

Copy link
Copy Markdown
Contributor

Why

Colors looked wrong when using the ncurses frontend. Admittedly I did use AI for this, as I don't really care to learn how to do crap with colors in programs that run in the terminal just to get this fixed. It was annoying and I didn't feel like spending a ton of time on this while I'm trying to do the SDL3 work in my spare time. Granted, I did learn a bit along the way about how terminals handle colors from reviewing the code and such.

I used glm-5.3-flash in Hermes Agent, if anyone really cares.

What this PR Does

The skinny of it is that OSC-4 and 24-bit truecolor are respected by the ncurses frontend. Originally colors were written to fit within a given range by calculating the HSV distance, and it would make a lot of things look noticeably different from what they would in the webview and likely SDL2 frontends.

AI Usage

It all started off by prompting about the colors in the ncurses frontend in both Ghostty and Windows Terminal using WSL, and then went from there to discuss how one could get proper colors. Initially there was "quantization" to convert colors, but supporting OSC-4 made more sense for modern terminals. It decided to create a function that calculates the CIELAB from sRGB for colors. I didn't ask why, but it seems to produce fairly accurate colors.

The specific comments it listed is:

Perceptual color distance for 256-color quantization.
sRGB -> CIELAB (D65 reference white) and CIE76 distance. This replaces
the earlier HSV distance, which distorted badly around grays and treated
hue and value errors identically.

A planning skill was used, and I had asked it to make sure any particular calculations for color codes be tweaked to be a bit more optimal using declaim / declare. I tested the changes out as well by rebuilding Lem and opening some files and changing the theme a few times.

For terminals that don't support OSC-4, the quantization will be the fallback for the 16-color nearest match.

AI Description

Stop overwriting the terminal palette; exact colors via OSC-4 and direct-color truecolor

  • init-colors no longer calls init_color wholesale: the terminal's own 16 scheme colors (e.g. Ghostty's non-xterm palette) are preserved and the lookup table is populated from color_content for slots 0-15 (slots 16+ keep the fixed xterm-256 grid values; curses' color table does not track the grid).
  • colors with no exact grid match get a fresh grid slot (16+) redefined to the exact RGB via init_color (OSC 4) instead of quantization; slots 0-15 are never touched; allocator resets on term-init; 16-color terminals fall back to nearest match (CIE76 in CIELAB space, replacing the old HSV distance). A slot is only committed when init_color reports OK and a color_content readback matches, so a failed redefinition falls through to quantization instead of burning the slot.
  • direct-color truecolor when terminfo has the RGB boolean (detected via tigetflag at term-init): color numbers are packed 24-bit RGB, pairs allocated with init_extended_pair, default pair kept at -1/-1 (assume_default_colors cannot carry packed RGB) and the theme background tracked separately for background-color; the default foreground stays the terminal's own in this mode.
  • the Lab helpers are declared (ftype + per-function (speed 3) (safety 2)) so SBCL emits unboxed double-float code while still checking ftype argument types against malformed color objects; Lab constants are named via alexandria:define-constant instead of read-time #. evaluation.
  • get-color-1 memoizes color-name -> palette slot: the scan used to rerun per get-color-pair call (per rendered cell, twice). The memo table is cleared by init-colors so entries cannot outlive the palette they were computed against.
  • docstrings on the new color-resolution functions document the resolve chain, the no-burn contract, and the intentional direct-mode behaviors.

Behavioural Changes

Color distance metric changed HSV to CIE76/CIELAB for 16-color nearest-match.

Truecolor terminals now get packed RGB pairs and the default foreground stays the terminal's own (theme foreground not applied in that mode).

Screenshots

Before with catppuccin-macchiato:

Screenshot 2026-11-11 180456 Screenshot 2026-11-11 180516

After changes with catppuccin-macchiato:

Screenshot 2026-11-11 180203 Screenshot 2026-11-11 180248

…4 and direct-color truecolor

- init-colors no longer calls init_color wholesale: the terminal's own
  16 scheme colors (e.g. Ghostty's non-xterm palette) are preserved and
  the lookup table is populated from color_content for slots 0-15
  (slots 16+ keep the fixed xterm-256 grid values; curses' color table
  does not track the grid).
- colors with no exact grid match get a fresh grid slot (16+) redefined
  to the exact RGB via init_color (OSC 4) instead of quantization;
  slots 0-15 are never touched; allocator resets on term-init;
  16-color terminals fall back to nearest match (CIE76 in CIELAB
  space, replacing the old HSV distance). A slot is only committed
  when init_color reports OK and a color_content readback matches, so
  a failed redefinition falls through to quantization instead of
  burning the slot.
- direct-color truecolor when terminfo has the RGB boolean (detected
  via tigetflag at term-init): color numbers are packed 24-bit RGB,
  pairs allocated with init_extended_pair, default pair kept at -1/-1
  (assume_default_colors cannot carry packed RGB) and the theme
  background tracked separately for background-color; the default
  foreground stays the terminal's own in this mode.
- the Lab helpers are declared (ftype + per-function (speed 3)
  (safety 2)) so SBCL emits unboxed double-float code while still
  checking ftype argument types against malformed color objects;
  Lab constants are named via alexandria:define-constant instead of
  read-time #. evaluation.
- get-color-1 memoizes color-name -> palette slot: the scan used to
  rerun per get-color-pair call (per rendered cell, twice). The memo
  table is cleared by init-colors so entries cannot outlive the
  palette they were computed against.
- docstrings on the new color-resolution functions document the
  resolve chain, the no-burn contract, and the intentional
  direct-mode behaviors.
@code-contractor-app

code-contractor-app Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

❌ Code Contractor Validation: FAILED

📌 Result for commit 2051f31

=== Contract: contract ===

✓ Code Contractor Validation Result
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

📋 Contract Source: Repository

📊 Statistics:
  Files Changed:    1
  Lines Added:      269
  Lines Deleted:    49
  Total Changed:    318
  Delete Ratio:     0.15 (15%)

Status: FAILED ❌

🤖 AI Providers:
  - codex — profile: bedrock-gpt61-terra

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
⚠️ Violations Found (2):

[ERROR] file_structure_rule
  AI check failed: "file_structure_rule"
  File: frontends/ncurses/term.lisp:436
  ❌ Reason:
    A new variable declaration appears after function definitions.

[ERROR] functional_style_rule
  AI check failed: "functional_style_rule"
  File: frontends/ncurses/term.lisp:458
  ❌ Reason:
    The new special variable holds mutable allocation state shared between
    functions rather than passed explicitly.
📋 Contract Configuration: contract (Source: Repository)
version: 2

trigger:
  paths:
    - "extensions/**"
    - "frontends/**/*.lisp"
    - "src/**"
    - "tests/**"
    - "contrib/**"
    - "**/*.asd"
  head_branches:
    exclude:
      - 'revert-*'

validation:
  limits:
    max_total_changed_lines: 400
    max_delete_ratio: 0.5
    max_files_changed: 10
    severity: warning

  ai:
    system_prompt: |
      You are a senior Common Lisp engineer reviewing code for Lem editor.
      Lem is a text editor with multiple frontends (ncurses, SDL2, webview).
      Focus on maintainability, consistency with existing code, and Lem-specific conventions.
    rules:
      # === File Structure ===
      - name: defpackage_rule
        prompt: |
          First form must be `defpackage` or `uiop:define-package`.
          Package name should match filename (e.g., `foo.lisp` → `:lem-ext/foo` or `:lem-foo`).
          Extensions must use `lem-` prefix (e.g., `:lem-python-mode`).

      - name: file_structure_rule
        prompt: |
          File organization (top to bottom):
          1. defpackage
          2. defvar/defparameter declarations
          3. Key bindings (define-key, define-keys)
          4. Class/struct definitions
          5. Functions and commands

          Only flag violations when elements appear OUT OF ORDER (e.g., key bindings AFTER functions, or defvar AFTER functions).
          Key bindings appearing BEFORE functions is correct and expected.

          Exception: In major-mode files (e.g., *-mode.lisp), flexible ordering is acceptable when it follows established patterns from similar modes (e.g., clojure-mode, python-mode).

      # === Style ===
      - name: loop_keywords_rule
        prompt: |
          Loop keywords must use colons: `(loop :for x :in list :do ...)`
          NOT: `(loop for x in list do ...)`

      - name: naming_conventions_rule
        prompt: |
          Naming conventions:
          - Functions/variables: kebab-case (e.g., `find-buffer`)
          - Special variables: *earmuffs* (e.g., `*global-keymap*`)
          - Constants: +plus-signs+ (e.g., `+default-tab-size+`)
          - Predicates: -p suffix for functions (e.g., `buffer-modified-p`)
          - Do NOT use -p suffix for user-configurable variables

      # === Documentation ===
      - name: docstring_rule
        prompt: |
          Required docstrings for:
          - Exported functions, methods, classes
          - `define-command` (explain what the command does)
          - Generic functions (`:documentation` option)
          Important functions should explain "why", not just "what".
        severity: warning

      # === Lem-Specific ===
      - name: internal_symbol_rule
        prompt: |
          Use exported symbols from `lem` or `lem-core` package.
          Avoid `lem::internal-symbol` access.
          If internal access is necessary, document why.

      - name: error_handling_rule
        prompt: |
          - `error`: Internal/programming errors
          - `editor-error`: User-facing errors (displayed in echo area)
          Always use `editor-error` for messages shown to users.

      - name: frontend_interface_rule
        prompt: |
          Frontend-specific code must use `lem-if:*` protocol.
          Do not call frontend implementation directly from core.
        severity: warning

      # === Functional Style ===
      - name: functional_style_rule
        prompt: |
          Prefer explicit function arguments over dynamic variables.
          Avoid using `defvar` for state passed between functions.
          Exception: Well-documented cases like `*current-buffer*`, constants, and configuration variables (not meant to be modified at runtime).

      - name: dynamic_symbol_call_rule
        prompt: |
          Avoid `uiop:symbol-call`. Rethink architecture instead.
          Exception: Acceptable in extensions when following patterns established in other major-mode extensions (e.g., for optional dependency loading or plugin systems).
          If used, document the reason.

      # === Libraries ===
      - name: alexandria_usage_rule
        prompt: |
          Alexandria utilities allowed: `if-let`, `when-let`, `with-gensyms`, etc.
          Avoid: `alexandria:curry` (use explicit lambdas)
          Avoid: `alexandria-2:*` functions not yet used in codebase

      # === Macros ===
      - name: macro_style_rule
        prompt: |
          Keep macros small. For complex logic, use `call-with-*` pattern:
          ```lisp
          (defmacro with-foo (() &body body)
            `(call-with-foo (lambda () ,@body)))
          ```
          Prefer `list` over backquote outside macros.

          Exception: In major-mode files, macro patterns that are standard in other major-modes (e.g., clojure-mode, python-mode) are acceptable.

💬 Feedback

Reply to a violation comment with:

  • /dismiss <reason> - Report false positive or not applicable

🔁 Re-validation

Validation does not re-run automatically when you push fixes. Trigger it with one of:

  • Comment /rerun to re-run validation (dismissed violations stay dismissed)
  • Click Run Validation on the Code Contractor check in the Checks tab
  • Comment @code-contractor-app on this pull request

To reset all dismissals and re-judge every violation, comment /revalidate instead (restores previously dismissed violations; requires write permission).

📚 About Code Contractor

Declarative Code Standards That Learn and Improve

Define domain-specific validation rules in YAML.
Your contracts document team knowledge and evolve into more accurate AI enforcement.

Want this for your repo?
→ Install Code Contractor

@code-contractor-app code-contractor-app Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Contractor validation failed ❌ — see the sticky comment for full results.

(cffi:defcfun ("init_extended_pair" %init-extended-pair)
:int (pair :int) (f :int) (b :int))

(defvar *direct-color* nil)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Contractor: file_structure_rule

Contract: contract

📍 frontends/ncurses/term.lisp:436

AI check failed: "file_structure_rule"

Reason:
A new variable declaration appears after function definitions.


💬 Reply /dismiss <reason> to dismiss this violation.

;; The initial value of 256 intentionally disables allocation: it is
;; only ever valid before init-colors runs (which resets it to 16), and
;; 256 is out of range for any *colors* array.
(defvar *redefine-next-slot* 256)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Contractor: functional_style_rule

Contract: contract

📍 frontends/ncurses/term.lisp:458

AI check failed: "functional_style_rule"

Reason:
The new special variable holds mutable allocation state shared between functions rather than passed explicitly.


💬 Reply /dismiss <reason> to dismiss this violation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant