refactor: 重构任务队列空闲处理与关闭游戏/模拟器配置#1525
Merged
Merged
Conversation
- 重新整理任务队列空闲时的处理分支与执行流程 - 调整关闭游戏/关闭模拟器相关的时间配置结构
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些高层次的反馈：
- 决定在空闲等待期间何时关闭模拟器的逻辑现在在
_wait_close_game和_wait_goto_main之间被重复了；建议抽取一个共享的辅助函数，这样可以将行为和配置处理集中在一个地方，保持一致性。 - 在
_wait_close_game中，当超出关闭模拟器的等待时间时，会无条件调用self.device.emulator_stop()；如果此时模拟器可能已经关闭，建议先检查_emulator_down（或类似标志），以避免冗余的停止调用或设备层面的错误。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- The logic that decides when to close the emulator during idle waits is now duplicated between `_wait_close_game` and `_wait_goto_main`; consider extracting a shared helper to keep behavior and configuration handling consistent in one place.
- In `_wait_close_game`, `self.device.emulator_stop()` is called unconditionally when the close-emulator wait is exceeded; if the emulator might already be down, consider checking `_emulator_down` (or similar) first to avoid redundant stop calls or device-layer errors.
## Individual Comments
### Comment 1
<location path="script.py" line_range="393" />
<code_context>
+ self.run("Restart")
+ return True
+
+ if close_game_wait <= timedelta(0):
logger.info("Close game during wait")
self.device.app_stop()
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify semantics for non-positive close_game_wait_duration values to avoid surprising behavior.
Right now any non-positive `close_game_wait_duration` (including the default 0) triggers an immediate close on every idle wait, regardless of `next_run`. Previously, a positive limit was a threshold, and 0 often means “no timeout.” If 0 is meant to mean “always close,” this is fine but should be confirmed; otherwise, consider treating `<= 0` as “no auto-close” and only stopping when `next_run > now + close_game_wait`. This also prevents negative misconfigurations from causing overly aggressive closing.
</issue_to_address>
### Comment 2
<location path="script.py" line_range="382-391" />
<code_context>
+ close_emulator_wait_duration = self.config.script.optimization.close_emulator_wait_duration
+ close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
+
+ if close_emulator_wait > timedelta(0) and next_run > datetime.now() + close_emulator_wait:
+ logger.info("Close emulator during wait")
+ self.device.emulator_stop()
+ self._emulator_down = True
+
+ if not self._wait_until_with_emulator_preheat(next_run):
+ return False
+
+ self.run("Restart")
+ return True
+
+ if close_game_wait <= timedelta(0):
logger.info("Close game during wait")
self.device.app_stop()
</code_context>
<issue_to_address>
**suggestion:** Use a single snapshot of `datetime.now()` to avoid slightly inconsistent threshold checks.
Both branches currently call `datetime.now()` separately when comparing to `next_run`. In short waits or high-frequency schedules, the tiny time difference between these calls can lead to one branch triggering while the other doesn’t. Capture `now = datetime.now()` once and reuse it for all comparisons so the behavior is deterministic and easier to reason about.
Suggested implementation:
```python
close_game_wait_duration = self.config.script.optimization.close_game_wait_duration
close_game_wait = self._time_to_timedelta(close_game_wait_duration)
close_emulator_wait_duration = self.config.script.optimization.close_emulator_wait_duration
close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
now = datetime.now()
if close_emulator_wait > timedelta(0) and next_run > now + close_emulator_wait:
```
```python
if next_run > now + close_game_wait:
```
</issue_to_address>
### Comment 3
<location path="script.py" line_range="383-384" />
<code_context>
+ close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
+
+ if close_emulator_wait > timedelta(0) and next_run > datetime.now() + close_emulator_wait:
+ logger.info("Close emulator during wait")
+ self.device.emulator_stop()
+ self._emulator_down = True
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling `emulator_stop` unconditionally may regress previous idempotent behavior when the emulator is already down.
Previously `_wait_close_emulator_or` checked `self._emulator_down` before calling `self.device.emulator_stop()`, and only logged when it was already closed. The new paths (`_wait_close_game`, `_wait_goto_main`) always invoke `emulator_stop()` and then set `_emulator_down = True`. If `emulator_stop()` doesn’t handle “already stopped” cleanly, this change could introduce errors or extra overhead. Consider restoring a `self._emulator_down` guard (or making `emulator_stop` explicitly idempotent) to match the prior behavior.
</issue_to_address>帮我变得更有用！请对每条评论点击 👍 或 👎，我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The logic that decides when to close the emulator during idle waits is now duplicated between
_wait_close_gameand_wait_goto_main; consider extracting a shared helper to keep behavior and configuration handling consistent in one place. - In
_wait_close_game,self.device.emulator_stop()is called unconditionally when the close-emulator wait is exceeded; if the emulator might already be down, consider checking_emulator_down(or similar) first to avoid redundant stop calls or device-layer errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic that decides when to close the emulator during idle waits is now duplicated between `_wait_close_game` and `_wait_goto_main`; consider extracting a shared helper to keep behavior and configuration handling consistent in one place.
- In `_wait_close_game`, `self.device.emulator_stop()` is called unconditionally when the close-emulator wait is exceeded; if the emulator might already be down, consider checking `_emulator_down` (or similar) first to avoid redundant stop calls or device-layer errors.
## Individual Comments
### Comment 1
<location path="script.py" line_range="393" />
<code_context>
+ self.run("Restart")
+ return True
+
+ if close_game_wait <= timedelta(0):
logger.info("Close game during wait")
self.device.app_stop()
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify semantics for non-positive close_game_wait_duration values to avoid surprising behavior.
Right now any non-positive `close_game_wait_duration` (including the default 0) triggers an immediate close on every idle wait, regardless of `next_run`. Previously, a positive limit was a threshold, and 0 often means “no timeout.” If 0 is meant to mean “always close,” this is fine but should be confirmed; otherwise, consider treating `<= 0` as “no auto-close” and only stopping when `next_run > now + close_game_wait`. This also prevents negative misconfigurations from causing overly aggressive closing.
</issue_to_address>
### Comment 2
<location path="script.py" line_range="382-391" />
<code_context>
+ close_emulator_wait_duration = self.config.script.optimization.close_emulator_wait_duration
+ close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
+
+ if close_emulator_wait > timedelta(0) and next_run > datetime.now() + close_emulator_wait:
+ logger.info("Close emulator during wait")
+ self.device.emulator_stop()
+ self._emulator_down = True
+
+ if not self._wait_until_with_emulator_preheat(next_run):
+ return False
+
+ self.run("Restart")
+ return True
+
+ if close_game_wait <= timedelta(0):
logger.info("Close game during wait")
self.device.app_stop()
</code_context>
<issue_to_address>
**suggestion:** Use a single snapshot of `datetime.now()` to avoid slightly inconsistent threshold checks.
Both branches currently call `datetime.now()` separately when comparing to `next_run`. In short waits or high-frequency schedules, the tiny time difference between these calls can lead to one branch triggering while the other doesn’t. Capture `now = datetime.now()` once and reuse it for all comparisons so the behavior is deterministic and easier to reason about.
Suggested implementation:
```python
close_game_wait_duration = self.config.script.optimization.close_game_wait_duration
close_game_wait = self._time_to_timedelta(close_game_wait_duration)
close_emulator_wait_duration = self.config.script.optimization.close_emulator_wait_duration
close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
now = datetime.now()
if close_emulator_wait > timedelta(0) and next_run > now + close_emulator_wait:
```
```python
if next_run > now + close_game_wait:
```
</issue_to_address>
### Comment 3
<location path="script.py" line_range="383-384" />
<code_context>
+ close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
+
+ if close_emulator_wait > timedelta(0) and next_run > datetime.now() + close_emulator_wait:
+ logger.info("Close emulator during wait")
+ self.device.emulator_stop()
+ self._emulator_down = True
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling `emulator_stop` unconditionally may regress previous idempotent behavior when the emulator is already down.
Previously `_wait_close_emulator_or` checked `self._emulator_down` before calling `self.device.emulator_stop()`, and only logged when it was already closed. The new paths (`_wait_close_game`, `_wait_goto_main`) always invoke `emulator_stop()` and then set `_emulator_down = True`. If `emulator_stop()` doesn’t handle “already stopped” cleanly, this change could introduce errors or extra overhead. Consider restoring a `self._emulator_down` guard (or making `emulator_stop` explicitly idempotent) to match the prior behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.run("Restart") | ||
| return True | ||
|
|
||
| if close_game_wait <= timedelta(0): |
There was a problem hiding this comment.
issue (bug_risk): 需要澄清 close_game_wait_duration 为非正值时的语义，以避免产生令人意外的行为。
目前任何非正的 close_game_wait_duration（包括默认值 0）都会在每次空闲等待时立即关闭游戏，而不考虑 next_run。之前，正数上限是一个阈值，而 0 通常意味着“无超时”。如果 0 的语义是“总是关闭”，那这样是可以的，但应当予以确认；否则建议将 <= 0 解释为“不要自动关闭”，只在 next_run > now + close_game_wait 时才停止。这也能防止负值配置错误导致过于激进的关闭行为。
Original comment in English
issue (bug_risk): Clarify semantics for non-positive close_game_wait_duration values to avoid surprising behavior.
Right now any non-positive close_game_wait_duration (including the default 0) triggers an immediate close on every idle wait, regardless of next_run. Previously, a positive limit was a threshold, and 0 often means “no timeout.” If 0 is meant to mean “always close,” this is fine but should be confirmed; otherwise, consider treating <= 0 as “no auto-close” and only stopping when next_run > now + close_game_wait. This also prevents negative misconfigurations from causing overly aggressive closing.
Comment on lines
+382
to
+391
| if close_emulator_wait > timedelta(0) and next_run > datetime.now() + close_emulator_wait: | ||
| logger.info("Close emulator during wait") | ||
| self.device.emulator_stop() | ||
| self._emulator_down = True | ||
|
|
||
| if not self._wait_until_with_emulator_preheat(next_run): | ||
| return False | ||
|
|
||
| self.run("Restart") | ||
| return True |
There was a problem hiding this comment.
suggestion: 使用一次性获取的 datetime.now() 快照，避免略微不一致的阈值判断。
当前两个分支在与 next_run 比较时分别调用了 datetime.now()。在短时间等待或高频调度的场景下，这些调用间的微小时间差可能导致一个分支触发而另一个不触发。建议先获取一次 now = datetime.now()，并在所有比较中复用，使行为更确定、更容易理解。
建议实现：
close_game_wait_duration = self.config.script.optimization.close_game_wait_duration
close_game_wait = self._time_to_timedelta(close_game_wait_duration)
close_emulator_wait_duration = self.config.script.optimization.close_emulator_wait_duration
close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
now = datetime.now()
if close_emulator_wait > timedelta(0) and next_run > now + close_emulator_wait: if next_run > now + close_game_wait:Original comment in English
suggestion: Use a single snapshot of datetime.now() to avoid slightly inconsistent threshold checks.
Both branches currently call datetime.now() separately when comparing to next_run. In short waits or high-frequency schedules, the tiny time difference between these calls can lead to one branch triggering while the other doesn’t. Capture now = datetime.now() once and reuse it for all comparisons so the behavior is deterministic and easier to reason about.
Suggested implementation:
close_game_wait_duration = self.config.script.optimization.close_game_wait_duration
close_game_wait = self._time_to_timedelta(close_game_wait_duration)
close_emulator_wait_duration = self.config.script.optimization.close_emulator_wait_duration
close_emulator_wait = self._time_to_timedelta(close_emulator_wait_duration)
now = datetime.now()
if close_emulator_wait > timedelta(0) and next_run > now + close_emulator_wait: if next_run > now + close_game_wait:
Comment on lines
+383
to
+384
| logger.info("Close emulator during wait") | ||
| self.device.emulator_stop() |
There was a problem hiding this comment.
issue (bug_risk): 无条件调用 emulator_stop 可能会回退之前在模拟器已关闭情况下的幂等行为。
之前 _wait_close_emulator_or 会在调用 self.device.emulator_stop() 前检查 self._emulator_down，并在已关闭时只记录日志。新的路径（_wait_close_game, _wait_goto_main）则总是调用 emulator_stop()，然后设置 _emulator_down = True。如果 emulator_stop() 无法优雅地处理“已停止”的情况，这一变化可能引入错误或额外开销。建议恢复对 self._emulator_down 的判断（或让 emulator_stop 明确实现为幂等），以匹配先前的行为。
Original comment in English
issue (bug_risk): Calling emulator_stop unconditionally may regress previous idempotent behavior when the emulator is already down.
Previously _wait_close_emulator_or checked self._emulator_down before calling self.device.emulator_stop(), and only logged when it was already closed. The new paths (_wait_close_game, _wait_goto_main) always invoke emulator_stop() and then set _emulator_down = True. If emulator_stop() doesn’t handle “already stopped” cleanly, this change could introduce errors or extra overhead. Consider restoring a self._emulator_down guard (or making emulator_stop explicitly idempotent) to match the prior behavior.
Change Summary
Reconstructed Intent点击此处展开这次改动最可能想实现的是:将"关闭游戏"和"关闭模拟器"的触发时机由原来的组合选项改为独立的等待时长阈值,让两者可以在 `close_game` 和 `goto_main` 策略下各自独立生效(通过 `wait_duration > 0` 启用)。原有 `close_emulator_or_*` 两个组合选项被移除,改为在 `_wait_close_game` 和 `_wait_goto_main` 内部各自处理模拟器关闭。Observed Constraints点击此处展开1. **已有配置兼容性**:已有用户配置中 `when_task_queue_empty` 可能保存为已移除的枚举值,启动时 fallback 到 `stay_there` 2. **旧配置字段残留**:pydantic 模型中旧字段 `close_game_limit_time` / `close_emulator_limit_time` 已删除,已有配置加载时这些字段会被忽略(pydantic 默认忽略额外字段) 3. **`goto_main` 语义变化**:中文标签"前往主界面"→"前往庭院",可能影响 UI 显示文本一致性(需确认这是翻译修正还是业务语义变化) 4. **`close_game_wait <= 0` 时行为**:`close_game_wait <= timedelta(0)` 时会执行关闭游戏(不是跳过),help text 说"设为0表示禁用"与实际行为矛盾——不过如果意图是"0表示立即关闭"则此点可忽略Intent Alignment
Release Risk
Validation Gaps点击此处展开- `close_game_wait_duration = 0` 时 `_wait_close_game` 的实际行为与 i18n help text 不符,需作者确认语义 - 已移除枚举值在已有配置中的兼容处理是否经过测试 - i18n 中 `goto_main` 标签变更是否与 UI 层(如 `GameUi` 中的相关引用)保持一致
|
Findings
|
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 Sourcery
通过区分游戏和模拟器的关闭时机并简化相应策略，优化空闲任务队列的处理。
Enhancements:
Original summary in English
Summary by Sourcery
Refine idle task queue handling by separating game and emulator shutdown timing and simplifying the corresponding strategies.
Enhancements: