On Mon, 5 Jan 2026 14:35:29 +0300 Kerem Aksu <[email protected]> wrote:
> Control+L should clear screen and redisplay prompt at the top > line. DPDK rdline library will not change anything on the screen when > fed with Control+L. When prompt is lost after too many text written > to the terminal, users will press Control+L to clear screen and put > the prompt to the top line. This is expected behavior in bash(1) or > applications that are using readline(3). Updated to behave as users > expected. > > Signed-off-by: Kerem Aksu <[email protected]> > --- Overall looks correct. AI review had some feedback. I would add a release note just for clarity ### Summary This patch modifies the Ctrl+L behavior in the cmdline library to clear the entire screen and redisplay the prompt at the top, matching standard bash/readline behavior. Currently, Ctrl+L only redisplays on the current line. --- ### Commit Message ✅ | Check | Status | Notes | |-------|--------|-------| | Subject ≤60 chars | ✅ | 40 characters | | Correct prefix | ✅ | `cmdline:` is correct for lib/cmdline | | Imperative mood | ✅ | "update" | | No trailing period | ✅ | | | Lowercase | ✅ | | | Body wrapped ≤75 chars | ✅ | | | Body doesn't start with "It" | ✅ | | | Signed-off-by | ✅ | Present | | Acked-by/Tested-by | ✅ | Both present | --- ### Code Review **Overall Assessment**: The approach is sound. Refactoring `rdline_redisplay` into a parameterized `rdline_reprint` static function maintains API compatibility while adding the new clear-screen behavior. #### Errors (must fix) **1. Extra whitespace after comma** (style violation) Lines 126-129 in the patch have double spaces: ```c rdline_puts(rdl, vt100_clear_screen); /* two spaces after comma */ rdline_puts(rdl, vt100_homecursor); /* two spaces after comma */ rdline_puts(rdl, vt100_home); /* two spaces after comma */ ``` Should be: ```c rdline_puts(rdl, vt100_clear_screen); rdline_puts(rdl, vt100_homecursor); rdline_puts(rdl, vt100_home); ``` #### Warnings (should consider) **2. Consider order of escape sequences** The current implementation sends `vt100_clear_screen` then `vt100_homecursor`. Some terminals may already position cursor at home after clear screen (ESC [2J), but the explicit `ESC [H` is safer for portability. This is fine as-is. **3. Potential documentation update** Consider whether the programmer's guide (`doc/guides/prog_guide/cmdline.rst`) or API documentation needs updating to reflect the changed Ctrl+L behavior. Users familiar with the old behavior might notice the change. #### Info (observations) **4. Good API preservation** The `RTE_EXPORT_SYMBOL(rdline_redisplay)` is correctly maintained, and the public function signature is unchanged. This preserves backward compatibility for any external callers. **5. New VT100 constant placement** The new `vt100_homecursor` is added in a reasonable location in `cmdline_vt100.h`, near related cursor movement sequences. **6. Existing style quirks preserved** The file already uses `for (i=0 ; i<...` style (spaces around semicolons but not around `=` and `<`). The patch follows this existing convention, which is correct. --- ### Verdict **Needs revision** - Fix the double-space style issue, then this is ready. The change itself is a sensible UX improvement that aligns DPDK's command line behavior with user expectations from bash and readline. The implementation is clean and maintains API compatibility.
