Re: [AD] Relative paths in the grabber

[Eric Botcazou <ebotcazou@xxxxxxxxxx>]

> Ok, due to some unexpected free time and interest in this over at
> Allegro.cc, I've updated the changes I made to the grabber almost a year
> ago to create relative rather than absolute paths to 4.1.9.

Thanks for the patience.

> The attached patch does the following: it adds a `use relative paths'
> option to the options menu that, when checked, causes the grabber to store
> the relative path to items in the datafile. Existing items are not
> affected.

Fine with me.

> There are also two new options in the item options box: one to convert
> paths to relative and one to convert to absolute. These work on mutiple
> selections as well.

Two minor nits: the first letter of each word must be capitalized in the new 
sub-menu. And you didn't provice shortcuts for "relative" and "absolute". 
See for example "Change type".

> I've been using the code in more or less this form for a while and it
> seems to work fine, however, there are a few things that bother me:
>  - the mechanism used to construct relative paths from absolute paths.
> Although it works, I'd like a second opinion on the code.

The algorithm looks ok. I think we can apply some local optimizations though: 
for example, when traversing an Unicode string without altering its content, 
the construct

	while (ugetc(s+pos)) {
	   c = ugetc(s+pos);
	   func(c);
	   pos+= uwidth(s+pos);
	}

can be optimized to

	p = s;
	while ((c = ugetx(&p)))
	   func(c);

>  - because the point (for me) of adding relative paths is easy
> compatibility between DOS/Windows and Linux, absolute paths use the / path
> seperator consistently. This is conterary to the standard Allegro
> practice, where \ is used by default on DOS/Windows.

This will break non-GNU ports on DOS/Windows. We should keep all the code 
separator-neutral, the library providing enough abstraction for this 
purpose. The updating code will probably have to be taught to convert 
relative paths to their platform-dependent form before using I/O functions.

> I've had to introduce a char *fix_filename_forward_slashes(char *filename)
> that converts all \ path seperators to /. I don't think it's particularly
> elegant and I feel it's out of place in the grabber. I'm not sure where
> else to put it, though... (or maybe there is already an Allegro internal
> symbol that does something similar? Are internal functions documented
> anywhere beside the source?)

We don't need it at all.

-- 
Eric Botcazou