[GRASS-dev] reading items from element list

[Glynn Clements]

Michael Barton wrote:

> Testing our r.prominence, I found that it will not locate any raster  
> files for input in the GUI. Looking at the code, it calls fcell.  
> According to element_list, this is a valid element, but it is not  
> clear how it is called, since it is called a "support_element". The  
> program requires a floating point raster, so specifying it in the GUI  
> is a good idea.
> 
> cell:rast:raster:raster files
>    cellhd:header
>    cats:category
>    colr:color
>    hist:history
>    cell_misc:misc
>    fcell:fcell
>    g3dcell:g3dcell
> 
> 
> So, does the GUI parser recognize support_element items? If so, how  
> are they to be called in the the section that specifies the GUI for a  
> module?

Checking for the existence of a raster is done by looking for the
.../cell/<mapname> file. Even FP maps have this file, although it has
zero length.

r.prominence appears to have been written without any reference to the
documentation or existing code. Two points stand out:

1.
	parm.input->gisprompt = "old,fcell";
	parm.output->gisprompt = "fcell";

These should be "old,cell,raster" and "new,cell,raster" respectively.
There is no way to instruct the parser to check for an FP map; you
have to do this yourself.

2. 
 	    if ((flag.localnorm->answer) || (flag.globalnorm->answer)) {
 	        G_remove("cats", parm.output->answer);
 	        G_remove("cell", parm.output->answer);
 	        G_remove("cell_hd", parm.output->answer);
 	        G_remove("cell_misc", parm.output->answer);
 	        G_remove("colr", parm.output->answer);
 	        G_remove("colr2", parm.output->answer);
 	        G_remove("fcell", parm.output->answer);
 	        G_remove("hist", parm.output->answer);
 	        G_rename("cats", sysstr, parm.output->answer);
 	        G_rename("cell", sysstr, parm.output->answer);
 	        G_rename("cell_hd", sysstr, parm.output->answer);
 	        G_rename("cell_misc", sysstr, parm.output->answer);
 	        G_rename("colr", sysstr, parm.output->answer);
 	        G_rename("colr2", sysstr, parm.output->answer);
 	        G_rename("fcell", sysstr, parm.output->answer);
 	        G_rename("hist", sysstr, parm.output->answer);

This is bogus. If there is a valid reason to remove/rename maps, it
should spawn g.remove/g.rename.

Some other issues:

 	    mapset = G_calloc(512, sizeof(char));
 	    mapset = G_find_cell(parm.input->answer, "");

The first line is pointless, and just wastes memory. The second line
should probably be G_find_cell2().

 	        G_fatal_error
 	            ("Input map is not a floating point map.\nOnly integer maps are allowed as input maps.");

Not localised, too verbose, and self-contraditory.

 	    if (!G_legal_filename(parm.output->answer)) {
 	        G_fatal_error("%s is not a legal filename for an output map.\n",
 	                      parm.output->answer);

Invalid use of G_legal_filename(), which is supposed to operate upon
individual components, not complete map names (map at mapset is not a
legal filename but is valid here, so long as the mapset is the current
mapset).

 	        if (!flag.quiet->answer) {
 	            G_percent(y, nrows - 1, 1);
 	            fflush(stdout);
 	        }

The fflush(stdout) is a bit pointless, given that G_percent() writes
to stderr (which is unbuffered). If a fflush() was needed, it would be
done by G_percent().

 	    free(mapset);

The return value from G_find_* may be shared and shouldn't be freed.

-- 
Glynn Clements <glynn at gclements.plus.com>