Opened 10 years ago

Last modified 5 years ago

#969 new task

move color structs to colors.h?

Reported by: hamish Owned by: grass-dev@…
Priority: blocker Milestone: 8.0.0
Component: Display Version: svn-trunk
Keywords: needinfo, RGBA_Color, G_str_to_color() Cc:
CPU: All Platform: All

Description

Hi,

in trunk the RGBA_Color struct has been moved into include/raster.h. As its use is more general than just raster maps (e.g. d.graph and D_symbol() use it) IMO there's no reason for it to be there and it should be moved into include/colors.h or back into gis.h.

Also, any objections to changing G_str_to_color() in trunk to use unsigned char instead of int for R,G,B? It would save some casting.

Hamish

Change History (10)

comment:1 Changed 7 years ago by martinl

include/colors.h sounds as reasonable please for RGBA_Color structure. Please go ahead.

comment:2 Changed 5 years ago by neteler

Would this count as an API change? Then make it a blocker or change to GRASS GIS 8 target.

comment:3 in reply to:  2 ; Changed 5 years ago by wenzeslaus

Priority: normalblocker

Replying to neteler:

Would this count as an API change? Then make it a blocker or change to GRASS GIS 8 target.

It is an API change.

Should we also change G_str_to_color()? This would be a blocker too.

comment:4 in reply to:  3 Changed 5 years ago by martinl

Replying to wenzeslaus:

Replying to neteler:

Would this count as an API change? Then make it a blocker or change to GRASS GIS 8 target.

It is an API change.

Should we also change G_str_to_color()? This would be a blocker too.

does anyone planning to work on this issue?

comment:5 in reply to:  3 ; Changed 5 years ago by mmetz

Replying to wenzeslaus:

Replying to neteler:

Would this count as an API change? Then make it a blocker or change to GRASS GIS 8 target.

It is an API change.

RGBA_Color moved to include/display.h in r62002,3 (it is only used in display functions).

Should we also change G_str_to_color()? This would be a blocker too.

IMHO no, because there is no bug reported for G_str_to_color(), using unsigned char instead of int means that a range check can no longer be done, and changing G_str_to_color() implies changing

display/d.barscale/draw_scale.c
display/d.extract/main.c
display/d.graph/do_graph.c
display/d.graph/main.c
display/d.grid/fiducial.c
display/d.labels/color.c
display/d.northarrow/draw_n_arrow.c
display/d.path/main.c
display/d.rast/display.c
display/d.thematic.area/main.c
display/d.thematic.area/plot1.c
display/d.vect.chart/main.c
display/d.vect/opt.c
display/d.vect/shape.c
general/g.cairocomp/main.c
general/g.pnmcomp/main.c
lib/cairodriver/Graph.c
lib/display/tran_colr.c
lib/imagery/iclass_signatures.c
lib/imagery/iclass_statistics.c
lib/nviz/nviz.c
lib/ogsf/Gp3.c
lib/ogsf/Gv3.c
lib/pngdriver/Graph_set.c
lib/raster/color_hist.c
misc/m.nviz.image/main.c
ps/ps.map/do_labels.c
ps/ps.map/getgrid.c
ps/ps.map/ps_outline.c
ps/ps.map/ps_vareas.c
ps/ps.map/ps_vlines.c
ps/ps.map/ps_vpoints.c
ps/ps.map/r_border.c
ps/ps.map/r_colortable.c
ps/ps.map/r_header.c
ps/ps.map/r_info.c
ps/ps.map/r_instructions.c
ps/ps.map/r_plt.c
ps/ps.map/r_text.c
ps/ps.map/r_vareas.c
ps/ps.map/r_vlegend.c
ps/ps.map/r_vlines.c
ps/ps.map/r_vpoints.c
ps/ps.map/r_wind.c
raster/r.out.png/main.c
raster/r.out.tiff/main.c
vector/v.colors/read_rgb.c
vector/v.to.rast/support.c

Thus the benefit of changing a harmless type cast comes at a cost.

comment:6 Changed 5 years ago by neteler

Is anything missing here?

comment:7 Changed 5 years ago by neteler

Keywords: needinfo added

comment:8 Changed 5 years ago by martinl

what is status of this ticket?

comment:9 in reply to:  5 ; Changed 5 years ago by martinl

Replying to mmetz:

Should we also change G_str_to_color()? This would be a blocker too.

IMHO no, because there is no bug reported for G_str_to_color(), using unsigned char instead of int means that a range check can no longer be done, and changing G_str_to_color() implies changing

it sounds like something for G8 and not G7 (we are in beta stage)...

comment:10 in reply to:  9 Changed 5 years ago by martinl

Milestone: 7.0.08.0.0

Replying to martinl:

Replying to mmetz:

Should we also change G_str_to_color()? This would be a blocker too.

IMHO no, because there is no bug reported for G_str_to_color(), using unsigned char instead of int means that a range check can no longer be done, and changing G_str_to_color() implies changing

it sounds like something for G8 and not G7 (we are in beta stage)...

I took liberty to change milestone of this ticket...

Note: See TracTickets for help on using tickets.