feat: Enhance emulator selection for macOS with AppleScript integrati… #69
Conversation
…on and fallback to Tk dialog
There was a problem hiding this comment.
Pull request overview
This PR enhances the emulator selection dialog for macOS by introducing native AppleScript integration with automatic fallback to the Tk file dialog. The primary improvement addresses the issue where macOS Tk dialogs can show .app bundles as grayed out or unselectable on certain macOS/Tk versions.
Key changes:
- Implements native macOS file picker using AppleScript's "choose file" command for better .app bundle selection
- Adds comprehensive error handling with fallback to Tk dialog when AppleScript fails or is unavailable
- Improves .app bundle path normalization to handle cases where users navigate inside bundles
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ui/pages/settings_page.py
Outdated
|
|
||
| if system == "Darwin": | ||
| try: | ||
| import subprocess |
There was a problem hiding this comment.
The subprocess module is imported inside the try block on line 714, but it's only used within the macOS-specific Darwin branch. This import should be moved to the top of the file with the other imports to follow Python best practices and improve code clarity. Local imports inside conditional blocks can make the code harder to maintain and understand.
ui/pages/settings_page.py
Outdated
| filename = filedialog.askopenfilename( | ||
| title="Select Emulator", | ||
| filetypes=filetypes, | ||
| initialdir=initialdir, | ||
| ) | ||
| except Exception: | ||
| # osascript not available/other error -> fallback to Tk dialog | ||
| filename = filedialog.askopenfilename( | ||
| title="Select Emulator", | ||
| filetypes=filetypes, | ||
| initialdir=initialdir, | ||
| ) | ||
| else: | ||
| filename = filedialog.askopenfilename( | ||
| title="Select Emulator", | ||
| filetypes=filetypes, | ||
| initialdir=initialdir, | ||
| mustexist=True, | ||
| ) |
There was a problem hiding this comment.
There is significant code duplication where the same filedialog.askopenfilename call with identical parameters appears three times (lines 735-739, 742-746, and 748-752). Consider extracting this into a helper function or storing it once in a variable to improve maintainability and reduce duplication.
ui/pages/settings_page.py
Outdated
| if not filename: | ||
| return | ||
|
|
||
| if filename: |
There was a problem hiding this comment.
The condition on line 762 checking 'if filename:' is redundant because there's already an early return on line 760 when 'not filename' is true. Any code after line 760 is guaranteed to have a truthy filename value, making this check unnecessary.
ui/pages/settings_page.py
Outdated
| except Exception: | ||
| # osascript not available/other error -> fallback to Tk dialog | ||
| filename = filedialog.askopenfilename( | ||
| title="Select Emulator", | ||
| filetypes=filetypes, | ||
| initialdir=initialdir, | ||
| ) |
There was a problem hiding this comment.
The bare except clause on line 740 catches all exceptions without any logging or specific handling. This can hide important errors and make debugging difficult. Consider catching more specific exceptions or at least logging the error to help with troubleshooting when issues occur.
ui/pages/settings_page.py
Outdated
| if filename and system == "Darwin" and ".app/" in filename: | ||
| filename = filename.split(".app/")[0] + ".app" |
There was a problem hiding this comment.
The .app bundle normalization logic using string splitting on ".app/" could incorrectly handle edge cases. For example, if a .app bundle path contains multiple ".app/" substrings in its full path (e.g., "/Users/name/MyApp.app/Contents/Resources/Another.app/file"), this will only split on the first occurrence. Consider using a more robust approach like finding the last occurrence of ".app/" or using path manipulation to ensure the bundle root is correctly identified.
| if filename and system == "Darwin" and ".app/" in filename: | |
| filename = filename.split(".app/")[0] + ".app" | |
| if filename and system == "Darwin" and ".app" in filename: | |
| app_index = filename.rfind(".app") | |
| if app_index != -1: | |
| filename = filename[: app_index + 4] |
…on and fallback to Tk dialog