Re: OpenVMS GTK diffs

[Colin Blake <colin theblakes com>]

Thanks for your comments Owen. Here's my replies:

Owen Taylor wrote:

> 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)

OK, will use CVS and pull the latest and work with that.

> 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.

OK. Was only using __VMS because that's defined automatically by the VMS compiler.

Does this mean that glib.h is included by EVERY gtk source file? Or can platform-specific
changes ONLY be made in Glib (I know you said that Gib is the preferred place for
platform-specific changes, but unless glib.h is included everywhere, I'll still need to use
__VMS in gtk).

> > 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.

On OpenVMS, you can not use ConnectionNumber in a select/poll to be notified when there is
something in the X event queue (I believe the spec says that what comes back as
ConnectionNumber is implementation dependent, and on VMS its NOT an fd). So I created
VMS_SETUP_WINDOW_NOTIFICATION (a macro for convenience, but I can make it a routine). This
code has to be called every time a window is created and its purpose is to cause a VMS
event flag (an event flag is a primitive notification mechanism on VMS) to be set each time
an X event occurs. I then use this event flag in the poll/select, and the rest of GDK is
pretty much unaltered.

Compared to what I was doing to the idle loop before, this approach is MUCH nicer!!

I think a good way to do this would be to have all gtk code call some new routine such as
gCreateWindow instead of XCreateWindow directly. Then in gCreateWindow we call
XCreateWindow and then perform any platform specific stuff. This way I don't have to worry
about whenever someone adds some new code to create a window. Thoughts?

> > +#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.

The X server on VMS essentially doesn't map the keys the way you expect. This is (I
believe) due to the history of the Digital keyboard and because the words printed on the
keycaps has changed over time. Believe me, if I don't make these translations, then the
keyboard behaves VERY strangely when running a GTK app. These translations are required to
make the keyboard appear normal.

With the exception of Delete/Backspace, all the keys which get translated are those in the
little cluster of 6 keys in the "middle" of the keyboard. These are the ones which have
gotten renamed over time on Digital keyboards. I don't know for sure, but I suspect that
Digital originally had the keys in a different layout to everyone else, but then admitted
defeat and changed the layout to match everyone else's.

The delete/backspace translation is there because on VMS the BACKSPACE key always deletes
the character to the left. VMS users get VERY upset if it deletes to the right.

> > 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.

OK, I'll rename it for everyone (and yes, I notified the VMS C folks and they are fixing
this snafu in later versions).

> > 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.)

OK. Where's the right place to put these #defines. They might be needed in other modules at
a later date.

> >  #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...

Correct. In stat.h st_ino is defined as:

  __ino_t  st_ino[3];

> 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.

Gotcha.

> >    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.

I took it out because "." isn't very intuitive for VMS users who have a non-UNIX file
system, and it didn't seem to serve any purpose (I thought directory contents were cached
by gtk, and so clicking on "." wouldn't force a refresh, but I guess I'm wrong). Anyway,
I'll put it back in.

> >  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?

There are two ways the gtk code determines a file's parent. The "easy way" is to just strip
off the last term. The "hard way" is to chdir into "..", do a getcwd, and then chdir back
again. The "hard way" doesn't work on VMS all the time, because something such as "/dka0"
isn't a directory on VMS (its a device) and if you try to chdir("/dka0") you won't get what
you expect. So the "hard way" has to be avoided at all costs on VMS.

I believe the bug I was referring to (its a while since I wrote that code) is this.
correct_parent tries to determine if what we think is the parent really is. It does this by
taking full spec such as "/foo/bar", lopping of the final term leaving "/foo", and then
comparing st_ino/st_dev of "/foo" with the expected st_ino/st_dev. If the test succeeds,
all is well (the "easy way" worked). If the test fails then we resort to the "hard way".
The "hard way" is we call find_parent_dir_fullname where we chdir into ".." and getcwd. As
stated above this won't always work on VMS. Anyway, the bug is that easy test will come to
the wrong conclusion if passed something such as "/foo/bar/". In this case it will strip
off the last "/" and then check "/foo/bar" to see if its the parent (of "/foo/bar/"). Its
not, as so gtk resorts to the hard way to find the parent. The bug is that the easy test
should first strip off any trailing "/"'s before looking for the last "/". If it did this,
then the easy test would work all the time (except for soft links). So the fix for VMS is
to always do it the "easy way", but do the easy way "right". And since VMS doesn't have
soft links, we're all set.

So, I think if the code in correct_parent was changed to remove trailing slashes before
finding the last "/", then VMS would never find itself in the situation where it needs to
use the hard way. That's the correct fix, I believe. The reason I didn't fix it this way
originally was because I wasn't 100% sure and I didn't want to break non-VMS code.

> > +** 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
> >  }
> >

See comments above.

> >  /**********************************************************************/
> > @@ -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

Good.

> ===
> #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;

The spec (http://www.opengroup.org/onlinepubs/7908799/xsh/tolower.html) clearly says "On
successful completion, tolower() returns the lower-case letter corresponding to the
argument passed; otherwise it returns the argument unchanged.". You shouldn't need the
isupper call?

> > 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)

This is a long story. The short version if that the C RTL (c lib) on VMS will accept UNIX
style file specs and translate them to VMS itself. So I can pass in
"/dka0/colin/myfile.dat" and it will find/use DKA0:[COLIN]MYFILE.DAT. This means that in
99% of the code I don't have to worry about translating filespecs before using them, and I
can play all the usual UNIX tricks like concatenating a directory name to a file name using
"/". This is great since I means I don't have to modify code. The 1% where I do get hit is
that HOME is defined in VMS format. Rather than find all the places which do a
getenv("HOME") and fix those, I decided to just redefine HOME to be its UNIX version
equivalent at startup and be done with it. This way I KNOW I've got them all.

But if there's now a g_get_home_dir call, and if you can guarantee that no-one will ever do
a getenv("HOME") again, then I can move my fix into there. I'll probably still redefine
HOME (and use a static flag so I only do it once), otherwise I'm translating from VMS to
UNIX format each time g_get_home_dir is called.

> >  #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.

OK. Will do.