应用运行 - 添加应用时按名称排序以便查找#2219
Open
JoshCai233 wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthrough将默认组应用缓存从 dict 改为按 [id, name] 对的列表（ Changes应用列表 -> 对偶列表与区域感知排序
Sequence Diagram(s)sequenceDiagram
participant UI as StandaloneRunInterface
participant Dialog as AddAppDialog
participant Widget as AppListWidget
participant Context as RunContext
UI->>Context: 请求所有应用 id/name
Context-->>UI: 返回未排序 id/name 列表
UI->>UI: 使用 locale.strxfrm 对 name 排序并缓存 self.all_apps
UI->>Dialog: 打开 AddAppDialog(available_apps=[id,name] 列表)
Dialog-->>UI: 返回 selected_ids
UI->>UI: 通过 self.all_apps 找到对应 name
UI->>Widget: 调用 add_app(id, name)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/one_dragon_qt/view/standalone_app_run_interface.py (1)
194-211:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
self._app_ids与排序后的列表行索引不匹配，导致返回错误的 app_id
self._app_ids（第 194 行）按available_apps.keys()的原始迭代顺序构建，但列表控件中的条目是按名称排序后的顺序添加的。get_selected_ids()通过行号i访问self._app_ids[i]，一旦排序顺序与原始顺序不同，就会返回错误的app_id。示例：
available_apps = {"app_b": "Beta", "app_a": "Alpha"}self._app_ids = ["app_b", "app_a"]- 排序后列表控件：第 0 行 = "Alpha"，第 1 行 = "Beta"
- 用户选中 "Alpha"（第 0 行）→
get_selected_ids()返回self._app_ids[0]="app_b"❌修复方式：在循环中同时追踪
app_id，排序后再重建self._app_ids；或使用QListWidgetItem.setData(Qt.ItemDataRole.UserRole, app_id)将app_id嵌入条目中。🐛 建议修复（追踪 app_id 并在排序后重建 self._app_ids）
- self._app_ids = list(available_apps.keys()) - self._list_widget = MultiSelectListWidget() self._list_widget.setSelectionMode(QAbstractItemView.SelectionMode.MultiSelection) self._list_widget.setViewportMargins(0, 0, 16, 0) self._list_widget.setMinimumHeight(400) self._list_widget.setMinimumWidth(480) item_font = getFont(16) - items = [] - for _, app_name in available_apps.items(): + items: list[tuple[str, QListWidgetItem]] = [] + for app_id, app_name in available_apps.items(): item = QListWidgetItem(gt(app_name)) item.setSizeHint(QSize(0, 36)) item.setFont(item_font) - items.append(item) + items.append((app_id, item)) items.sort(key=lambda x: x[1].text()) # 按名称排序以便查找 + self._app_ids = [app_id for app_id, _ in items] - for item in items: + for _, item in items: self._list_widget.addItem(item)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/one_dragon_qt/view/standalone_app_run_interface.py` around lines 194 - 211, The list widget items are sorted by display name but self._app_ids is built from available_apps.keys() so indexes no longer match in get_selected_ids(); fix by storing the app_id with each QListWidgetItem (use QListWidgetItem.setData(Qt.ItemDataRole.UserRole, app_id)) when creating items or rebuild self._app_ids after sorting so the index order matches the visual order, then update get_selected_ids() to retrieve app_id from item.data(UserRole) (or from the new self._app_ids) rather than assuming original index alignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/one_dragon_qt/view/standalone_app_run_interface.py`:
- Around line 194-211: The list widget items are sorted by display name but
self._app_ids is built from available_apps.keys() so indexes no longer match in
get_selected_ids(); fix by storing the app_id with each QListWidgetItem (use
QListWidgetItem.setData(Qt.ItemDataRole.UserRole, app_id)) when creating items
or rebuild self._app_ids after sorting so the index order matches the visual
order, then update get_selected_ids() to retrieve app_id from
item.data(UserRole) (or from the new self._app_ids) rather than assuming
original index alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78959b52-f093-46c0-b09a-0c8ae6cf767a
📒 Files selected for processing (1)
src/one_dragon_qt/view/standalone_app_run_interface.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/one_dragon_qt/view/standalone_app_run_interface.py (1)
195-218:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win排序后仍沿用原始
app_id顺序会选错应用。
self._app_ids在排序前就按available_apps的原始顺序保存了，但 Line 215 又把显示项重排了一次。这样get_selected_ids()再按选中行号回查时，取到的就是排序前的app_id，用户选中的名称和最终添加的应用会错位。🐛 建议修复
- self._app_ids = list(available_apps.keys()) + self._app_ids: list[str] = [] self._list_widget = MultiSelectListWidget() self._list_widget.setSelectionMode(QAbstractItemView.SelectionMode.MultiSelection) self._list_widget.setViewportMargins(0, 0, 16, 0) self._list_widget.setMinimumHeight(400) self._list_widget.setMinimumWidth(480) item_font = getFont(16) - items = [] - for _, app_name in available_apps.items(): + items: list[tuple[str, QListWidgetItem]] = [] + for app_id, app_name in available_apps.items(): item = QListWidgetItem(gt(app_name)) item.setSizeHint(QSize(0, 36)) item.setFont(item_font) - items.append(item) + items.append((app_id, item)) # 按拼音排序以便查找 try: locale.setlocale(locale.LC_COLLATE, "zh_CN.UTF-8") except: locale.setlocale(locale.LC_COLLATE, "") - items.sort(key=lambda x: locale.strxfrm(x.text())) + items.sort(key=lambda x: locale.strxfrm(x[1].text())) - for item in items: + self._app_ids = [app_id for app_id, _ in items] + for _, item in items: self._list_widget.addItem(item)Also applies to: 225-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/one_dragon_qt/view/standalone_app_run_interface.py` around lines 195 - 218, The displayed QListWidget items are sorted independently of self._app_ids, so get_selected_ids() which maps selected row indices back to self._app_ids returns wrong app_ids; after you build and sort the items list (the variable items and its sort call using locale.strxfrm), rebuild self._app_ids to match that new visual order by mapping each sorted QListWidgetItem back to its app_id from available_apps (or create a list of tuples (app_id, app_name) before creating items and sort that), and use that reordered self._app_ids wherever get_selected_ids() or the later code (the block around the later use referenced) relies on row indices to look up ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/one_dragon_qt/view/standalone_app_run_interface.py`:
- Around line 210-215: The code calls locale.setlocale(locale.LC_COLLATE, ...)
during dialog initialization (using items.sort with key=lambda x:
locale.strxfrm(x.text())), which mutates process-global locale and uses a bare
except; change to a local, Qt-based comparison using QCollator or QLocale (e.g.,
create a QCollator/QLocale instance and use its key/compare for items.sort) so
sorting is localized without touching process state; if you must keep the
locale.setlocale approach temporarily, narrow the except to catch only
locale.Error and avoid swallowing other exceptions, and keep the items.sort key
using the localized transform consistently.
---
Outside diff comments:
In `@src/one_dragon_qt/view/standalone_app_run_interface.py`:
- Around line 195-218: The displayed QListWidget items are sorted independently
of self._app_ids, so get_selected_ids() which maps selected row indices back to
self._app_ids returns wrong app_ids; after you build and sort the items list
(the variable items and its sort call using locale.strxfrm), rebuild
self._app_ids to match that new visual order by mapping each sorted
QListWidgetItem back to its app_id from available_apps (or create a list of
tuples (app_id, app_name) before creating items and sort that), and use that
reordered self._app_ids wherever get_selected_ids() or the later code (the block
around the later use referenced) relies on row indices to look up ids.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8cf0759-649c-44cb-af44-77de288ed710
📒 Files selected for processing (1)
src/one_dragon_qt/view/standalone_app_run_interface.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/one_dragon_qt/view/standalone_app_run_interface.py (1)
168-168: ⚡ Quick win用
next()替代单元素切片（Ruff RUF015）。♻️ 建议修复
- self.app_list_widget.add_app(app_id, [item[1] for item in all_apps if item[0] == app_id][0]) + self.app_list_widget.add_app(app_id, next(item[1] for item in all_apps if item[0] == app_id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/one_dragon_qt/view/standalone_app_run_interface.py` at line 168, Replace the single-element list slice [item[1] for item in all_apps if item[0] == app_id][0] with next(item[1] for item in all_apps if item[0] == app_id) to avoid creating an intermediate list; update the call site self.app_list_widget.add_app(app_id, ...) accordingly (use next(...) as the second argument), and if missing entries are possible consider supplying a default to next(..., default_value) to avoid StopIteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/one_dragon_qt/view/standalone_app_run_interface.py`:
- Line 54: The type annotation list[[str, str]] is invalid; replace it with a
proper nested collection type (either list[list[str]] or list[tuple[str, str]]
for a more precise tuple). Update the four occurrences of list[[str, str]] in
this module (for example the attribute self.all_apps) to use list[list[str]] or
list[tuple[str, str]] consistently, and if you need to support older Python
versions, import and use typing.List/typing.Tuple instead.
- Around line 122-128: _refresh_app_list currently filters the pinyin-sorted
_get_all_apps() (all_apps) which resets the user order saved in config.app_list
and uses an O(n²) nested comprehension; change the logic to preserve
config.app_list order by mapping IDs to names from all_apps (use a dict like
{id: name}) and then build app_list by iterating config.app_list and only
including ids present in that map, and for better performance when you need the
inverse (filtering all_apps) use a set(config.app_list) to avoid O(n²); update
the code paths in _refresh_app_list that produce app_list and then call
app_list_widget.set_app_list with the reordered list so the UI reflects the
saved user order.
---
Nitpick comments:
In `@src/one_dragon_qt/view/standalone_app_run_interface.py`:
- Line 168: Replace the single-element list slice [item[1] for item in all_apps
if item[0] == app_id][0] with next(item[1] for item in all_apps if item[0] ==
app_id) to avoid creating an intermediate list; update the call site
self.app_list_widget.add_app(app_id, ...) accordingly (use next(...) as the
second argument), and if missing entries are possible consider supplying a
default to next(..., default_value) to avoid StopIteration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4931015-6f6e-4462-a25c-8f6164e4fd7b
📒 Files selected for processing (1)
src/one_dragon_qt/view/standalone_app_run_interface.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/one_dragon_qt/view/standalone_app_run_interface.py (1)
97-118: ⚡ Quick win请确认
self.all_apps不会变成陈旧缓存。
_get_all_apps()首次构建后就一直复用self.all_apps；如果默认组应用、应用名称或语言环境会在界面生命周期内变化，这里会继续显示旧列表/旧排序，导致“添加应用”弹窗和实际注册信息不一致。若这些数据是动态的，建议在变更时失效缓存，或者改成每次打开弹窗时重建。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/one_dragon_qt/view/standalone_app_run_interface.py` around lines 97 - 118, The method _get_all_apps currently caches results in self.all_apps and never refreshes, causing stale data if default_group_apps, application names, or locale change; update the code to avoid or invalidate this cache: either remove persistent caching and always rebuild the list inside _get_all_apps, or add cache invalidation hooks that clear self.all_apps when ctx.run_context.default_group_apps, get_application_name results, or locale changes (e.g. on language change or group update events); reference symbols: _get_all_apps, self.all_apps, self.ctx.run_context.default_group_apps, self.ctx.run_context.get_application_name, and the locale handling block so the cache is refreshed before sorting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/one_dragon_qt/view/standalone_app_run_interface.py`:
- Around line 97-118: The method _get_all_apps currently caches results in
self.all_apps and never refreshes, causing stale data if default_group_apps,
application names, or locale change; update the code to avoid or invalidate this
cache: either remove persistent caching and always rebuild the list inside
_get_all_apps, or add cache invalidation hooks that clear self.all_apps when
ctx.run_context.default_group_apps, get_application_name results, or locale
changes (e.g. on language change or group update events); reference symbols:
_get_all_apps, self.all_apps, self.ctx.run_context.default_group_apps,
self.ctx.run_context.get_application_name, and the locale handling block so the
cache is refreshed before sorting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c723666-092c-4bee-9f0e-685004e348d4
📒 Files selected for processing (1)
src/one_dragon_qt/view/standalone_app_run_interface.py
Contributor
|
同步一条龙运行列表的排序会不会更好 |
Contributor
Author
还有插件呢，其实可以搞个分组，但是目前插件并没有那么多 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit