-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support more browsers #340
Conversation
The system default browser is detected and opened if it is of type firefox, safari or edge. Otherwise we fallback on chrome.
3c02a98
to
4d6194a
Compare
I got an error testing on Dec 5:
|
It turns out there's no easy cross platform way to detect the users default browser. You actually can't uninstall safari on macs so I just use that on MacOs. On Windows I attempt to use edge then chrome then firefox and on Linux Chrome then firefox. Reporting an error if all fail. |
Hmm, that is annoying; why don't we just fall through all of the browsers regardless of os? So test Safari first, then if that errors try and open firefox, then chrome, etc. and not worry about the os at all. |
Good idea, that is simpler. |
mentat/vision/vision_manager.py
Outdated
self.driver = webdriver.Chrome(service=service) | ||
try: | ||
self.driver = webdriver.Safari() | ||
except Exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except Exception: | |
except Exception as e: | |
if "remote automation" in str(e).lower(): | |
stream.send(f"To use Safari, enable remote automation.", color="red") |
The first time I tried /screenshot
, it failed (didn't recognize Safari, no backups installed). I found I needed to:
- In the Safari menu bar, click 'Safari' and then 'Preferences'.
- Go to the 'Advanced' tab and check 'Show Develop menu in menu bar'.
- Open the 'Develop' menu in the menu bar and choose 'Allow Remote Automation'.
..and then it would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks. I think given that I'll try to open with chrome before telling users to enable remote automation. But if they have safari installed I'll print a different message informing them of that option.
stream.send( | ||
'No browser open. Run "/screenshot path" with a url or local file', | ||
color="red", | ||
"Error taking screenshot. Please run with a valid url or local path.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if this except was inside vision_manager.screenshot; that way we don't have to rewrite this except if we decide to use .screenshot again somewhere.
mentat/vision/vision_manager.py
Outdated
if safari_installed: | ||
ctx.stream.send( | ||
"No suitable browser found. To use Safari, enable" | ||
" remote automation. Alternatively install Chrome.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" remote automation. Alternatively install Chrome.", | |
" remote automation.", |
mentat/vision/vision_manager.py
Outdated
"No suitable browser found. Install Chrome or" | ||
" Firefox.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"No suitable browser found. Install Chrome or" | |
" Firefox.", | |
"No suitable browser found.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of really small comments, but it looks good to me! I can't test this but it looks like @granawkins already tested this for Safari so I'm ok approving it.
The system default browser is detected and opened if it is of type firefox, safari or edge. Otherwise we fallback on chrome.