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

Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,3 +9,4 @@ Eel.egg-info
*.swp
venv/
.tox
.vscode
44 changes: 36 additions & 8 deletions eel/edge.py
View file
Open in desktop
Original file line numberDiff line numberDiff 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"

@thatfloflothatflofloMar 31, 2023

Copy link
Copy Markdown
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":

@thatfloflothatflofloMar 31, 2023

Copy link
Copy Markdown
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)

@thatfloflothatflofloMar 31, 2023

Copy link
Copy Markdown
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:

@thatfloflothatflofloMar 31, 2023

Copy link
Copy Markdown
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():

@thatfloflothatflofloMar 31, 2023

Copy link
Copy Markdown
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"

@thatfloflothatflofloMar 31, 2023

Copy link
Copy Markdown
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

@thatfloflothatflofloMar 31, 2023

Copy link
Copy Markdown
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.