Re: OpenVMS GTK diffs

[Owen Taylor <otaylor redhat com>]

OK, here are some comments about the GTK+ patches. They are mostly
stylistic in nature; with two major themes:

 OS-level compatibility should be handled in GLib whenever possible.
 There should be as few #ifdef as possible.
 
Colin Blake <colin@theblakes.com> writes:

> These are all the changes I had to make to the GTK code, in order to get
> it build/run on OpenVMS. I can explain the reason for each change if
> needed.

First, you are rather behind here in the version. If you are working on a port,
you really need to be working from the development version in CVS
(see http://developer.gnome.org/tools/cvs.html for information about acccessing
GNOME CVS anonymously)

Also, as policy for the way we do port-specific #defines, the standard
is that you should define a:

 G_OS_VMS

macro in GLib and use that of _VMS.
 
> diff -u -r -x*.OBJ -x*.EXE [gtk.gtk_-1_2_7.GDK]GDKDND.C [gtk.gtk.GDK]GDKDND.C
> --- [gtk.gtk_-1_2_7.GDK]GDKDND.C	Wed Feb 16 11:18:01 2000
> +++ [gtk.gtk.GDK]GDKDND.C	Fri May 19 09:13:27 2000
> @@ -890,6 +890,11 @@
>  			      -100, -100, 10, 10, 0, 0,
>  			      InputOnly, CopyFromParent,
>  			      (CWOverrideRedirect | CWEventMask), &attr);
> +#ifdef __VMS
> +#include "config.h"
> +#include <starlet.h>
> +              VMS_SETUP_WINDOW_NOTIFICATION(display,motif_drag_window)
> +#endif

I'd like to see the definition of this macro. It's hard to imagine something
that is enough like X that the rest of GDK works, and then you need to 
make a special call here... 

But, as a general rule, doing it like this is wrong. You should be trying
to avoid scattering ifdef's in the code. Make a function:

 gdk_x11_setup_window_notification()

Which is a NOP on normal machines and does whatever VMS magic is needed here.

> +#ifdef __VMS
> +
> +#define GDK_DRemove 0x1000FF00 /* XK_DRemove */
> +
> +static int vms_keysyms = -1;
> +
> +static KeySym VMS_translate_keysym(KeySym key) {
> +
> +    /* If first time, pick up environment variable setting */
> +    if (vms_keysyms == -1) {
> +        if (getenv("GTK_NO_KEY_TRANSLATION") == NULL)
> +            vms_keysyms = 1;
> +        else
> +            vms_keysyms = 0;
> +    }
> +
> +    /* If we are doing the magic, then do it */
> +    if (vms_keysyms) {
> +        switch (key) {
> +          case GDK_Delete:      return GDK_BackSpace;
> +          case GDK_Select:      return GDK_Delete;
> +          case GDK_Find:        return GDK_Insert;
> +          case GDK_Insert:      return GDK_Home;
> +          case GDK_Page_Up:     return GDK_End;
> +          case GDK_DRemove:     return GDK_Page_Up;
> +          case GDK_Page_Down:   return GDK_Page_Down;
> +          default:              break;
> +        }
> +    }

I can't imagine why you would want this. Presumably somebody running X on
VMS has made X return the right keysyms? 

The X protocol defines the key symbol values, the X protocol values
are in gdkkeysymdef.h. This cannot be correct.

> diff -u -r -x*.OBJ -x*.EXE [gtk.gtk_-1_2_7.GDK]GXID.C [gtk.gtk.GDK]GXID.C
> --- [gtk.gtk_-1_2_7.GDK]GXID.C	Sun Aug 16 21:15:07 1998
> +++ [gtk.gtk.GDK]GXID.C	Thu May 18 13:11:22 2000
> @@ -18,6 +18,10 @@
>  
>  #include "gxid_proto.h"
>  
> +#if defined(__VMS)
> +#define socket_fd fd_socket
> +#endif
> +
>  /* #define DEBUG_CLIENTS  */
>  /* #define DEBUG_EVENTS */

socket_fd is a private variable here. If it conflicts with something
on VMS, then it should be renamed always, not worked around with magic
#defines.
  
> diff -u -r -x*.OBJ -x*.EXE [gtk.gtk_-1_2_7.GTK]GTKFILESEL.C [gtk.gtk.GTK]GTKFILESEL.C
> --- [gtk.gtk_-1_2_7.GTK]GTKFILESEL.C	Wed Feb 16 11:18:07 2000
> +++ [gtk.gtk.GTK]GTKFILESEL.C	Fri May 19 13:42:41 2000
> @@ -56,6 +56,14 @@
>  #include "gtkdialog.h"
>  #include "gtkintl.h"
>  
> +#ifdef __VMS
> +/*
> +** These are required because on OpenVMS ino_t isn't a scalar.
> +*/
> +#define ino_cpy(dest,from) (memcpy((void*)dest,(void*)from,3*sizeof(ino_t)))
> +#define ino_eql(e1,e2) (memcmp((void *)e1,(void *)e2,3*sizeof(ino_t))==0)
> +#endif
> +

Again, do this as:

#ifdef __VMS
#define ino_cpy(dest,from) (memcpy((void*)dest,(void*)from,3*sizeof(ino_t)))
#define ino_eql(e1,e2) (memcmp((void *)e1,(void *)e2,3*sizeof(ino_t))==0)
#else
#define ino_cpy(dest,from) (dest = src)
#define ino_eql(dest,from) (dest == from)
#define

Avoid scattering #defines all over the place. (I'd rather see ino_copy, ino_equal,
and avoid cryptic abbreviations.)

>  #define DIR_LIST_WIDTH   180
>  #define DIR_LIST_HEIGHT  180
>  #define FILE_LIST_WIDTH  180
> @@ -101,7 +109,11 @@
>   */
>  struct _CompletionDirSent
>  {
> +#ifndef __VMS
>    ino_t inode;
> +#else
> +  ino_t inode[3];
> +#endif

Bogggle. You mean the st_ino field is not a single ino_t? What was somebody 
smoking? How could you make something that pretended to be a UNIX compatibility
layer and have such brokenness...

But, anyways, you should do this something like:
 
#ifdef G_OS_VMS
typedef ino_t g_ino_t[3];
#else
typedef ino_t g_ino_t;
#endif

Encapsulate all the ino_t dependencies in one place.

>    time_t mtime;
>    dev_t device;
>  
> @@ -1331,8 +1343,11 @@
>  
>    /* Set the dir_list to include ./ and ../ */
>    text[1] = NULL;
> +#ifndef __VMS
> +/* Don't include "./" in the directory list */
>    text[0] = "./";
>    row = gtk_clist_append (GTK_CLIST (fs->dir_list), text);
> +#endif

But clicking on . is the way to refresh the current directory; if
'.' isn't supported on VMS, I don't think this is the right place
to special case it. Emulating the Unix behavior shiold be pretty
easiyl.
  
>  static gchar*
>  find_parent_dir_fullname(gchar* dirname)
>  {
> +#ifndef __VMS
>    gchar buffer[MAXPATHLEN];
>    gchar buffer2[MAXPATHLEN];
>  
> @@ -2318,6 +2350,32 @@
>      }
>  
>    return g_strdup(buffer2);
> +#else
> +/*
> +** This is the OpenVMS version. All we do here is truncate the string to
> +** the last slash (also excluding any trailing slashes that might be there).
> +** If the last character is a /, then we need to locate the second from
> +** last slash. This is a bug that's common to all platforms, but for

Could you expand on this more, what is a bug?

> +** everyone else just means that we use the "hard way" to find our parent.
> +** But on OpenVMS the "hard way" fails if we are in something like "/dka100"
> +** since we can't chdir into "/" (it doesn't exist on OpenVMS).
> +*/
> +  gchar buffer[MAXPATHLEN+1], *last_slash;
> +  int len=strlen(dirname);
> +  strcpy(buffer,dirname);
> +  while (len > 1) {
> +    if (buffer[len-1] == '/') {
> +      buffer[len-1] = 0;
> +      len--;
> +    }
> +    else
> +      break;
> +  }
> +  last_slash = strrchr(buffer, '/');
> +  g_assert(last_slash);
> +  last_slash[1] = 0;
> +  return g_strdup(buffer);
> +#endif
>  }
>  
>  /**********************************************************************/
> @@ -2389,7 +2447,11 @@
>  {
>    gint diff = 0;
>  
> +#ifndef __VMS
>    while(*pat && *text && *text == *pat)
> +#else
> +  while(*pat && *text && !strncasecmp(text,pat,1))
> +#endif

This is already done in GTK+ 1.3

===
#if defined(G_OS_WIN32) || defined(G_WITH_CYGWIN)
#define FOLD(c) (tolower(c))
#else
#define FOLD(c) (c)
#endif

/* returns the index (>= 0) of the first differing character,
 * PATTERN_MATCH if the completion matches */
static gint
first_diff_index(gchar* pat, gchar* text)
{
  gint diff = 0;

  while(*pat && *text && FOLD(*text) == FOLD(*pat))
===

Which is going to be faster. Though, using tolower() on lower case characters
is not completely portable ... this should be:

 FOLD(c) (isupper(c) ? tolower(c) : c)

>      {
>        pat += 1;
>        text += 1;

> diff -u -r -x*.OBJ -x*.EXE [gtk.gtk_-1_2_7.GTK]GTKMAIN.C [gtk.gtk.GTK]GTKMAIN.C
> --- [gtk.gtk_-1_2_7.GTK]GTKMAIN.C	Wed Feb 16 11:18:07 2000
> +++ [gtk.gtk.GTK]GTKMAIN.C	Fri May 19 13:46:44 2000
> @@ -51,6 +51,9 @@
>  #include "config.h"
>  #include "gtkdebug.h"
>  #include "gtkintl.h"
> +#ifdef __VMS
> +#include <unixlib.h>
> +#endif
>  
>  /* Private type definitions
>   */
> @@ -185,6 +188,31 @@
>  
>    if (gtk_initialized)
>      return TRUE;
> +
> +#ifdef __VMS
> +/*
> +** In many places in the code, getenv("HOME") is used to get the home
> +** directory. On OpenVMS this returns something like DKA0:[COLIN], but
> +** in several places in the code, it expects this to be an absolute
> +** UNIX filespec such as /dka0/colin. So, rather then changes all calls
> +** to getenv("HOME"), let's fix HOME.
> +**
> +** There is a bug in the CRTL where a call to stat for a directory such as
> +** /sys$sysroot/sysmgr/.netscape will fail if the .netscape directory is in
> +** the SYS0 sysmgr (it works fine if its in the common sysmgr, but mkdir
> +** will of course create it in SYS0). Since the most common case where this
> +** happens is the SYSTEM account, we avoid the problem here by forcing it
> +** to the common sysmgr.
> +*/
> +{
> +  char *hp;
> +  hp = decc$translate_vms(getenv("HOME"));
> +  if (strcmp(hp,"/sys$sysroot/sysmgr"))
> +    setenv ( "HOME", hp, 1 );
> +  else
> +    setenv ( "HOME", "/sys$common/sysmgr", 1 );
> +}
> +#endif

Everywhere in GTK+, things should be using g_get_home_dir(). You should do
this translation in that function. Note, that with the Win32 port, most
dependencies on Unix path names should be removed and there are provisions
for things like C:\foo\bar. 

So, perhaps the DKA:[COLIN] would work now? (Not that I have much of an
idea of how VMS path names work)
  
>  #if	0
>    g_set_error_handler (gtk_error);
> diff -u -r -x*.OBJ -x*.EXE [gtk.gtk_-1_2_7.GTK]TESTRGB.C [gtk.gtk.GTK]TESTRGB.C
> --- [gtk.gtk_-1_2_7.GTK]TESTRGB.C	Wed Feb 24 10:15:18 1999
> +++ [gtk.gtk.GTK]TESTRGB.C	Fri May 19 07:51:29 2000
> @@ -49,10 +49,13 @@
>  get_time (void)
>  {
>    struct timeval tv;
> +#if !defined(__VMS)
>    struct timezone tz;
>  
>    gettimeofday (&tv, &tz);
> -
> +#else
> +  gettimeofday (&tv, NULL);
> +#endif
>    return tv.tv_sec + 1e-6 * tv.tv_usec;
>  }
>  
> 
> ----------

Make this use g_get_current_time(), do whatever fixing is needed in that function.

Regards,
                                                     Owen