Created attachment 788 [details]
openbox-3.3-rc2-_NET_WORKAREA-amd64.patch

If what you say is true, which i believe it is, then a lot of variables need
changing in this file. The actual function that calls XChangeProperty has
guint32 in the function definition. So if I understand correctly, this should
just be long, not unsigned long or anything? I have got some 64-bit bug reports
before, but you're the first with a patch :).

I have got my information from freedesktop and a xlib manual:
http://tronche.com/gui/x/xlib/window-information/XChangeProperty.html
http://standards.freedesktop.org/wm-spec/latest/ar01s03.html

As I understand it, one should use long (not unsigned or anything else). At
least that is what is says in the XChangeProperty manual.
The patch I have attached fixes the problem on my amd64 machine, and does not
introduced any new problems on my x86 workstation at the univerisity (I've tried
it today, before posting this bug-report).

Another indication of that this is a correct patch, is that this is the way
firefox handles X-properties.
A small sample I have found during my bug-hunting
(mozilla/gfx/src/gtk/nsScreenGtk.cpp):

void
nsScreenGtk :: Init ()
{
  mAvailRect = mRect = nsRect(0, 0, gdk_screen_width(), gdk_screen_height());

  // We need to account for the taskbar, etc in the available rect.
  // See http://freedesktop.org/Standards/wm-spec/index.html#id2767771

  // XXX It doesn't change that often, but we should probably
  // listen for changes to _NET_WORKAREA.
  // XXX do we care about _NET_WM_STRUT_PARTIAL?  That will
  // add much more complexity to the code here (our screen
  // could have a non-rectangular shape), but should
  // lead to greater accuracy.

#if GTK_CHECK_VERSION(2,2,0)
  GdkWindow *root_window = gdk_get_default_root_window();
#else
  GdkWindow *root_window = GDK_ROOT_PARENT();
#endif // GTK_CHECK_VERSION(2,2,0)

  long *workareas;
  GdkAtom type_returned;
  int format_returned;
  int length_returned;

#if GTK_CHECK_VERSION(2,0,0)
  GdkAtom cardinal_atom = gdk_x11_xatom_to_atom(XA_CARDINAL);
#else
  GdkAtom cardinal_atom = (GdkAtom) XA_CARDINAL;
#endif

  gdk_error_trap_push();

  if (!gdk_property_get(root_window,
                        gdk_atom_intern ("_NET_WORKAREA", FALSE),
                        cardinal_atom,
                        0, G_MAXLONG, FALSE,
                        &type_returned,
                        &format_returned,
                        &length_returned,
                        (guchar **) &workareas)) {
    // This window manager doesn't support the freedesktop standard.
    // Nothing we can do about it, so assume full screen size.
    return;
  }

and so on... (it just checkes that the workarea returned by the window manager
(openbox in this case) sets reasonable values for _NET_WORKAREA)

I have also had a quick look at the openbox code, and it seems there could be a
lot of places where guint32 is used where long is actually the correct choice.
I might have a look at it in the weekend and send more patches if I find other
bugs. But if I don't find the time to go bug-hunting, I might have time to test
if you have some patches for this kind of bugs or any other amd64 related
problems. Feel free to contact me.

Okay, thanks. I'll apply this patch then.

Created attachment 791 [details]
openbox-3.3-rc2-64bit-property.patch

I found several places where guint32 is used incorrectly instead of gulong (If
properties are negative values, the functions using them should cast between
singned and unsigned).
I think this should fix all the occurences where properties are set, but I
haven't looked at problems (I don't know if there are any) with getting
properties.

Anyway, this patch also make firefox startup maximized if I maximized it before
I closed it last time... So bugs are fixed by this patch.
I haven't had the time to test it on a non-64-bit machine.

Btw. this patch messed with stuff the other patch (788) touched, so it might
not apply cleanly against CVS (it would be easiere with CVS access :-) )

that's why they invented interdiff :)

@@ -245,7 +245,7 @@
                     (*data)[i] = xdata[i];
                     break;
                 case 16:
-                    ((guint16*)*data)[i] = ((guint16*)xdata)[i];
+                    ((gushort*)*data)[i] = ((gushort*)xdata)[i];
                     break;
                 case 32:
                     ((guint32*)*data)[i] = ((gulong*)xdata)[i];

^ there are some more hunks like that, why should the 32 case still be guint32?

Cool... I didn't know about interdiff (it seems very usefull)

The following is my understanding of X-properties (they might be wrong):

The point about guint32 vs. long is that:
- properties are 32, 16 or 8-bits (always, no matter what type is used to
retrieve  or set them).
- If one wants to set or read 8-bit properties, char should be used.
- If one wants to set or read 16-bit properties, short should be used (thats way
I changed the use of guint16 to gushort).
- If one want to set or read 32-bit properties, long should be used (eventhought
a long is 64 bits on amd64, the extra bits is just wasted, but they are very
important when dealing/indexing with arrays!)

Most of the openbox code uses guint32 to read 32-bits properties into, which
should be correct. But that dosen't change the fact that they should be read (in
the function get_all in prop.c) using a long array. But the values a each index
of the long array is 32-bit values.
Thats why
((guint32*)*data)[i] = ((gulong*)xdata)[i];
is necessary to make C calculate the correct index, since data is a guchar array
and xdata (the read property) is a long array (but declared as guchar, since it
shold also handle the 8 and 16-bit cases).

I hope the description is clear enough and correct :-)

that explains why the 32: has guint32 on the lhs and gulong on the rhs, but not
why the 16: case has gushort on both sides :)

Maybe I was a bit quick there, the correct line should probably read:
((guint16*)*data)[i] = ((gushort*)xdata)[i];
but as far as i'm concerned guint16 == gushort on both x86 and amd64, so not a
big difference.
But you are right, we should get it completely right now we are looking at it.

Okay, i've changed those two cases now, hopefully everything is right now.

Actually when I come to think about it, the patch i'm going to attach in a
moment seems better than the old one.
It has the following advantages:
- It is shorter (but still fixes the problem)
- It is more to the point... Capturing that props are 8, 16 or 32 bits but they
still get set/read right.
- It involves changes in fewer functions, and the changes are actually to the
functions where it seems appropriated.

Sorry for bothering you again :-)

Created attachment 794 [details]
openbox-3.3-rc2-64bit-property-v2.patch

i'll look at this later [tm] :)

i think i will leave it with the first version