Re: Image support for Carbon Emacs (Re: Consolidation of image support)

[storm@cua.dk (Kim F. Storm)]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> >>>>> On Thu, 18 Dec 2003 22:57:00 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:
> 
> My assignment paper seems to be accepted.  Because I've never made
> larger patches to emacs, I'd like to post the patch for comments, both
> on style and code. (That was suggested by Kim F. Storm.  Thanks.)

I have looked briefly on the code, and overall it looks really good.

There are some things in the ChangeLogs which can be improved; see below.  

I cannot test your patch, but I suggest that you commit it, and see how
it is received (provided that you are prepared to handle the complaints
and bug reports :-)

When you commit your changes to CVS, pls. commit one file at a time, and
use the corresponding changelog entry for that file as cvs comment.

You should write a small entry for etc/NEWS.


Here are my comments on your change logs.  Note that the is is not a
complete list; there are other places that need similar changes.


	* dispextern.h: Change HAVE_CARBON to MAC_OS to cope with Mac OS
	8/9 version.

This applies to all of dispextern.h, so the following line is not needed:

	(struct glyph_string): Likewise.


	emacs.c (main): Likewise.
	frame.c (x_set_frame_parameters, x_report_frame_params)
	(x_set_fullscreen, x_set_border_width, Vdefault_frame_alist):
	Likewise.
	xdisp.c (define_frame_cursor1, expose_window, expose_frame):
	Likewise.

The above entries should be moved to the relevant file sections below.
For these, "Likewise" should be "Change HAVE_CARBON to MAC_OS".


	* macgui.h [MAC_OSX] (Carbon/Carbon.h): Add #include.
Change this to:
        * macgui.h [MAC_OSX]: Include Carbon/Carbon.h.



	[MAC_OSX] (mktime, DEBUG, Z, free, malloc, realloc, max, min)
	(init_process): Add #undef and #define enclosing the above #include

Move [MAX_OSX] to before the : and maybe simplify text to

	(mktime, DEBUG, Z, free, malloc, realloc, max, min)
	(init_process) [MAC_OSX] : Avoid conflicts with Carbon/Carbon.h.



	dispextern.h [MAC_OSX] (Carbon/Carbon.h): Remove #include and
	enclosing #undef and #define (now in macgui.h).
	macfns.c [MAC_OSX] (Carbon/Carbon.h): Likewise.
	macmenu.c [MAC_OSX] (Carbon/Carbon.h): Likewise.
	macterm.c [MAC_OSX] (Carbon/Carbon.h): Likewise.
	macterm.h [MAC_OSX] (Carbon/Carbon.h): Likewise.

Again, move these lines to the relevant file sections.
Also, simplify text to:
        [MAC_OSX] Do not include Carbon/Carbon.h (now in macgui.h).

	
	* macgui.h [!MAC_OSX] (QDOffscreen.h, Controls.h): Add #include.
Write: 
        * macgui.h [!MAC_OSX]: Include QDOffscreen.h and Controls.h.



	[MAC_OSX] (INFINITY): Add #undef to avoid the definition in math.h
Write:
	(INFINITY) [MAC_OSX]: Avoid conflict with definition in math.h.


	* macterm.h (RED16_FROM_ULONG, GREEN16_FROM_ULONG,
	BLUE16_FROM_ULONG): New #define to extract 16-bit depth color
Write:
	* macterm.h (RED16_FROM_ULONG, GREEN16_FROM_ULONG)
	(BLUE16_FROM_ULONG): New #define to extract 16-bit depth color



In the long list of changes for macfns.c and macterm.c, you don't need
to strictly follow the sequence of the code, so you can should collapse
similar entries like these [not the complete list]:
	
	(x_set_cursor_type): Sync with xfns.c.
	(x_make_gc): Sync with xfns.c but enclose unused `border_tile'
	with #if 0.
	(Fxw_color_values): Sync with xfns.c.
	(valid_image_p, image_value_type, parse_image_spec): Sync with
	xfns.c.
	(image_ascent): Sync with xfns.c.
	(x_clear_image, x_alloc_image_color): Sync with xfns.c.

into just one group:

	(x_set_cursor_type, x_make_gc, Fxw_color_values, valid_image_p)
        (image_value_type, parse_image_spec, image_ascent)
	(x_clear_image, x_alloc_image_color): Sync with xfns.c.

Likewise for "Add declaration", "New function (from xfns.c)", etc.


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk