Skip to content

fix(pet): keep desktop pet on the active display#66

Open
wjl2023 wants to merge 3 commits intolsdefine:mainfrom
wjl2023:fix-desktop-pet-multimonitor
Open

fix(pet): keep desktop pet on the active display#66
wjl2023 wants to merge 3 commits intolsdefine:mainfrom
wjl2023:fix-desktop-pet-multimonitor

Conversation

@wjl2023
Copy link
Copy Markdown
Contributor

@wjl2023 wjl2023 commented Apr 14, 2026

Summary

  • keep the desktop pet on a visible area of the correct display in multi-monitor setups
  • track the display of the GenericAgent window and move the pet when the app window changes monitors
  • exit the desktop pet automatically when the parent GenericAgent process closes
  • avoid snapping the pet back after manual dragging by only repositioning when the target display actually changes
  • keep the fallback to the legacy desktop_pet.pyw launcher path in stapp.py

Validation

  • python -m py_compile frontends/stapp.py
  • py_compile.compile('frontends/desktop_pet_v2.pyw', doraise=True)

Copy link
Copy Markdown
Owner

@lsdefine lsdefine left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The multi-monitor tracking direction is solid, and I appreciate the careful handling of drag-vs-reposition (_rect_key dedup). A few things to address before merging:


🔴 Critical Bug: Singleton Pet + Single Parent PID = Regression

The pet uses a singleton pattern (port 51983) — only one instance runs regardless of how many GA sessions exist. But --parent-pid binds the pet to exactly one GA process.

GA's primary usage pattern is multiple concurrent sessions (several stapp tabs in parallel), not one-GA-one-pet. So:

  1. GA-1 starts → spawns pet with --parent-pid=GA1_PID
  2. GA-2 starts → port taken, reuses existing pet (singleton working correctly)
  3. GA-1 closes → pet detects parent dead → pet exits
  4. GA-2 still running, but pet is gone

This is a regression — before this PR, closing one GA session didn't kill the pet. Now it does.

Suggested fix: Track a set of parent PIDs. Each GA that connects (or spawns) should register its PID via the existing HTTP server (e.g. POST /register?pid=xxx). The pet only exits when all registered PIDs are dead.


🔴 os.kill(pid, 0) Is Unreliable on Windows

def _parent_alive(parent_pid):
    try:
        os.kill(parent_pid, 0)
        return True
    except OSError:
        return False

On Windows, os.kill(pid, 0) behavior varies across Python versions — some call OpenProcess, others call TerminateProcess. Also susceptible to PID reuse.

Safer alternative:

if sys.platform == 'win32':
    kernel32 = ctypes.windll.kernel32
    h = kernel32.OpenProcess(0x100000, False, pid)  # SYNCHRONIZE
    if h:
        kernel32.CloseHandle(h)
        return True
    return False

🟡 Suggestions

  • _find_streamlit_screen_rect is ~70 lines with two unrelated platform paths — Consider splitting into _find_screen_rect_darwin() and _find_screen_rect_win32(), each ~30 lines and independently readable.

  • ctypes structs (RECT, MONITORINFO) defined inside the function — They get recreated every call. Moving them to module level avoids redundant class creation every 2 seconds.

  • import re added but appears unused in this diff — please remove if not needed.


🟢 What's Good

  • Pure helper functions (_clamp, _pet_position, _rect_key) — clean, no side effects
  • argparse for CLI params — clean approach
  • _rect_key preventing snap-back after manual drag — thoughtful UX detail
  • stapp.py change is minimal (just passing args) — good change radius control
  • PetBase abstraction with platform subclasses — good composability

Please fix the singleton + parent PID regression first (the critical one), then this should be good to go. The other items can be addressed here or in a follow-up. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants