Skip to content

[update] <components>:finsh/shell.c 增加新功能 #10394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 6, 2025

Conversation

illustriousness
Copy link
Contributor

@illustriousness illustriousness commented Jun 13, 2025

添加以下功能 (需要kconfig使能FINSH_USING_WORD_OPERATION)
1 ctrl+back 按单词删除
2 ctrl+左右箭头 按单词切换光标

拉取/合并请求描述:(PR description)

[

为什么提交这份PR (why to submit this PR)

你的解决方案是什么 (what is your solution)

请提供验证的bsp和config (provide the config and bsp)

  • BSP:
  • .config:
  • action:

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/workflows/bsp_buildings.yml 详细请参考链接BSP自查

@BernardXiong
Copy link
Member

BernardXiong commented Jun 16, 2025

感觉这样做了以后,是否会变得复杂化?对于shell来说,反而希望增加"一些类脚本的简单功能,例如可以定义变量,一些变量甚至可以存储下来,以及包含简单的条件处理。" 当然,这些高级功能应该是可配置,可裁剪的。

@illustriousness
Copy link
Contributor Author

“在调试长命令时,单词级操作可提升效率。此功能在主流 Shell(如 bash/zsh)中为标准支持”
此功能已通过 FINSH_USING_WORD_OPERATION 完全隔离,默认关闭
您的顾虑我已经理解 后续我提PR时往类脚本方向进行开发

/* Delete characters with proper RT_null termination */
rt_memmove(&shell->line[start],
&shell->line[start + del_count],
new_len - start + 1); // +1 包含 RT_null 终止符
Copy link
Member

Choose a reason for hiding this comment

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

请使用英文的注释。下同

{
if (curpos <= 0) return 0;

// Skip whitespace
Copy link
Member

Choose a reason for hiding this comment

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

请尽可能使用 /* */ C语言风格的注释,下同。

@illustriousness illustriousness force-pushed the finsh branch 2 times, most recently from 50071dc to 3f13eb4 Compare July 1, 2025 02:13
@illustriousness
Copy link
Contributor Author

@Rbb666 麻烦帮忙看一下

@@ -661,7 +728,43 @@ static void finsh_thread_entry(void *parameter)

continue;
}
#if defined(FINSH_USING_WORD_OPERATION)
Copy link
Member

Choose a reason for hiding this comment

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

这块不要有缩进

Copy link

github-actions bot commented Aug 5, 2025

📌 Code Review Assignment

🏷️ Tag: components

Reviewers: Maihuanyi

Changed Files (Click to expand)
  • components/finsh/Kconfig
  • components/finsh/shell.c

📊 Current Review Status (Last Updated: 2025-08-05 17:45 CST)

  • Maihuanyi Pending Review

📝 Review Instructions

  1. 维护者可以通过单击此处来刷新审查状态: 🔄 刷新状态
    Maintainers can refresh the review status by clicking here: 🔄 Refresh Status

  2. 确认审核通过后评论 LGTM/lgtm
    Comment LGTM/lgtm after confirming approval

  3. PR合并前需至少一位维护者确认
    PR must be confirmed by at least one maintainer before merging

ℹ️ 刷新CI状态操作需要具备仓库写入权限。
ℹ️ Refresh CI status operation requires repository Write permission.

添加以下功能(需要kconfig使能FINSH_USING_WORD_OPERATION)
1 ctrl+back 按单词删除
2 ctrl+左右箭头 按单词切换光标

Signed-off-by: Yucai Liu <[email protected]>
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds word-based navigation and editing functionality to the RT-Thread finsh shell component. The changes enable users to navigate by words using Ctrl+Arrow keys and delete words using Ctrl+Backspace when the new FINSH_USING_WORD_OPERATION configuration option is enabled.

  • Adds two utility functions for finding word boundaries in command line text
  • Implements Ctrl+Left/Right arrow key handling for word-based cursor movement
  • Implements Ctrl+Backspace for word-based deletion
  • Adds Kconfig option to enable/disable the new functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
components/finsh/shell.c Implements word navigation functions and keyboard shortcut handlers
components/finsh/Kconfig Adds configuration option to enable word operation features
Comments suppressed due to low confidence (1)

components/finsh/shell.c:2

  • The copyright year 2025 appears to be in the future. Based on my knowledge cutoff in January 2025, this year may not be accurate for the current date.
 * Copyright (c) 2006-2025, RT-Thread Development Team

if (curpos <= 0) return 0;

/* Skip whitespace */
while (--curpos > 0 && (line[curpos] == ' ' || line[curpos] == '\t'));
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The loop uses pre-decrement --curpos in the condition, which could cause the function to skip checking the character at the initial position. This may lead to incorrect word boundary detection.

Suggested change
while (--curpos > 0 && (line[curpos] == ' ' || line[curpos] == '\t'));
while (curpos > 0 && (line[curpos] == ' ' || line[curpos] == '\t'))
curpos--;

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果当前curpos 为空格 跳过就跳过了
如果不是空格 预减也不影响

Comment on lines +732 to +733
/* Add Ctrl+Backspace handling */
else if (ch == 0x17) /* Ctrl+Backspace (typically ^W) */
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The comment suggests this is typically Ctrl+Backspace, but 0x17 is actually Ctrl+W. Ctrl+Backspace typically sends different escape sequences depending on the terminal. This could confuse users about which key combination actually triggers this functionality.

Suggested change
/* Add Ctrl+Backspace handling */
else if (ch == 0x17) /* Ctrl+Backspace (typically ^W) */
/* Add Ctrl+W (delete previous word) handling */
else if (ch == 0x17) /* Ctrl+W (delete previous word) */

Copilot uses AI. Check for mistakes.

/* Delete characters and properly add RT_NULL termination */
rt_memmove(&shell->line[start],
&shell->line[start + del_count],
new_len - start + 1);
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The calculation new_len - start + 1 for the memmove length appears incorrect. It should be shell->line_position - start - del_count to move the remaining characters after the deleted word.

Suggested change
new_len - start + 1);
shell->line_position - start - del_count);

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@illustriousness illustriousness requested a review from Rbb666 August 6, 2025 03:08
@Rbb666 Rbb666 merged commit 236e830 into RT-Thread:master Aug 6, 2025
64 checks passed
@illustriousness illustriousness deleted the finsh branch August 6, 2025 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants