Skip to content
This repository was archived by the owner on Jun 22, 2025. It is now read-only.

Edge app #663

Draft
doug-benn wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Edge app #663

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0ea5a25
.vscode to gitignore
doug-benn Feb 13, 2023
0c243dd
Edge App Mode
doug-benn Feb 13, 2023
463f81d
Edge pyinstaller "stdout=sps.PIPE, stderr=sps.PIPE, stdin=sps.PIPE," …
doug-benn Feb 13, 2023
6085773
.vscode to gitignore
doug-benn Feb 13, 2023
4c86c90
Edge App Mode
doug-benn Feb 13, 2023
d69ae82
Merge branch 'Edge-app' of https://github.com/doug-benn/Eel into Edge…
doug-benn Feb 14, 2023
1fb06d7
New Types added
doug-benn Feb 14, 2023
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
Prev Previous commit
Next Next commit
Edge pyinstaller "stdout=sps.PIPE, stderr=sps.PIPE, stdin=sps.PIPE," … 
…issue fix
  • Loading branch information
@doug-benn
doug-benn committed Feb 13, 2023
commit 463f81d90c4f70b8b79377e54ce58b7aaba8fe05
35 changes: 21 additions & 14 deletions eel/edge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,38 @@
import subprocess as sps
import sys

name = 'Edge'
name = "Edge"


def run(path, options, start_urls):
if path != 'edge_legacy':
if options['app_mode']:
if path != "edge_legacy":
Copy link
Contributor

@thatfloflo thatfloflo Mar 31, 2023

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)
sps.Popen([path, "--app=%s" % url] + options["cmdline_args"], stdout=sps.PIPE, stderr=sps.PIPE, stdin=sps.PIPE)
Copy link
Contributor

@thatfloflo thatfloflo Mar 31, 2023

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=sys.stderr, stdin=sps.PIPE)
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=sys.stdout, stderr=sys.stderr, stdin=sps.PIPE, shell=True)
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():
if sys.platform in ['win32', 'win64']:
if sys.platform in ["win32", "win64"]:
return _find_edge_win()
else:
return None


def _find_edge_win():
Copy link
Contributor

@thatfloflo thatfloflo Mar 31, 2023

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'

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

@thatfloflo thatfloflo Mar 31, 2023

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:
Expand All @@ -36,8 +43,8 @@ def _find_edge_win():
if not os.path.isfile(edge_path):
continue
except WindowsError:
edge_path = 'edge_legacy'
edge_path = "edge_legacy"
else:
break

return edge_path
return edge_path
Copy link
Contributor

@thatfloflo thatfloflo Mar 31, 2023

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.