Good evening everyone,
This took a little longer than expected because something felt a little
off in what was happening (see the P.S. below). The patch I'm attaching
implements the "clean" fix I proposed in this thread. It relocates
assets under <themedir>/assetname.
The changes are in two functions:
1. In findCopyFile, it does wcopy_file(dir, fullPath, file) instead of
wcopy_file(dir, fullPath, fullPath) (where dir is really <themedir>,
fullPath is <full path to file>/file, and file is the file's name,
without the rest of the path). This causes file to be copied at
<themedir>/file, not <themedir>/fullPath.
2. On the *other* branches in makeThemePack (i.e. when
strrchr(WMGetFromPLString(file), '/') doesn't return null, which causes
findCopyFile to be called because the full path to the file is not
known), makeThemePack now gets the "new" path before copying the file,
and then calls wcopy_file with the new path as a destination.
I have tested this patch on my system as well as I could, but I would
appreciate any additional testing. Needless to say, any comments are
appreciated, too.
While testing, I did stumble upon something that appears to be an
unrelated bug. I'm not reporting it separately because I'd appreciate
another opinion on it (maybe my patch introduces it in some manner that
escapes me).
If I save a theme, apply it (causing the assets to be loaded from the
.themed directory, rather than from wherever they're on my system), then
gestyle -p again, it doesn't work. wcopy_file borks like this:
WINGs(wcopy_file(findfile.c:441)): error: Could not open input file
"~/GNUstep/Library/WindowMaker/Themes/test.themed/BlueImage.jpeg": No
such file or directory
The file *does* exist, so the obvious culprit is that ~ doesn't get
expanded to /home/alazar, but I don't understand is why
WMGetFromPLString(file) would return a path that begins with "~" in the
first place. I suspect this problem did not occur back when copying was
done via system() because the shell was expanding the "~", but that's no
longer the case. Besides, the "dirty" fix (mkdir-ing the whole hierarchy
before wcopy_file) would not have avoided this, either.
Can anyone find something wrong with this reasoning?
If not. I'll provide a separate patch for this problem.
Best regards,
Alexandru Lazar
Carlos R. Mafra wrote:
On Tue, 19 Apr 2016 at 21:26:45 +0300, Alexandru Lazar wrote:
getstyle should either fix this automatically (by renaming one of the
files after the relocation) or complain about it.
I think it should not "fix" this automatically. Suppose
you rename it. Are you going to rename it to what? We should
avoid surprises.
There's a finite number of assets that can define a theme, so
renaming file.png to file_1.png (and subsequently file_2.png, up to
file_<nresources>.png) would be harmless.
I may not have been completely clear here -- I'm not thinking of
renaming the original files (which would be an unpleasant surprise)
just of copying them under a different name under
<themedir>/assets/. E.g. /home/user/tiles/blue.png would be copied
as <themedir>/assets/blue.png; the next file called blue.png that
getstyle encounters would be copied as <themedir>/assets/blue_1.png.
We could complain about it have the user fix this, but I bet 90% of
the users would go ahead and rename file.png to file_1.png, too.
I'm not married to this idea though (especially since it requires
more code and, therefore, more bugs). The case is unlikely enough
that we could just keep this thing in mind as a possible
improvement, and do it later if someone asks.
Since this scenario is very unlikely I'd say we must
keep the code as simple as possible. A warning is
enough.
>From e1a3717732b9d1c0dfb637bb209cbaf5cd1a9ff8 Mon Sep 17 00:00:00 2001
From: Alexandru Lazar <ala...@startmail.com>
Date: Wed, 20 Apr 2016 21:21:33 +0300
Subject: [PATCH] getstyle: fix wcopy_file paths
Some (presumably stale) calls to wcopy_file used what appears to
be an incorrect destination which did not always exist. This patch
forces assets to be copied under <themedir>/<asset>, rather than
<themedir><absolute path of asset>. It works by first getting the
"new" path (i.e. the one that will be inserted in the property
list), which is relative to <themedir> (and appears to be always
in the root directory, too); it then copies the file to that path.
This *may* have been the original intended behaviour, as the one
it replaces clutters the path and leaks configuration data. In
addition, the style file seems to store only the file's name, not
the path relative to <themedir>, even when the file is copied with
its full hierarchy.
Signed-off-by: Alexandru Lazar <ala...@startmail.com>
---
util/getstyle.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/util/getstyle.c b/util/getstyle.c
index f0de4c4..c2c61c3 100644
--- a/util/getstyle.c
+++ b/util/getstyle.c
@@ -171,7 +171,7 @@ static void findCopyFile(const char *dir, const char *file)
(void)wrmdirhier(ThemePath);
return;
}
- wcopy_file(dir, fullPath, fullPath);
+ wcopy_file(dir, fullPath, file);
wfree(fullPath);
}
@@ -234,9 +234,10 @@ static void makeThemePack(WMPropList * style, const char *themeName)
p = strrchr(WMGetFromPLString(file), '/');
if (p) {
- wcopy_file(themeDir, WMGetFromPLString(file), WMGetFromPLString(file));
-
newPath = wstrdup(p + 1);
+
+ wcopy_file(themeDir, WMGetFromPLString(file), newPath);
+
WMDeleteFromPLArray(value, 1);
WMInsertInPLArray(value, 1, WMCreatePLString(newPath));
free(newPath);
@@ -253,9 +254,10 @@ static void makeThemePack(WMPropList * style, const char *themeName)
p = strrchr(WMGetFromPLString(file), '/');
if (p) {
- wcopy_file(themeDir, WMGetFromPLString(file), WMGetFromPLString(file));
-
newPath = wstrdup(p + 1);
+
+ wcopy_file(themeDir, WMGetFromPLString(file), newPath);
+
WMDeleteFromPLArray(value, 1);
WMInsertInPLArray(value, 1, WMCreatePLString(newPath));
free(newPath);
@@ -267,9 +269,10 @@ static void makeThemePack(WMPropList * style, const char *themeName)
p = strrchr(WMGetFromPLString(file), '/');
if (p) {
- wcopy_file(themeDir, WMGetFromPLString(file), WMGetFromPLString(file));
-
newPath = wstrdup(p + 1);
+
+ wcopy_file(themeDir, WMGetFromPLString(file), newPath);
+
WMDeleteFromPLArray(value, 2);
WMInsertInPLArray(value, 2, WMCreatePLString(newPath));
free(newPath);
--
2.8.0
