Skip to content
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

Edge app #663

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ Eel.egg-info
*.swp
venv/
.tox
.vscode
44 changes: 36 additions & 8 deletions eel/edge.py
Original file line number Diff line number Diff line change
@@ -1,20 +1,48 @@
import platform
import os
import subprocess as sps
import sys
from typing import List

from eel.types import OptionsDictT

name: str = 'Edge'
name: str = "Edge"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.



def run(_path: str, options: OptionsDictT, start_urls: List[str]) -> None:
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":
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose passing back the string 'start microsoft-edge' from _find_edge_win(). For two reasons:

  1. This ensures that what is returned by _find_edge_win() is always executable in principle if it is not None.
  2. 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 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

else:
args = options["cmdline_args"] + start_urls
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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if platform.system() == 'Windows':
return True
if sys.platform in ["win32", "win64"]:
return _find_edge_win()
else:
return None

return False

def _find_edge_win():
Copy link
Contributor

Choose a reason for hiding this comment

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

This function needs a return type annotation, str is best if you're confident it will always return a string.

import winreg as reg

reg_path = r"SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\msedge.exe"
Copy link
Contributor

Choose a reason for hiding this comment

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

While semi-obvious, it would be good to annotate reg_path as reg_path: str


for install_type in reg.HKEY_CURRENT_USER, reg.HKEY_LOCAL_MACHINE:
try:
reg_key = reg.OpenKey(install_type, reg_path, 0, reg.KEY_READ)
edge_path = reg.QueryValue(reg_key, None)
reg_key.Close()
if not os.path.isfile(edge_path):
continue
except WindowsError:
edge_path = "edge_legacy"
else:
break

return edge_path
Copy link
Contributor

Choose a reason for hiding this comment

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

I see two potential issues with this loop:

  1. edge_path here 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.

  2. Your loop runs exactly twice, once for reg.HKEY_CURRENT_USER and once for reg.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 if os.path.isfile(edge_path) is True and no WindowsError (for sub_key not found) is raised.
    You could then set just do

    edge_path = _query_winreg(reg.HKEY_LOCAL_MACHINE, reg_path) or edge_path
    return _query_winreg(reg.HKEY_CURRENT_USER, reg_path) or edge_path
    

    which would make this quite clean and more readable in my opinion (though one can argue about hijacking or to select between truthy and falsy values.