TEST#6
Open
Radiump123 wants to merge 11 commits into
Open
Conversation
added 9 commits
May 13, 2026 12:49
…re missing, replaced mentions of the old GitHub by the new repo, fixed the issue Hogjects#200. No AI used.
Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch dev Your branch is up to date with 'origin/dev'. Changes to be committed: modified: src/lufus/gui/dialogs.py modified: src/lufus/gui/gui.py modified: src/lufus/gui/languages/Deutsch.csv modified: src/lufus/gui/languages/English.csv modified: "src/lufus/gui/languages/Espa\303\261ol.csv" modified: "src/lufus/gui/languages/Fran\303\247ais.csv" modified: "src/lufus/gui/languages/Portugue\314\202s Brasileiro.csv" modified: src/lufus/gui/languages/Svenska.csv modified: "src/lufus/gui/languages/\320\240\321\203\321\201\321\201\320\272\320\270\320\271.csv" modified: "src/lufus/gui/languages/\321\203\320\272\321\200\320\260\321\227\320\275\321\201\321\214\320\272\320\260.csv" modified: "src/lufus/gui/languages/\330\271\330\261\330\250\331\212.csv" modified: "src/lufus/gui/languages/\340\246\254\340\246\276\340\246\202\340\246\262\340\246\276.csv"
Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch dev Your branch is ahead of 'origin/dev' by 1 commit. (use "git push" to publish your local commits) Changes to be committed: modified: src/lufus/gui/dialogs.py modified: src/lufus/gui/gui.py modified: "src/lufus/gui/languages/Espa\303\261ol.csv" modified: "src/lufus/gui/languages/\320\240\321\203\321\201\321\201\320\272\320\270\320\271.csv" modified: "src/lufus/gui/languages/\321\203\320\272\321\200\320\260\321\227\320\275\321\201\321\214\320\272\320\260.csv" modified: "src/lufus/gui/languages/\330\271\330\261\330\250\331\212.csv" modified: "src/lufus/gui/languages/\340\246\254\340\246\276\340\246\202\340\246\262\340\246\276.csv"
…d some scaling. Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch dev Your branch is ahead of 'origin/dev' by 2 commits. (use "git push" to publish your local commits) Changes to be committed: new file: src/lufus/browse_freely.py modified: src/lufus/gui/dialogs.py modified: src/lufus/gui/gui.py
Please enter the commit message for your changes. Lines starting with '' will be ignored, and an empty message aborts the commit. On branch dev Your branch is ahead of 'origin/dev' by 4 commits. (use "git push" to publish your local commits) Changes to be committed: new file: tests/test_browse_freely.py
Reviewer's GuideRefactors browser opening to use a centralized helper that safely opens URLs as the non-root user, improves language/theme initialization and persistence across elevation, adds a pre-flash confirmation dialog, refines dialogs and localization, and updates project metadata, flashing behavior, and tests for the new behaviors. Sequence diagram for open_url_non_root browser invocationsequenceDiagram
actor User
participant App as GuiComponent
participant Helper as open_url_non_root
participant Runuser as subprocess.Popen
participant Browser as webbrowser
User->>App: trigger URL open
App->>Helper: open_url_non_root(url)
alt [effective UID is root and target_user resolved]
Helper->>Runuser: Popen(["runuser","-u",target_user,"--","xdg-open",url], env)
Runuser-->>Helper: process started
Helper-->>App: return
else [fallback]
Helper->>Browser: open(url)
Browser-->>Helper: browser launched
Helper-->>App: return
end
Sequence diagram for theme and language initialization with elevationsequenceDiagram
actor User
participant Main as launch_gui_with_usb_data
participant Theme as _load_initial_theme
participant Lang as _load_initial_language
participant Elevate as elevate_privileges
User->>Main: launch_gui_with_usb_data()
Main->>Theme: _load_initial_theme()
Main->>Lang: _load_initial_language()
Main->>Elevate: elevate_privileges()
Elevate-->>Main: exits current process
%% New elevated/root process
User->>Main: launch_gui_with_usb_data() (root)
Main->>Theme: _load_initial_theme()
Main->>Lang: _load_initial_language()
Main-->>User: GUI initialized with theme and language
Sequence diagram for pre-flash confirmation dialog in perform_flashsequenceDiagram
actor User
participant MainWin as perform_flash
participant Msg as QMessageBox
User->>MainWin: perform_flash()
MainWin->>Msg: create warning dialog
MainWin->>Msg: exec()
Msg-->>MainWin: clickedButton()
alt [user clicked Cancel]
MainWin->>MainWin: log_message("Flash aborted by user at warning prompt", "WARN")
MainWin->>MainWin: reset buttons and progress bar
MainWin-->>User: flash aborted
else [user clicked OK]
MainWin->>MainWin: continue with flash operation
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
launch_gui_with_usb_data,_load_initial_language()is called both before and afterelevate_privileges(), which means the post-elevation call may read the root user’s config and overwrite the intended user language; consider only initializing language in the non-root context or guarding the second call appropriately. - In
open_url_non_root, when onlySUDO_USERis set you never derive a UID, soXDG_RUNTIME_DIRis left empty; consider resolving the UID forsudo_user(e.g. viapwd.getpwnam) to populate a more accurate environment for GUI apps.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `launch_gui_with_usb_data`, `_load_initial_language()` is called both before and after `elevate_privileges()`, which means the post-elevation call may read the root user’s config and overwrite the intended user language; consider only initializing language in the non-root context or guarding the second call appropriately.
- In `open_url_non_root`, when only `SUDO_USER` is set you never derive a UID, so `XDG_RUNTIME_DIR` is left empty; consider resolving the UID for `sudo_user` (e.g. via `pwd.getpwnam`) to populate a more accurate environment for GUI apps.
## Individual Comments
### Comment 1
<location path="src/lufus/browse_freely.py" line_range="39" />
<code_context>
+
+ # Method 1: Use runuser which is specifically designed for this
+ # We need to preserve the environment for the GUI session
+ env = {
+ "DISPLAY": os.environ.get("DISPLAY", ":0"),
+ "XAUTHORITY": os.environ.get("XAUTHORITY", ""),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Environment for the spawned browser process does not set HOME, which may cause profile or permission issues.
Since many GUI apps depend on `HOME` for profiles and config, relying only on `DISPLAY`, `XAUTHORITY`, `XDG_RUNTIME_DIR`, `WAYLAND_DISPLAY`, and `PATH` risks using root’s home (or none) when running as another user, especially in the `sudo_user` case without `target_uid`. Please also set `HOME` to the target user’s home directory (e.g. `pwd.getpwnam(target_user).pw_dir`) and add it to `env` when available.
Suggested implementation:
```python
import os
import pwd
```
```python
if target_user:
try:
log.info(f"Attempting to open URL as user {target_user} (UID {target_uid}): {url}")
# Determine the target user's home directory so GUI apps use the correct profile/config
home_dir = None
try:
home_dir = pwd.getpwnam(target_user).pw_dir
except KeyError:
log.warning("Could not determine home directory for user %s", target_user)
# Method 1: Use runuser which is specifically designed for this
# We need to preserve the environment for the GUI session
env = {
"DISPLAY": os.environ.get("DISPLAY", ":0"),
"XAUTHORITY": os.environ.get("XAUTHORITY", ""),
"XDG_RUNTIME_DIR": os.environ.get(
"XDG_RUNTIME_DIR", f"/run/user/{target_uid}" if target_uid else ""
),
"WAYLAND_DISPLAY": os.environ.get("WAYLAND_DISPLAY", ""),
"PATH": "/usr/local/bin:/usr/bin:/bin",
# Ensure HOME is set so GUI apps use the target user's profile and config
"HOME": home_dir or os.environ.get("HOME", ""),
}
# Filter out empty values
env = {k: v for k, v in env.items() if v}
```
If `pwd` is already imported elsewhere in this file, adjust the import edit accordingly to avoid duplicate imports (e.g. by adding it to an existing import line instead of introducing a new one).
</issue_to_address>
### Comment 2
<location path="tests/test_browse_freely.py" line_range="11-20" />
<code_context>
+ self.assertEqual(env.get("DISPLAY"), ":0")
+ self.assertEqual(env.get("XDG_RUNTIME_DIR"), "/run/user/1000")
+
+ @patch("os.geteuid")
+ @patch("os.environ.get")
+ @patch("subprocess.Popen")
+ def test_open_url_as_root_via_sudo(self, mock_popen, mock_env_get, mock_geteuid):
+ # Simulate root (UID 0)
+ mock_geteuid.return_value = 0
</code_context>
<issue_to_address>
**suggestion (testing):** Consider using `patch.dict(os.environ, ...)` instead of patching `os.environ.get` to make the tests more realistic and resilient.
Patching `os.environ.get` with a `side_effect` intercepts every env lookup (e.g. `DISPLAY`, `XAUTHORITY`, `XDG_RUNTIME_DIR`), which couples the tests to the exact way `open_url_non_root` reads the environment. Instead, patching `os.environ` via `patch.dict(os.environ, {"PKEXEC_UID": "1000", ...}, clear=False)` (and similarly for the sudo case) more accurately reflects real usage and keeps the tests resilient to internal changes in environment access.
Suggested implementation:
```python
@patch("os.geteuid")
@patch("subprocess.Popen")
@patch("pwd.getpwuid")
def test_open_url_as_root_via_pkexec(self, mock_getpwuid, mock_popen, mock_geteuid):
```
```python
# Mock environment variables
env_overrides = {
"PKEXEC_UID": "1000",
"DISPLAY": ":0",
"XDG_RUNTIME_DIR": "/run/user/1000",
}
with patch.dict(os.environ, env_overrides, clear=False):
open_url_non_root(self.url)
# Mock pwd info
```
1. Apply a similar refactor to the `test_open_url_as_root_via_sudo` test: remove any `@patch("os.environ.get")` decorator and associated `mock_env_get` parameter, and replace any `side_effect`-based environment mocking with a `with patch.dict(os.environ, {...}, clear=False):` block that sets the sudo-related variables your production code expects (e.g. `SUDO_USER`, `DISPLAY`, `XDG_RUNTIME_DIR`, etc.), wrapping the `open_url_non_root(self.url)` call in that context.
2. If there are other tests in this file that also patch `os.environ.get`, consider converting them to `patch.dict` in the same way to keep the test suite consistent and resilient to internal environment access changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+11
to
+20
| @patch("os.geteuid") | ||
| @patch("webbrowser.open") | ||
| def test_open_url_non_root_as_regular_user(self, mock_webbrowser, mock_geteuid): | ||
| # Simulate regular user (UID 1000) | ||
| mock_geteuid.return_value = 1000 | ||
|
|
||
| open_url_non_root(self.url) | ||
|
|
||
| # Should fallback to standard webbrowser.open | ||
| mock_webbrowser.assert_called_once_with(self.url) |
There was a problem hiding this comment.
suggestion (testing): Consider using patch.dict(os.environ, ...) instead of patching os.environ.get to make the tests more realistic and resilient.
Patching os.environ.get with a side_effect intercepts every env lookup (e.g. DISPLAY, XAUTHORITY, XDG_RUNTIME_DIR), which couples the tests to the exact way open_url_non_root reads the environment. Instead, patching os.environ via patch.dict(os.environ, {"PKEXEC_UID": "1000", ...}, clear=False) (and similarly for the sudo case) more accurately reflects real usage and keeps the tests resilient to internal changes in environment access.
Suggested implementation:
@patch("os.geteuid")
@patch("subprocess.Popen")
@patch("pwd.getpwuid")
def test_open_url_as_root_via_pkexec(self, mock_getpwuid, mock_popen, mock_geteuid): # Mock environment variables
env_overrides = {
"PKEXEC_UID": "1000",
"DISPLAY": ":0",
"XDG_RUNTIME_DIR": "/run/user/1000",
}
with patch.dict(os.environ, env_overrides, clear=False):
open_url_non_root(self.url)
# Mock pwd info- Apply a similar refactor to the
test_open_url_as_root_via_sudotest: remove any@patch("os.environ.get")decorator and associatedmock_env_getparameter, and replace anyside_effect-based environment mocking with awith patch.dict(os.environ, {...}, clear=False):block that sets the sudo-related variables your production code expects (e.g.SUDO_USER,DISPLAY,XDG_RUNTIME_DIR, etc.), wrapping theopen_url_non_root(self.url)call in that context. - If there are other tests in this file that also patch
os.environ.get, consider converting them topatch.dictin the same way to keep the test suite consistent and resilient to internal environment access changes.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/lufus/gui/i18n.py" line_range="29-38" />
<code_context>
+ Falls back to English if detection fails or no match is found.
+ """
+ try:
+ loc = locale.getdefaultlocale()
+ except Exception:
+ loc = (None, None)
</code_context>
<issue_to_address>
**suggestion:** Using `locale.getdefaultlocale()` may be problematic going forward due to deprecation; consider an alternative approach.
`locale.getdefaultlocale()` is deprecated in recent Python versions and may be removed. Since only the language is needed, consider alternatives such as `locale.getlocale()[0]`, `os.environ.get("LANG")`, or `locale.setlocale(locale.LC_CTYPE, "")` followed by `locale.getlocale()` to avoid relying on a deprecated API.
```suggestion
def detect_system_language() -> str:
"""Detect the system locale and return the matching language name.
Falls back to English if detection fails or no match is found.
"""
try:
# Ensure locale is initialized from the environment, then read current locale.
locale.setlocale(locale.LC_CTYPE, "")
loc = locale.getlocale()
except Exception:
loc = (None, None)
lang_code = (loc[0] or "") or os.environ.get("LANG", "")
```
</issue_to_address>
### Comment 2
<location path="tests/test_flash_usb_and_find_usb_fixes.py" line_range="148-149" />
<code_context>
pass
class FakePipe:
- def readline(self):
+ def read(self, n=-1):
return b""
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the fake stderr pipe to assert that `read` is called with the expected chunk size (4096).
`FakePipe.read` currently ignores `n` and always returns `b""`, so the test doesn’t verify that `process.stderr.read(4096)` is actually called with the intended chunk size. Consider recording the passed-in `n` (e.g., `self.last_n = n`) and asserting it is `4096` (or at least `> 0`) after the call to catch regressions like switching back to `readline()` or changing the chunk size.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
148
to
+149
| class FakePipe: | ||
| def readline(self): | ||
| def read(self, n=-1): |
There was a problem hiding this comment.
suggestion (testing): Strengthen the fake stderr pipe to assert that read is called with the expected chunk size (4096).
FakePipe.read currently ignores n and always returns b"", so the test doesn’t verify that process.stderr.read(4096) is actually called with the intended chunk size. Consider recording the passed-in n (e.g., self.last_n = n) and asserting it is 4096 (or at least > 0) after the call to catch regressions like switching back to readline() or changing the chunk size.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/lufus/gui/i18n.py" line_range="34-41" />
<code_context>
+ Falls back to English if detection fails or no match is found.
+ """
+ try:
+ # Ensure locale is initialized from the environment, then read current locale.
+ locale.setlocale(locale.LC_CTYPE, "")
+ loc = locale.getlocale()
+ except Exception:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Calling `locale.setlocale` here mutates global process state and may have unintended side effects.
`locale.setlocale(locale.LC_CTYPE, "")` here mutates the global locale and can unexpectedly impact formatting/parsing and libraries that depend on locale (e.g. Qt). If you only need to detect the system locale, prefer `locale.getdefaultlocale()` or environment variables (e.g. `$LANG`) without changing the active locale, or temporarily change it and immediately restore the original value.
```suggestion
try:
# Read the system locale without mutating global locale state.
loc = locale.getdefaultlocale()
except Exception:
loc = (None, None)
# Fallback to the process locale if the default locale could not be determined.
if not loc or not loc[0]:
try:
loc = locale.getlocale()
except Exception:
loc = (None, None)
lang_code = (loc[0] or "") or os.environ.get("LANG", "")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+34
to
+41
| try: | ||
| # Ensure locale is initialized from the environment, then read current locale. | ||
| locale.setlocale(locale.LC_CTYPE, "") | ||
| loc = locale.getlocale() | ||
| except Exception: | ||
| loc = (None, None) | ||
|
|
||
| lang_code = (loc[0] or "") or os.environ.get("LANG", "") |
There was a problem hiding this comment.
suggestion (bug_risk): Calling locale.setlocale here mutates global process state and may have unintended side effects.
locale.setlocale(locale.LC_CTYPE, "") here mutates the global locale and can unexpectedly impact formatting/parsing and libraries that depend on locale (e.g. Qt). If you only need to detect the system locale, prefer locale.getdefaultlocale() or environment variables (e.g. $LANG) without changing the active locale, or temporarily change it and immediately restore the original value.
Suggested change
| try: | |
| # Ensure locale is initialized from the environment, then read current locale. | |
| locale.setlocale(locale.LC_CTYPE, "") | |
| loc = locale.getlocale() | |
| except Exception: | |
| loc = (None, None) | |
| lang_code = (loc[0] or "") or os.environ.get("LANG", "") | |
| try: | |
| # Read the system locale without mutating global locale state. | |
| loc = locale.getdefaultlocale() | |
| except Exception: | |
| loc = (None, None) | |
| # Fallback to the process locale if the default locale could not be determined. | |
| if not loc or not loc[0]: | |
| try: | |
| loc = locale.getlocale() | |
| except Exception: | |
| loc = (None, None) | |
| lang_code = (loc[0] or "") or os.environ.get("LANG", "") |
Owner
Author
|
broke smth i think |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Update GUI behavior, language handling, and URL opening to improve usability, persistence, and non-root browser launching, while rebranding GitHub references to the Hogjects organization.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: