Created attachment 55128 [details] [review]
Patch

Created attachment 55129 [details]
visual demonstration

Thanks for the patch.  I personally think it makes sense for this kind of mixed
mode thing (I'd like it when I use it), and it should work no different than now
for both the everything-runs-locally and LTSP cases; however, Havoc is more
familiar with all the weird cases out there.  Havoc, can you think of anything
that this might be problematic for?

Anyway, some comments on the patch:

- No tabs allowed in Metacity code.  Tabs are Evil (TM).  ;-)

- coding style is to have braces two spaces further indented than the if/else
(the code between braces is still an extra two spaces beyond the braces); i.e.
gtk+ style.

- I'd rather have set_title_text() take an Atom parameter so that it can be
defined by as
  /**
   * Called by set_window_title and set_icon_title to set the value of
   * *target to an appropriately modified version of title.  Also sets the
   * specified atom if the window title is modified.
   */
i.e. have the meta_prop_set_utf8_string_hint() inside that function too.

- I'd rather remove the triple else that comes up in set_title_text(); leaving
it there would make it harder to make other title modifications.  Besides, I
think it'd be easier to just start a new if at the end that sets the utf8 string
hint property if modified is true and otherwise does the necessary string duping
to set *target.

- If a window has no title and is running on a different machine than metacity,
should it have a blank title or a title that merely states that it's running on
a different machine?  Your code does the first but it seems that the second
would be more consistent.

Created attachment 55139 [details] [review]
refactoring per Elijah's comments

Looks good, just two more nitpicks:

- As per elsewhere in the code, stuff in an if/else branch that is not
surrounded by spaces should only be indented two more spaces over instead of four.

- The boolean check on atom seems a little weird (I had to go searching through
documentation to figure out what it meant and whethere it was okay; maybe I
should have known but I didn't).  I'd prefer it changed to "atom != None", so
that it's a little clearer.  It's still not the kind of thing we really check
anywhere (functions which take an atom only accept valid atoms seems to be the
convention) so it still stands out but I guess it doesn't hurt.

1. Ok. My apologies. I'm not highly familiar with the coding style. For
reference, is there a useful set of vim instructions to get this sort of indenting?

2. This is my fault. I kinda assumed that Atom was secretly a (void *) (which I
recall is popular in X land). I just wanted to check that something bad hadn't
been passed. Also perhaps to allow the function to be used without an Atom
(useful?).

Did you just want to clean it up and commit it at the same time, or did you want
me to send you another version?

No worries; thanks for putting up with all the nitpicks.  :)  I think a large
number of the issues were due my having not been very clear when we talked in
IRC; sorry about that.  Anyway, I have no clue about how to modify vim to get
appropriate indentation and I don't know whether or not it's useful to to allow
the function to be used without an Atom (it's just not the kind of thing we do
elsewhere so thus the reason I started looking stuff up).  Havoc would have to
comment on the atom stuff.

Go ahead and attach a new version; I think this is pretty cool, but Havoc still
needs to approve it.

Created attachment 55152 [details] [review]
final update

Comment on attachment 55152 [details] [review]
final update

The patch seems fine to me; I don't honestly know if it will hose up ltsp (not
sure how that usually works), I think if it does cause big problems for an ltsp
setup we should consider reverting the patch. But fine to commit and give it a
shot.

2005-11-24  Davyd Madeley  <davyd@fugro-fsi.com.au>

        * src/window-props.c: display hostname in titlebar for remote X
          clients. Closes bug #322202.