Edge app #663
Edge app #663
Conversation
| import subprocess as sps | ||
| import sys | ||
| from typing import List | ||
|
|
||
| from eel.types import OptionsDictT | ||
|
|
||
| name: str = 'Edge' | ||
| name: str = "Edge" |
There was a problem hiding this comment.
Although admittedly there are a few inconsistencies, the eel package generally prefers single quotes for strings. I think it would be good if you could change these back throughout.
| cmd = 'start microsoft-edge:{}'.format(start_urls[0]) | ||
| sps.Popen(cmd, stdout=sys.stdout, stderr=sys.stderr, stdin=sps.PIPE, shell=True) | ||
| def run(path: str, options: OptionsDictT, start_urls: List[str]) -> None: | ||
| if path != "edge_legacy": |
There was a problem hiding this comment.
I would propose passing back the string 'start microsoft-edge' from _find_edge_win(). For two reasons:
- This ensures that what is returned by _find_edge_win() is always executable in principle if it is not
None. - Users may locally introduce an alias for a binary, e.g. when they have several browser versions running, that they might reasonably call edge_legacy, so that any errors (even if unlikely) caused by a name conflict on the system would be untraceable.
| if path != "edge_legacy": | ||
| if options["app_mode"]: | ||
| for url in start_urls: | ||
| sps.Popen([path, "--app=%s" % url] + options["cmdline_args"], stdout=sps.PIPE, stderr=sps.PIPE, stdin=sps.PIPE) |
There was a problem hiding this comment.
For consistency I'd suggest you break the long lines here for the calls to sps.Popen here and below, similar to how they are implemented in chrome.py?
| sps.Popen([path, "--new-window"] + args, stdout=sps.PIPE, stderr=sps.PIPE, stdin=sps.PIPE) | ||
| else: | ||
| cmd = "start microsoft-edge:{}".format(start_urls[0]) | ||
| sps.Popen(cmd, stdout=sps.PIPE, stderr=sps.PIPE, stdin=sps.PIPE, shell=True) | ||
|
|
||
|
|
||
| def find_path() -> bool: |
There was a problem hiding this comment.
find_path() does not return a boolean anymore, so the type annotation must be changed or it won't pass the static typing tests. I believe this should return Optional[str] now.
|
|
||
| return False | ||
|
|
||
| def _find_edge_win(): |
There was a problem hiding this comment.
This function needs a return type annotation, str is best if you're confident it will always return a string.
| def _find_edge_win(): | ||
| import winreg as reg | ||
|
|
||
| reg_path = r"SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\msedge.exe" |
There was a problem hiding this comment.
While semi-obvious, it would be good to annotate reg_path as reg_path: str
| else: | ||
| break | ||
|
|
||
| return edge_path |
There was a problem hiding this comment.
I see two potential issues with this loop:
-
edge_pathhere is potentially unbound (think what happens if for God knows what reason no WindowsError is ever raised and Edge not found), and its type is also technically indeterminate, which is a bad situation.To address this, I'd suggest setting
edge_path: str = 'start microsoft-edge'as a fallback value before entering the loop. -
Your loop runs exactly twice, once for
reg.HKEY_CURRENT_USERand once forreg.HKEY_LOCAL_MACHINE. These options are fixed and there is no reason we should think that further iterations should become available in the future, so the loop can be unrolled.I would take the point that it's rather ugly to write this twice in a row, so what I would do is add a separate function to query the registry, e.g.
_query_winreg(key: str, sub_key: str) -> Optional[str]which implements the checking logic, and returns either the string ifos.path.isfile(edge_path)is True and no WindowsError (for sub_key not found) is raised.
You could then set just doedge_path = _query_winreg(reg.HKEY_LOCAL_MACHINE, reg_path) or edge_path return _query_winreg(reg.HKEY_CURRENT_USER, reg_path) or edge_pathwhich would make this quite clean and more readable in my opinion (though one can argue about hijacking
orto select between truthy and falsy values.
Edge running in App Mode, code from #251
All credit to @ncotrb
I was going to be good and write tests but currently tests only support Chrome. Selenium works with Edge in v4 but that is Python 3.7+. I couldn't get selenium 3 working with edge, I didn't try very hard but its not well supported I know that much. I could have crack at python 3.7+ tests maybe?
Thanks