-
-
Notifications
You must be signed in to change notification settings - Fork 828
Respect local environment configuration for app file storage #4606
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
Changes from 10 commits
f26bc36
2f71930
7263543
aa7a41f
d28cd44
24127b0
db0438e
62a2aac
ee18b57
86464b7
cffccf1
6d5a646
234af0d
31ea1ad
577da44
599b222
c97ba8c
f0a4e31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Desktop backends now respect user and system environment configuration (`XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_CACHE_HOME`, and `XDG_STATE_HOME` on Linux; `%LOCALAPPDATA%` on Windows) for application storage paths. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,5 @@ def test_subclassed_as_deep_module(): | |
| ) | ||
| def test_cant_reassign(app, path_name): | ||
| """App path attributes are read-only.""" | ||
| # Theoretically, this could leak out of this test... but only if it fails! | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't remove comments that are still accurate, correct, and unrelated to the changes that you're making. |
||
| with pytest.raises(AttributeError): | ||
| setattr(app.paths, path_name, "") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| from contextlib import contextmanager | ||
|
|
||
| from PySide6.QtGui import QPalette | ||
| from PySide6.QtWidgets import QComboBox | ||
| from PySide6.QtWidgets import QComboBox, QSizePolicy | ||
| from travertino.size import at_least | ||
|
|
||
| from .base import Widget | ||
|
|
@@ -11,6 +11,10 @@ class Selection(Widget): | |
| def create(self): | ||
| self.native = QComboBox() | ||
| self.native.setSizeAdjustPolicy(QComboBox.SizeAdjustPolicy.AdjustToContents) | ||
| self.native.setSizePolicy( | ||
| QSizePolicy.Policy.Expanding, | ||
| QSizePolicy.Policy.Fixed, | ||
| ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this have to do with paths?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, this was an unrelated fix for the test_flex_horizontal_widget_size test failure in the Qt CI. I have removed it from this PR. Should I submit that fix as a separate PR? And how should we handle the failing CI on this branch in the meantime?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you believe you've found a fix for something, then by all means submit it as a PR. However, CI is currently passing on main, so your claim that this fixes a CI failure requires evidence, such as a reliable reproduction case, or at least the conditions under which a failure is known to occur. It's possible you may have found a transient error in testing - and if this is a fix for that problem, then great... but if it's a transient problem, "CI now passes" won't be a verification of that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...and to prove the point - I've just re-run the one failing wayland-qt test on this PR, and it passed. So - there's clearly an issue here, but you'll need to convince me this is the fix for the problem - ideally, in a separate PR explaining the source of the problem and why this is a fix.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing me in the right direction. I was able to reproduce the failure in the linux-wayland-qt CI environment. Without the QSizePolicy change, test_flex_horizontal_widget_size fails with an actual width of 73px while the test expects at least 350px. With the Expanding, Fixed size policy, the same CI test passes. I'm doing a further A/B investigation to determine exactly which part of the size policy causes the difference and to make sure the change is not just masking a timing/environment issue. I'll use that to establish the root cause before submitting the separate PR. |
||
| self.native.currentIndexChanged.connect(self.qt_on_current_index_changed) | ||
| self._item_id_count = 0 | ||
| self._last_selected_item_id = 0 | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again - what do these changes have to do with paths? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import os | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
|
|
@@ -35,41 +36,62 @@ def __init__(self, app): | |
| self.app = app | ||
| assert isinstance(self.app._impl.native, TextualApp) | ||
|
|
||
| @property | ||
| def _win32_app_dir(self): | ||
| local_app_data = os.environ.get("LOCALAPPDATA") | ||
| base_dir = ( | ||
| Path(local_app_data) if local_app_data else (Path.home() / "AppData/Local") | ||
| ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, |
||
| return base_dir / AUTHOR / FORMAL_NAME | ||
|
|
||
| @property | ||
| def config_path(self): | ||
| if sys.platform == "darwin": | ||
| return Path.home() / f"Library/Preferences/{APP_ID}" | ||
| elif sys.platform == "win32": | ||
| return Path.home() / f"AppData/Local/{AUTHOR}/{FORMAL_NAME}/Config" | ||
| return self._win32_app_dir / "Config" | ||
| else: | ||
| return Path.home() / f".config/{APP_NAME}" | ||
| return ( | ||
| Path(os.environ.get("XDG_CONFIG_HOME") or (Path.home() / ".config")) | ||
| / APP_NAME | ||
| ) | ||
|
|
||
| @property | ||
| def data_path(self): | ||
| if sys.platform == "darwin": | ||
| return Path.home() / f"Library/Application Support/{APP_ID}" | ||
| elif sys.platform == "win32": | ||
| return Path.home() / f"AppData/Local/{AUTHOR}/{FORMAL_NAME}/Data" | ||
| return self._win32_app_dir / "Data" | ||
| else: | ||
| return Path.home() / f".local/share/{APP_NAME}" | ||
| return ( | ||
| Path(os.environ.get("XDG_DATA_HOME") or (Path.home() / ".local/share")) | ||
| / APP_NAME | ||
| ) | ||
|
|
||
| @property | ||
| def cache_path(self): | ||
| if sys.platform == "darwin": | ||
| return Path.home() / f"Library/Caches/{APP_ID}" | ||
| elif sys.platform == "win32": | ||
| return Path.home() / f"AppData/Local/{AUTHOR}/{FORMAL_NAME}/Cache" | ||
| return self._win32_app_dir / "Cache" | ||
| else: | ||
| return Path.home() / f".cache/{APP_NAME}" | ||
| return ( | ||
| Path(os.environ.get("XDG_CACHE_HOME") or (Path.home() / ".cache")) | ||
| / APP_NAME | ||
| ) | ||
|
|
||
| @property | ||
| def logs_path(self): | ||
| if sys.platform == "darwin": | ||
| return Path.home() / f"Library/Logs/{APP_ID}" | ||
| elif sys.platform == "win32": | ||
| return Path.home() / f"AppData/Local/{AUTHOR}/{FORMAL_NAME}/Logs" | ||
| return self._win32_app_dir / "Logs" | ||
| else: | ||
| return Path.home() / f".local/state/{APP_NAME}/log" | ||
| return ( | ||
| Path(os.environ.get("XDG_STATE_HOME") or (Path.home() / ".local/state")) | ||
| / APP_NAME | ||
| / "log" | ||
| ) | ||
|
|
||
| async def assert_event_loop(self): | ||
| pytest.skip("Event loop assertions are not implemented on Textual.") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import os | ||
| from functools import cached_property | ||
| from pathlib import Path | ||
|
|
||
|
|
@@ -13,7 +14,11 @@ def _app_dir(self): | |
| # No coverage testing of this because we can't easily configure | ||
| # the app to have no author. | ||
| author = "Unknown" if App.app.author is None else App.app.author | ||
| return Path.home() / f"AppData/Local/{author}/{App.app.formal_name}" | ||
| local_app_data = os.environ.get("LOCALAPPDATA") | ||
| base_dir = ( | ||
| Path(local_app_data) if local_app_data else (Path.home() / "AppData/Local") | ||
| ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same again - fallback mode can be used to simplify things. |
||
| return base_dir / author / App.app.formal_name | ||
|
|
||
| # The rest are cached at the interface level: | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.