Skip to content

fix: cheap(-ish) fixes. - #2371

Open
alecStewart1 wants to merge 1 commit into
lem-project:mainfrom
alecStewart1:fix/cheap-fixes
Open

alecStewart1 wants to merge 1 commit into
lem-project:mainfrom
alecStewart1:fix/cheap-fixes

Conversation

@alecStewart1

@alecStewart1 alecStewart1 commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Bunch of little fixes I found when poking around the code:

  1. search.lisp - comparator was inverted (*case-fold-search* on -> exact match, off -> insensitive). Fix makes default search case-sensitive as intended.
  2. physical-line.lisp - image-object equality/mergability compared object 1 to itself 3X (always true); now compares 1 <-> 2.
  3. edit.lisp - kill-line with a too-large negative arg killed from point to buffer end; now goes to buffer start.
  4. basic.lisp insert-buffer - copy-tree on line plists stops cross-buffer mutation sharing; no trailing newline after the last line means inserted text now matches the source exactly.
  5. window.lisp - get-next-window now honors a caller-supplied window-list in the attached-window branch (previously silently fell back to the global list), and get-previous-window gains the same attached-window handling.
  6. eol-cursor equality/mergability now includes true-cursor-p, so a real cursor and a same-color eol-cursor no longer merge and lose cursor styling.

AI Usage Disclosure

Did you use LLMs/AI tools for this PR?

  • No, this is 100% human-authored.
  • Yes, AI-assisted (Human-led logic, AI-assisted implementation).
  • Yes, AI-generated (Logic primarily derived from prompt/vibe).

@code-contractor-app

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

Copy link
Copy Markdown
Contributor

⏳ Commit c577049 has not been validated yet. The results below are for a previous commit. Comment /rerun (keeps dismissed violations dismissed) or @code-contractor-app to re-validate.

✅ Code Contractor Validation: PASSED

📌 Result for commit 903adca

=== Contract: contract ===

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

📋 Contract Source: Repository

📊 Statistics:
  Files Changed:    8
  Lines Added:      53
  Lines Deleted:    43
  Total Changed:    96
  Delete Ratio:     0.45 (45%)

Status: PASSED ✅

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

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
🎉 No violations detected. Great job!
📋 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.
📚 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

(if *case-fold-search*
#'char=
#'char-equal))
#'char-equal

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since *case-fold-search* defaults to nil (line 3), fixing the comparator without changing the default makes every search-forward/search-backward caller case-sensitive. That includes isearch (C-s/C-r), query-replace-internal, and directory-mode lookups. For example, C-s foo used to match Foo and no longer does. If the old behavior should stay, also change the default to (defvar *case-fold-search* t), which matches Emacs.

(setf (line:line-plist (point-line point)) (line:line-plist line))
(insert-character point #\newline)))
(setf (line:line-plist (point-line point)) (copy-tree (line:line-plist line)))
(when (line:line-next line)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Dropping the trailing newline changes what callers get back. copy-input-buffer-to-output-buffer in src/ext/interactive-mode.lisp used to leave the output point on a new line after the copied input. Now it stops at the end of the input line, so the execute-input output (claude-code, for example) is written onto the same line as the user input. The prompt-region-p span also no longer covers a line break. Callers such as interactive-mode need to add the newline themselves, or this behavior should stay.

Comment thread src/window/window.lisp
(if (attached-window-p window)
(get-previous-window (attached-window-parent-window window) window-list)
(let ((prev (let ((result (member window (reverse window-list))))
(if (cdr result)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(car window-list) is the wrong wrap-around for the previous window. When WINDOW is the first element of WINDOW-LIST, (member window (reverse window-list)) returns a one-element tail, so this returns WINDOW itself instead of the last window. It should be (car (last window-list)).

Comment thread extensions/vi-mode/commands.lisp Outdated
(let ((lem:*case-fold-search* t))
(when (loop repeat count
(when (loop repeat count
for result = (funcall (if (eq direction :forward)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Removing the binding ties vi f/t/F/T to the global *case-fold-search*. Before, binding it to t gave exact matching (because of the inverted comparator). Now a user who sets *case-fold-search* to t, which works now that the comparator is fixed, also gets case-insensitive f/t, which Vim does not do. Bind (let ((lem:*case-fold-search* nil)) ...) explicitly instead of dropping the binding. Also, .claude/rules/lisp-style.md says loop_keywords_rule: Use (loop :for ... :do ...) with colons, and this line uses repeat/for/unless without colons.

Comment thread src/window/window.lisp
(cadr result)
(car window-list)))))
(or (window-attached-window next)
next))))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

get-previous-window has no callers in the repository: previous-window calls next-window with a negative N, and the function is not exported in internal-packages.lisp. The new attached-window branch adds untested code to a dead function. Either delete it or route previous-window through it.

Comment thread src/buffer/file.lisp Outdated
:condition e
:filename stream-filename))))
(read-line stream nil))
(handler-case (read-line stream nil)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This switch from handler-bind to handler-case is not in the PR description. handler-case unwinds the stack before encoding-read-error is signaled, so the debugger and any outer handler lose the original read-line frames and decoding restarts, for example SBCL attempt-resync. Behavior for the find-file-buffer and execute-find-file handlers is the same, so this change only removes diagnostics.

Comment thread src/display/physical-line.lisp Outdated
(eol-cursor-object-color drawing-object-2)))
(and (equal (eol-cursor-object-color drawing-object-1)
(eol-cursor-object-color drawing-object-2))
(eq (eol-cursor-object-true-cursor-p drawing-object-1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

true-cursor-p comes from cursor-attribute-p, which returns (attribute-value attribute :cursor), a generalized boolean. Comparing it with eq treats two true values as different whenever they are not the same object. Today every caller sets it to t, but (eq (not (null a)) (not (null b))) is safer. The same applies to the drawing-object-mergable-p method below.

@alecStewart1
alecStewart1 force-pushed the fix/cheap-fixes branch 2 times, most recently from b748067 to 8cccca2 Compare November 11, 2026 15:04
1. `search.lisp` - comparator was inverted (`*case-fold-search*` on -> exact match, off -> insensitive). Fix makes default search case-sensitive as intended.
2. `physical-line.lisp` - `image-object` equality/mergability compared object 1 to itself 3X (always true); now compares 1 <-> 2.
3. `edit.lisp` - `kill-line` with a too-large negative arg killed from point to buffer end; now goes to buffer start.
4. `basic.lisp` `insert-buffer` - copy-tree on line plists stops cross-buffer mutation sharing; no trailing newline after the last line means inserted text now matches the source exactly.
5. `window.lisp` - `get-next-window` now honors a caller-supplied window-list in the attached-window branch (previously silently fell back to the global list), and `get-previous-window` gains the same attached-window handling.
6. `eol-cursor` equality/mergability now includes `true-cursor-p`, so a real cursor and a same-color `eol-cursor` no longer merge and lose cursor styling.
@alecStewart1

Copy link
Copy Markdown
Contributor Author

/revalidate

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.

2 participants