fix: handle a missing pardus-xfce-settings without crashing or discarding user config - #1
Open
ehlinazgumus wants to merge 2 commits into
Open
ehlinazgumus wants to merge 2 commits into
ehlinazgumus wants to merge 2 commits into
Conversation
…ts exist
restore_default_settings() removed the user's panel configuration and then
copied the packaged defaults over it:
rm -rf ~/.config/xfce4/panel/* ~/.config/.../xfce4-panel.xml;
cp -R /etc/xdg/pardus/xfce4/panel/* ~/.config/xfce4/panel/;
Two problems. The steps are joined with ';', so each one runs whether or not
the previous succeeded, and subprocess.run() was called without check=True, so
a failure was never reported. And the source it copies from belongs to
pardus-xfce-settings, which is not a dependency of this package or of
pardus-xfce-tweaks:
pardus-xfce-tweaks Depends: python3-gi, python3:any, python3 (>= 3.5),
gir1.2-gtk-3.0, gir1.2-xfconf-0,
pardus-lib-xfce (>= 0.3)
pardus-lib-xfce Depends: python3 (>= 3.5), python3-gi,
gir1.2-glib-2.0, gir1.2-xfconf-0
On a system where that package is absent, /etc/xdg/pardus does not exist at
all, so pressing "Restore Defaults" deleted the panel configuration, both cp
calls failed silently, and the panel came back with nothing to load. The user
lost their layout with no way to get it back and no error shown.
Check for the defaults first and raise FileNotFoundError before touching
anything the user owns, keep a .bak copy of the configuration since the
operation is otherwise irreversible, and chain the shell steps with && plus
check=True so a failure stops the sequence and reaches the caller.
Callers should catch FileNotFoundError and tell the user the defaults are
missing rather than failing silently.
Declaring a dependency on pardus-xfce-settings would be worth doing as well;
this change makes the missing-file case safe either way.
DatetimeManager copies /etc/xdg/pardus/xfce4/panel/datetime-8.rc into the user's config at module import time, but only checks whether the destination exists - never the source. That file comes from pardus-xfce-settings, which is not a declared dependency of this library, so on a system where it is not installed shutil.copyfile raises FileNotFoundError. Because the copy runs in the module body, the error propagates through pardus-xfce-tweaks' import of this module and the application dies before its window is created. Launched from the menu it fails silently, with nothing shown to the user. Guard the copy on the source existing and treat a missing config as an empty one, so a missing optional package degrades the datetime plugin settings instead of taking down the whole UI. restore_default_settings() had the same unguarded pattern in a worse form: it removed the user's file first and only then copied the default over it, so a missing source left the user with nothing. Check first, and drop the remove - copyfile already overwrites. It also left config_file_data holding the pre-restore contents, which the next save_file() would write straight back out; refresh it.
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
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.
Two places in this library read files from
/etc/xdg/pardus/xfce4/...without checking that they are there. Those files come frompardus-xfce-settings, which is not a declared dependency ofpardus-lib-xfceor ofpardus-xfce-tweaks:So the package is only pulled in by the two desktop meta-packages. Installing XFCE another way —
apt install xfce4— or installingpardus-xfce-tweakson its own leaves it out.1.
DatetimeManagercrashes at import when the defaults are missingThe copy runs in the module body and only checks the destination:
Because it is at module level, the error escapes through
pardus-xfce-tweaks'import and the application dies before its window exists. On Pardus 25.1 with
xfce4+pardus-xfce-tweaks0.5.0 installed andpardus-xfce-settingsabsent:Launched from the applications menu there is no traceback and no window — nothing happens at all.
This PR guards the copy on the source existing and treats a missing config as an empty one, so the datetime plugin settings degrade instead of the whole UI failing to open.
DatetimeManager.restore_default_settings()had the same pattern in a worse form —os.remove()first,copyfile()second — which left the user with no config at all when the source was missing. It now checks first, and drops theremovesincecopyfileoverwrites anyway. It also leftconfig_file_dataholding the pre-restore contents, which the nextsave_file()would write straight back out; that is refreshed now.2.
PanelManager.restore_default_settings()deletes before it checksThe steps are chained with
;, so a failingcpdoes not stop anything, and there is nocheck=True— the removal has already happened by then either way. This PR verifies the defaults are present before touching anything the user owns, keeps a.bakcopy since the operation cannot be undone, chains the shell steps with&&, and addscheck=True.In practice issue 1 currently masks this one: without
pardus-xfce-settingsthe application never opens, so the button cannot be reached. The guard is still worth having — the removal is irreversible, and a partially installed or damaged defaults directory would hit the same path.Testing
DatetimeManagerwas exercised directly against a temporaryHOME:FileNotFoundErrorat importconfig_file_data == ""restore_default_settings(), defaults missingset()thenrestore_default_settings()PanelManagerwas verified at the patch level: with the defaults absent the old code removed the user's configuration, the new code raisesFileNotFoundErrorand leaves it in place.Neither change has been run end to end against an installed package on a full XFCE session.
A note on packaging
The code fixes stop the failures, but the underlying issue is the missing declaration.
pardus-lib-xfcereads files at runtime that onlypardus-xfce-settingsprovides, sodebian/controlarguably wants aDepends:(or at leastRecommends:) on it. I have left that out of this PR since it is a packaging decision for you to make. The same gap exists forpardus-xfce-icon-theme, referenced frompardus-xfce-tweaks'MainWindow.py:714and:727.