Re: getstyle cannot create target file

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 <[email protected]>
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 <[email protected]>
---
 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