🔨 Refactor main window position control logic

[guiyanakuang]

close #2928

[CompileFuture2024]

Claude finished @guiyanakuang's task —— View job

---

PR Review: Refactor main window position control logic

Review Checklist:

• Analyze changed files and understand refactoring
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage implications
• Provide final recommendations

Understanding the Changes:

This refactoring removes complex, platform-specific window positioning logic and consolidates window state management. The key changes are:

1. Removed from DesktopAppWindowManager (20 lines):

   • _mainWindowState StateFlow and related methods
   • setMainWindowState() and getMainWindowState() methods

2. Removed from platform-specific tray views (~70 lines total):

   • refreshWindowPosition() functions in MacTrayView, LinuxTrayView, WindowsTrayView
   • Complex positioning calculations based on screen bounds, insets, and tray locations
   • Platform-specific window positioning logic

3. Simplified MainWindow (9 lines added):

   • Now uses rememberWindowState() with WindowPosition(Alignment.Center)
   • Removes dependency on window manager for positioning

Code Quality & Best Practices ✅

Positive aspects:

• Significant simplification: Removes 113 lines of complex positioning code
• Better separation of concerns: Window positioning is now handled by the UI layer rather than the business logic layer
• Compose-idiomatic: Uses rememberWindowState() which is the standard Compose Desktop pattern
• Reduced coupling: MainWindow no longer depends on AppWindowManager for positioning

Consistency: The refactoring maintains consistency with Compose Desktop best practices.

Potential Bugs & Issues ⚠️

Behavioral Changes:

1. Window positioning behavior change:

   • Before: Complex platform-specific positioning (near tray, edge of screen)
   • After: Simple center positioning
   • Impact: Users will notice the window now always opens centered instead of positioned relative to the tray

2. Dynamic repositioning removed:

   • The old code repositioned windows on mouse clicks (MacTrayMouseClicked used to call refreshWindowPosition)
   • This behavior is now lost

Questions for consideration:

• Was the complex positioning logic removed intentionally or as part of cleanup?
• Are there user experience implications of always centering the window?

Performance Considerations ✅

Improvements:

• Reduced state management: Eliminates StateFlow updates for window positioning
• Simpler calculations: No complex screen bounds/insets calculations on every positioning
• Better memory usage: Removes unnecessary state storage in AppWindowManager

No performance regressions identified.

Security Concerns ✅

No security issues identified. The changes are UI-focused and don't affect:

• Data handling
• Network communication
• File system access
• Cryptographic operations

Test Coverage Implications ⚠️

Potential test gaps:

• Unit tests: The complex positioning logic that was removed likely had associated tests that may now be obsolete
• Integration tests: Window positioning behavior tests may need updates
• Platform-specific tests: Each platform's positioning logic tests should be reviewed

Recommendations:

• Review and update tests related to window positioning
• Add tests for the new centered positioning behavior
• Remove tests for the deleted setMainWindowState/getMainWindowState methods

Final Recommendations

Overall Assessment: ✅ Good refactoring with minor concerns

Strengths:

• Excellent code simplification (113 lines removed)
• More maintainable and easier to understand
• Follows Compose Desktop best practices
• Reduces complexity without introducing bugs

Considerations:

1. User Experience: Verify that centering windows is the intended behavior change
2. Documentation: Consider updating any documentation that references the old positioning behavior
3. Testing: Update test suites to reflect the new behavior

Verdict: Approve with minor considerations

This is a well-executed refactoring that significantly improves code maintainability. The behavioral change to center-positioning appears intentional and simplifies the user experience. The code quality improvements outweigh the minor concerns.

---

[Copilot]

Pull Request Overview

This PR refactors the main window positioning by dropping per-OS manual placement and switching to Compose’s rememberWindowState with a centered default.

• Removed OS-specific refreshWindowPosition implementations in each tray view.
• Updated MainWindow to use rememberWindowState centered instead of manager-driven state.
• Deleted the mainWindowState flow and related APIs from DesktopAppWindowManager.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
app/src/desktopMain/kotlin/com/crosspaste/ui/WindowsTrayView.kt	Removed Windows-specific positioning logic and initial refresh call
app/src/desktopMain/kotlin/com/crosspaste/ui/MainWindow.kt	Introduced rememberWindowState with centered position
app/src/desktopMain/kotlin/com/crosspaste/ui/MacTrayView.kt	Dropped macOS-specific refreshWindowPosition code
app/src/desktopMain/kotlin/com/crosspaste/ui/LinuxTrayView.kt	Dropped Linux-specific refreshWindowPosition code
app/src/desktopMain/kotlin/com/crosspaste/app/DesktopAppWindowManager.kt	Deleted mainWindowState flow and setter/getter APIs

Comments suppressed due to low confidence (2)

app/src/desktopMain/kotlin/com/crosspaste/ui/WindowsTrayView.kt:81

• Initial window positioning logic was removed; add tests to verify that the main window now appears centered on first launch under Windows.

            }

app/src/desktopMain/kotlin/com/crosspaste/ui/MainWindow.kt:44

• [nitpick] MainWindow now uses rememberWindowState with a centered default; consider adding a brief comment explaining why we moved away from per-OS placement logic.

    val mainWindowState =

[Copilot]

[nitpick] Since this method now only forwards the event to mouseClickedAction, consider inlining or removing it to reduce indirection and simplify the click handler.