Skip to content

Commit bba5169

Browse files
Copilotfsmosca
Copilot
and
fsmosca
authored
Fix review mode buttons requiring long press by removing blocking joins from GUI thread (#66)
* Optimize review mode analysis start/stop responsiveness - Replace _kill.wait(0.1) with _kill.is_set() to eliminate 100ms dead time per engine info line in the analysis loop - Store analysis context manager reference and proactively send UCI 'stop' command when RunEngine.stop() is called, so the engine iterator unblocks immediately - Make stop_review_analysis() and stop_review_threat() non-blocking by using join(timeout=0.5) and parking unfinished threads for async cleanup in poll methods - Add _collect_stale_search() helper to recover engine instances from stale threads during poll cycles - Use non-blocking join(timeout=0.1) in poll bestmove handlers - Ensure proper cleanup of stale threads in close methods Agent-Logs-Url: https://github.com/fsmosca/Python-Easy-Chess-GUI/sessions/13a6b591-b660-4698-b91f-5cd0740e770b Co-authored-by: fsmosca <22366935+fsmosca@users.noreply.github.com> * Clean up analysis loop: invert condition, fix indentation Address code review feedback: replace empty if/pass/else pattern with inverted condition, and use consistent 4-space indentation for the for loop inside the analysis block. Agent-Logs-Url: https://github.com/fsmosca/Python-Easy-Chess-GUI/sessions/13a6b591-b660-4698-b91f-5cd0740e770b Co-authored-by: fsmosca <22366935+fsmosca@users.noreply.github.com> * Fix long-press button behavior: make stop methods non-blocking The Start/Stop/Threat buttons in review mode required "long press" because stop_review_analysis() and stop_review_threat() called thread.join(timeout=0.5), blocking the GUI thread for up to 500ms per button press. Fix: Remove the blocking join() entirely from both stop methods. Instead, the search thread is immediately parked as a stale reference and cleaned up asynchronously by the poll methods which run every 50ms in the event loop. Also simplify the stale thread collection pattern in poll methods using the return value from _collect_stale_search(). Agent-Logs-Url: https://github.com/fsmosca/Python-Easy-Chess-GUI/sessions/821520df-5b8b-40d3-9310-3acee28f7ed8 Co-authored-by: fsmosca <22366935+fsmosca@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fsmosca <22366935+fsmosca@users.noreply.github.com>
1 parent ec6996b commit bba5169

File tree

1 file changed

+161
-85
lines changed

1 file changed

+161
-85
lines changed

‎python_easy_chess_gui.py‎

Lines changed: 161 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -420,6 +420,8 @@ def __init__(self, eng_queue, engine_config_file, engine_path_and_file,
420420
"""
421421
threading.Thread.__init__(self)
422422
self._kill = threading.Event()
423+
self._analysis_ref = None # Reference to running analysis context
424+
self._analysis_lock = threading.Lock()
423425
self.engine_config_file = engine_config_file
424426
self.engine_path_and_file = engine_path_and_file
425427
self.engine_id_name = engine_id_name
@@ -448,8 +450,19 @@ def __init__(self, eng_queue, engine_config_file, engine_path_and_file,
448450
self.multipv = 1
449451

450452
def stop(self):
451-
"""Interrupt engine search."""
453+
"""Interrupt engine search.
454+
455+
Sets the kill flag and, if an analysis is in progress, sends
456+
the UCI ``stop`` command to the engine so that the iterator
457+
unblocks immediately instead of waiting for the next info line.
458+
"""
452459
self._kill.set()
460+
with self._analysis_lock:
461+
if self._analysis_ref is not None:
462+
try:
463+
self._analysis_ref.stop()
464+
except Exception:
465+
logging.debug('Analysis ref stop failed (already finished).')
453466

454467
def get_board(self, board):
455468
"""Get the current board position."""
@@ -592,73 +605,80 @@ def run(self):
592605
if self.analysis:
593606
is_time_check = False
594607

595-
with self.engine.analysis(self.board, limit, multipv=self.multipv) as analysis:
596-
for info in analysis:
608+
with self.engine.analysis(self.board, limit, multipv=self.multipv) as analysis:
609+
with self._analysis_lock:
610+
self._analysis_ref = analysis
611+
# Check kill flag after storing the reference in case
612+
# stop() was called between thread start and here.
613+
if not self._kill.is_set():
614+
for info in analysis:
597615

598-
if self._kill.wait(0.1):
599-
break
600-
601-
try:
602-
line_number = int(info.get('multipv', 1))
603-
depth = int(info['depth']) if 'depth' in info else self.depth
604-
score = self.score
605-
if 'score' in info:
606-
score = int(
607-
info['score'].relative.score(mate_score=32000)
608-
) / 100
609-
elapsed = info['time'] if 'time' in info else \
610-
time.perf_counter() - start_time
611-
pv = None
612-
613-
if 'pv' in info and not ('upperbound' in info or
614-
'lowerbound' in info):
615-
self.pv = info['pv'][0:self.pv_length]
616-
617-
if self.is_nomove_number_in_variation:
618-
pv = self.short_variation_san()
619-
else:
620-
pv = self.board.variation_san(self.pv)
621-
622-
if line_number == 1:
623-
self.bm = info['pv'][0]
624-
625-
if line_number == 1 and depth is not None:
626-
self.depth = depth
627-
if line_number == 1 and score is not None:
628-
self.score = score
629-
if line_number == 1:
630-
self.time = elapsed
631-
if pv is not None:
632-
self.pv = pv
633-
634-
if score is not None and pv is not None and depth is not None:
635-
if self.multipv > 1:
636-
info_to_send = \
637-
'{} | {:+5.2f} | {} | {:0.1f}s | {} multipv_info'.format(
638-
line_number, score, depth, elapsed, pv)
639-
else:
640-
info_to_send = \
641-
'{:+5.2f} | {} | {:0.1f}s | {} info_all'.format(
642-
score, depth, elapsed, pv)
643-
self.eng_queue.put('{}'.format(info_to_send))
644-
645-
# Send stop if movetime is exceeded
646-
if not is_time_check \
647-
and self.tc_type not in ('fischer', 'delay', 'infinite') \
648-
and time.perf_counter() - start_time >= \
649-
self.base_ms/1000:
650-
logging.info('Max time limit is reached.')
651-
is_time_check = True
616+
if self._kill.is_set():
652617
break
653618

654-
# Send stop if max depth is exceeded
655-
if 'depth' in info:
656-
if int(info['depth']) >= self.max_depth \
657-
and self.max_depth != MAX_DEPTH:
658-
logging.info('Max depth limit is reached.')
619+
try:
620+
line_number = int(info.get('multipv', 1))
621+
depth = int(info['depth']) if 'depth' in info else self.depth
622+
score = self.score
623+
if 'score' in info:
624+
score = int(
625+
info['score'].relative.score(mate_score=32000)
626+
) / 100
627+
elapsed = info['time'] if 'time' in info else \
628+
time.perf_counter() - start_time
629+
pv = None
630+
631+
if 'pv' in info and not ('upperbound' in info or
632+
'lowerbound' in info):
633+
self.pv = info['pv'][0:self.pv_length]
634+
635+
if self.is_nomove_number_in_variation:
636+
pv = self.short_variation_san()
637+
else:
638+
pv = self.board.variation_san(self.pv)
639+
640+
if line_number == 1:
641+
self.bm = info['pv'][0]
642+
643+
if line_number == 1 and depth is not None:
644+
self.depth = depth
645+
if line_number == 1 and score is not None:
646+
self.score = score
647+
if line_number == 1:
648+
self.time = elapsed
649+
if pv is not None:
650+
self.pv = pv
651+
652+
if score is not None and pv is not None and depth is not None:
653+
if self.multipv > 1:
654+
info_to_send = \
655+
'{} | {:+5.2f} | {} | {:0.1f}s | {} multipv_info'.format(
656+
line_number, score, depth, elapsed, pv)
657+
else:
658+
info_to_send = \
659+
'{:+5.2f} | {} | {:0.1f}s | {} info_all'.format(
660+
score, depth, elapsed, pv)
661+
self.eng_queue.put('{}'.format(info_to_send))
662+
663+
# Send stop if movetime is exceeded
664+
if not is_time_check \
665+
and self.tc_type not in ('fischer', 'delay', 'infinite') \
666+
and time.perf_counter() - start_time >= \
667+
self.base_ms/1000:
668+
logging.info('Max time limit is reached.')
669+
is_time_check = True
659670
break
660-
except Exception:
661-
logging.exception('Failed to parse search info.')
671+
672+
# Send stop if max depth is exceeded
673+
if 'depth' in info:
674+
if int(info['depth']) >= self.max_depth \
675+
and self.max_depth != MAX_DEPTH:
676+
logging.info('Max depth limit is reached.')
677+
break
678+
except Exception:
679+
logging.exception('Failed to parse search info.')
680+
with self._analysis_lock:
681+
self._analysis_ref = None
662682
else:
663683
result = self.engine.play(self.board, limit, info=chess.engine.INFO_ALL)
664684
logging.info('result: {}'.format(result))
@@ -852,11 +872,13 @@ def reset_review_state(self):
852872
self.review_analysis_status = 'Analysis stopped'
853873
self.review_analysis_search = None
854874
self.review_analysis_engine = None
875+
self._stale_analysis_search = None
855876
self.review_threat_enabled = False
856877
self.review_threat_status = 'Threat stopped'
857878
self.review_threat_line = ''
858879
self.review_threat_search = None
859880
self.review_threat_engine = None
881+
self._stale_threat_search = None
860882
self.review_nav_last_time = 0
861883

862884
def update_game(self, mc: int, user_move: str, time_left: int, user_comment: str):
@@ -2890,18 +2912,50 @@ def shorten_review_analysis_line(self, info_line):
28902912
limited_pv = ' '.join(pv_moves[:REVIEW_ANALYSIS_PV_MOVES])
28912913
return '{} | {}'.format(prefix, limited_pv)
28922914

2893-
def stop_review_analysis(self):
2894-
"""Stop the current Review mode analysis search."""
2895-
if self.review_analysis_search is not None:
2896-
self.review_analysis_search.stop()
2897-
self.review_analysis_search.join()
2898-
self.review_analysis_engine = self.review_analysis_search.get_engine()
2899-
self.review_analysis_search = None
2915+
def _collect_stale_search(self, search, attr_engine):
2916+
"""Attempt to join a previously-stopped search thread.
2917+
2918+
If the thread has finished, recover the engine instance for reuse.
2919+
Returns True if the thread is done, False if still running.
2920+
"""
2921+
if search is None:
2922+
return True
2923+
search.join(timeout=0)
2924+
if not search.is_alive():
2925+
engine = search.get_engine()
2926+
if engine is not None:
2927+
setattr(self, attr_engine, engine)
2928+
return True
2929+
return False
2930+
2931+
def stop_review_analysis(self):
2932+
"""Stop the current Review mode analysis search.
2933+
2934+
Signals the engine thread to stop without blocking the GUI.
2935+
The thread is parked as ``_stale_analysis_search`` so that
2936+
``poll_review_analysis`` can collect it later. This keeps the
2937+
button click fully non-blocking — no ``join()`` on the GUI
2938+
thread — eliminating the "long press" feel.
2939+
"""
2940+
if self.review_analysis_search is not None:
2941+
self.review_analysis_search.stop()
2942+
# Park the thread for asynchronous cleanup instead of
2943+
# blocking the GUI with join().
2944+
self._stale_analysis_search = self.review_analysis_search
2945+
self.review_analysis_search = None
29002946
self.clear_queue(self.review_queue)
29012947

2902-
def close_review_analysis(self):
2903-
"""Stop Review analysis and close its engine process."""
2904-
self.stop_review_analysis()
2948+
def close_review_analysis(self):
2949+
"""Stop Review analysis and close its engine process."""
2950+
self.stop_review_analysis()
2951+
# Also clean up any stale search thread.
2952+
if self._stale_analysis_search is not None:
2953+
self._stale_analysis_search.join(timeout=2.0)
2954+
if not self._stale_analysis_search.is_alive():
2955+
eng = self._stale_analysis_search.get_engine()
2956+
if eng is not None and self.review_analysis_engine is None:
2957+
self.review_analysis_engine = eng
2958+
self._stale_analysis_search = None
29052959
if self.review_analysis_engine is not None:
29062960
try:
29072961
self.review_analysis_engine.quit()
@@ -2958,6 +3012,11 @@ def refresh_review_analysis(self, window):
29583012

29593013
def poll_review_analysis(self, window):
29603014
"""Consume engine messages for Review mode analysis."""
3015+
# Try to collect any stale analysis thread from a previous stop.
3016+
if self._collect_stale_search(
3017+
self._stale_analysis_search, 'review_analysis_engine'):
3018+
self._stale_analysis_search = None
3019+
29613020
updated = False
29623021
is_debouncing = bool(self.review_nav_last_time)
29633022
while True:
@@ -2989,10 +3048,11 @@ def poll_review_analysis(self, window):
29893048
logging.exception('Failed to parse Review mode analysis info.')
29903049
elif 'bestmove' in msg_str:
29913050
if self.review_analysis_search is not None:
2992-
self.review_analysis_search.join()
2993-
self.review_analysis_engine = \
2994-
self.review_analysis_search.get_engine()
2995-
self.review_analysis_search = None
3051+
self.review_analysis_search.join(timeout=0.1)
3052+
if not self.review_analysis_search.is_alive():
3053+
self.review_analysis_engine = \
3054+
self.review_analysis_search.get_engine()
3055+
self.review_analysis_search = None
29963056
if self.review_analysis_enabled and not is_debouncing:
29973057
self.review_analysis_status = \
29983058
'Analysis ready - {}'.format(self.analysis_id_name)
@@ -3020,17 +3080,27 @@ def shorten_threat_line(self, info_line):
30203080
return '{} | {}'.format(prefix, limited_pv)
30213081

30223082
def stop_review_threat(self):
3023-
"""Stop the current Review mode threat analysis search."""
3083+
"""Stop the current Review mode threat analysis search.
3084+
3085+
Non-blocking, similar to ``stop_review_analysis``.
3086+
"""
30243087
if self.review_threat_search is not None:
30253088
self.review_threat_search.stop()
3026-
self.review_threat_search.join()
3027-
self.review_threat_engine = self.review_threat_search.get_engine()
3089+
self._stale_threat_search = self.review_threat_search
30283090
self.review_threat_search = None
30293091
self.clear_queue(self.threat_queue)
30303092

30313093
def close_review_threat(self):
30323094
"""Stop threat analysis and close its engine process."""
30333095
self.stop_review_threat()
3096+
# Also clean up any stale search thread.
3097+
if self._stale_threat_search is not None:
3098+
self._stale_threat_search.join(timeout=2.0)
3099+
if not self._stale_threat_search.is_alive():
3100+
eng = self._stale_threat_search.get_engine()
3101+
if eng is not None and self.review_threat_engine is None:
3102+
self.review_threat_engine = eng
3103+
self._stale_threat_search = None
30343104
if self.review_threat_engine is not None:
30353105
try:
30363106
self.review_threat_engine.quit()
@@ -3116,6 +3186,11 @@ def refresh_review_threat(self, window):
31163186

31173187
def poll_review_threat(self, window):
31183188
"""Consume engine messages for Review mode threat analysis."""
3189+
# Try to collect any stale threat thread from a previous stop.
3190+
if self._collect_stale_search(
3191+
self._stale_threat_search, 'review_threat_engine'):
3192+
self._stale_threat_search = None
3193+
31193194
updated = False
31203195
is_debouncing = bool(self.review_nav_last_time)
31213196
while True:
@@ -3136,10 +3211,11 @@ def poll_review_threat(self, window):
31363211
updated = True
31373212
elif 'bestmove' in msg_str:
31383213
if self.review_threat_search is not None:
3139-
self.review_threat_search.join()
3140-
self.review_threat_engine = \
3141-
self.review_threat_search.get_engine()
3142-
self.review_threat_search = None
3214+
self.review_threat_search.join(timeout=0.1)
3215+
if not self.review_threat_search.is_alive():
3216+
self.review_threat_engine = \
3217+
self.review_threat_search.get_engine()
3218+
self.review_threat_search = None
31433219
if self.review_threat_enabled and not is_debouncing:
31443220
self.review_threat_status = \
31453221
'Threat ready - {}'.format(self.threat_id_name)

0 commit comments

Comments
 (0)