Skip to content

Add an application grid - #105

Open
Mjoyufull wants to merge 6 commits into
feat/dmenu-panel-movementfrom
feat/app-grid
Open

Mjoyufull wants to merge 6 commits into
feat/dmenu-panel-movementfrom
feat/app-grid

Conversation

@Mjoyufull

@Mjoyufull Mjoyufull commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Add an opt-in launcher grid using the existing panel and image pipeline.

  • Configure columns and cell height through --app-grid, --grid-row-height, or [app_launcher].
  • Place icons above names and preserve pinned colors, selection backgrounds, and the selected preview.
  • Center names independently of pins and selection markers, with equal text space in every cell.
  • Share geometry across drawing, image sizing, mouse hits, scrolling, and two-dimensional navigation.

Base: feat/dmenu-panel-movement. The manual grid pass passed apart from pin-dependent centering, now fixed with a regression test. The full stack passes 285 unit tests, 7 CLI tests, the graphics regression test, strict Clippy, release build, and rustdoc.

Greptile Summary

The app-launcher grid adds opt-in multi-column layout and navigation, but wrapped vertical movement from a partial final row changes columns unexpectedly. The full help view also omits the new grid flags, and the annotated configuration example does not include their defaults. These documented repository requirements and the navigation behavior need attention before merging.

Confidence Score: 4/5

Not merge-safe: wrapped grid navigation can select the wrong column, and the required help and annotated configuration documentation must be completed before merging.

The partial-row navigation failure was reproduced with an executable focused scenario. Two additional findings show that the required documentation remains incomplete.

Files Needing Attention: src/modes/app_launcher/events.rs needs column-preserving vertical wrapping; src/cli/help.rs needs the grid flags in the full help tree; USAGE.md and config.toml need annotated grid configuration defaults.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for the posted P1 finding and linked it to the corresponding review comment.
  • T-Rex produced a proof for the posted P2 finding.
  • Validated the general-contract-validation-proof by tracing the grid-navigation logic in src/modes/app_launcher/events.rs and confirming the mapping from steps to grid positions, including the observed 9 (row 2, column 1) -> 2 (row 0, column 2) transition with column preservation false.
  • Attached supporting artifacts for the grid navigation validation.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (2)

  1. src/cli/help.rs, line 88-89 (link)

    P2 Include grid flags in help

    The full --help option tree omits --app-grid and --grid-row-height, even though the man page documents both options. This violates the repository directive to keep the CLI man page synchronized with --help; the full help entries must be added before merging.

    Context Used: PROJECT_STANDARDS.md (source)

    Knowledge Base Used: Command-line interface

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/cli/help.rs
    Line: 88-89
    
    Comment:
    **Include grid flags in help**
    
    The full `--help` option tree omits `--app-grid` and `--grid-row-height`, even though the man page documents both options. This violates the repository directive to keep the CLI man page synchronized with `--help`; the full help entries must be added before merging.
    
    **Context Used:** PROJECT_STANDARDS.md ([source](https://github.com/mjoyufull/fsel/blob/main/PROJECT_STANDARDS.md))
    
    **Knowledge Base Used:** [Command-line interface](https://app.greptile.com/dev-org-14/-/custom-context/knowledge-base/mjoyufull/fsel/-/docs/command-line-interface.md)
    
    ---
    
    For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
  2. General comment

    P2 Wrapped Down movement from a partial final grid row shifts columns

    • Bug
      • For 4 columns and 11 entries, pressing Down at index 9 (row 2, column 1) with wrapping enabled selects index 2 (row 0, column 2), rather than index 1 in the same visible column.
    • Cause
      • In grid_neighbor, the forward-wrap branch at src/modes/app_launcher/events.rs:244-245 uses step - (len - selected). With 9, 11, and 4 this is 4 - (11 - 9) = 2, which does not retain the original column.
    • Fix
      • On forward vertical wrapping, calculate the destination from the selected column in the first row (for example, selected % columns) rather than wrapping the linear overflow distance.

    T-Rex Ran code and verified through T-Rex

Prompt To Fix All With AI
### Issue 1
src/modes/app_launcher/events.rs:244-245
**Preserve columns when wrapping**

With a four-column grid containing eleven items, wrapping Down from index 9 selects index 2. That moves from visible column 1 to column 2 instead of wrapping to index 1 in column 1. The grid therefore makes vertical navigation jump diagonally whenever its final row is incomplete, rather than moving by a grid row as documented.

### Issue 2
src/cli/help.rs:88-89
**Include grid flags in help**

The full `--help` option tree omits `--app-grid` and `--grid-row-height`, even though the man page documents both options. This violates the repository directive to keep the CLI man page synchronized with `--help`; the full help entries must be added before merging.

### Issue 3
USAGE.md:767-768
**Document grid config defaults**

The grid settings appear here only as bare values, while the canonical annotated `config.toml` omits `grid_columns` and `grid_row_height` entirely. This violates the repository directive requiring annotated example configuration with defaults; the annotated entries must be added before merging.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "Keep grid names centered independently o..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

Context used (5)

@Mjoyufull
Mjoyufull marked this pull request as ready for review September 11, 2026 01:18

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: afa96b9653

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/modes/app_launcher/events.rs Outdated
Comment thread src/ui/app_list.rs
Comment thread src/modes/app_launcher/events.rs Outdated
Comment thread USAGE.md

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 14 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/modes/app_launcher/events.rs Outdated
Comment thread src/ui/app_list.rs
Comment thread src/cli/help.rs Outdated
Comment thread fsel.1
Comment thread src/cli/help.rs Outdated
Comment thread USAGE.md
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