Created attachment 37312 [details] [review]
The proposed code cleanup pach

So the short answer is this is not right, but I'll give you the long answer too
since I hope you aren't discouraged and will contribute other patches.

First the big picture issue is that gtk/gtk.h is not included in tabpopup.h, and
that's why the TODO you have to merge the two TabEntry is not correct. The
TabEntry in the .c file is an implementation detail of the widget, and the one
in the .h file is an API. The difference between them is the difference between
interface and implementation.

Read HACKING and doc/code-overview.txt to learn about why gtk.h isn't included.

More stylistic and nitpick issues:
- use "diff -uP" rather than plain diff for patches
- there are some indentation style issues such as braces in the same 
  column as the if()
- I think I don't use gchar*, if I do don't copy it ;-)
- don't put close parens on their own line
- metatabentry_to_tabentry should have more underscores in it and would
  normally be called tab_entry_new() or something to that effect
- factoring out tab_entry_new() is plausible
- I would not use doxygen comments, because none of the rest of the codebase
  uses doxygen comments. Also these comments are really too long; the 
  information in them should be fairly clear from the code itself.

I'll work on factoring out the tab_entry_new() function and fixing the stylistic
problems.

I had noticed that the TabEntry is implementation whilst the MetaTabEntry is
API, and on reading doc/code-overview.txt I see your reasons.

What comment style should I use (for my less verbose comments :)?

- I've factor'ed out the tab_entry_new() function.
- used diff -uP to create patch
- fixed indentation problems (well Emacs did)
- Chaged the gchar* (from your code) to a char * to match the char * in the
TabEntry struct.
- removed Doxygen comments but left in two one liners, I copied your commenting
style aswel.

There's no real use for this patch except for negating the need for a
g_list_reverse().  All it does is factor out tab_entry_new.  If you want to
apply it (and it's good enough) the ChangeLog entry would be:

2005-02-11 Aidan Delaney <adelaney@cs.may.ie>

    Trivial code refactoring.  Fixes #166980

    * src/tabpopup.c: (tab_entry_new): Added method to create new TabEntry from
a MetaTabEntry.
    * src/tabpopup.c: (meta_ui_tab_popup_new): Appended to list, instead of
prepending, negating the need to reverse the list.

Created attachment 37346 [details] [review]
Havoc friendly version of first patch :)

Metacity doesn't use tabs (some have crept in, but not intentionally).  You have
quite a few in your patch, please remove them.  Also, please use -p when
creating patches (which is not the same as -P; Havoc made a mistake in his
instructions this time because he has always requested "diff -up" before...)

Yeah the -p or -P I mean is the one that shows function names.

Comment style should just be nice readable notes on things that aren't obvious
from the names and organization in the code.

I'm having problems getting the indentation right.  If anyone uses Emacs can I
grab your .emacs file.  Or if you're vi heads and this behaviour is default vi,
I'll switch.

At the risk of sounding like a total idiot...I did some reading on coding
standards and it looks to me that Havocs standards are the GNU coding standards.
 Is this correct?

I also ran the CVS tabpopup.h through "cat -T" and found that it contains a lot
of tab characters [1].

So this patch now makes tabpopup.c in strict conformance with GNU coding
standards with tabs-as-spaces being 8 spaces wide.  I also converted any line
matching the regexp ^\w*$ to ^\n$.

I'm sure at this stage that I've got the formatting right, if it is meant to
follow GNU coding standards.  If it's not, please tell me so I can redo it.

[1]balor@localhost$ cat -T metacity/src/tabpopup.c | grep "\^I"
      pixels = row;^I^I^I
          pixels[3] /= 2;^I^I^I^I
        }^I^I^I
^I^I^I gdk_display_get_screen (gdk_display_get_default (),
^I^I^I^I^I^I screen_number));
^I^I^I screen);
^Isizeof (MetaSelectImageClass),
^INULL,           /* base_init */
^INULL,           /* base_finalize */
^I(GClassInitFunc) meta_select_image_class_init,
^INULL,           /* class_finalize */
^INULL,           /* class_data */
^Isizeof (MetaSelectImage),
^I16,             /* n_preallocs */
^I(GInstanceInitFunc) NULL,
^I   (widget->allocation.x + widget->allocation.width
^I    - (widget->requisition.width - misc->xpad * 2)) *
^I   misc->xalign) + 0.5;
^I   (widget->allocation.y + widget->allocation.height
^I    - (widget->requisition.height - misc->ypad * 2)) *
^I   misc->yalign) + 0.5;

Created attachment 37575 [details] [review]
Patch to factor out tab_entry_new and do strict GNU coding stnadards

Almost GNU, metacity code is more the GTK standards which have a couple of minor
differences (e.g. prototype formatting)

It's best not to mix whitespace cleanups on existing code with other kinds of
change.

One of the following may be helpful to you while working on metacity in emacs:
  M-x eval-expression<enter>(setq indent-tabs-mode nil)
  M-x untabify
  M-% <tab><enter><8 spaces><enter>

I have a line containing "(setq-default indent-tabs-mode nil)" in my .emacs file
(which I can turn off with "M-x eval-expression<enter>(setq indent-tabs-mode
't)" when working on less-enlightened gnome modules).

Created attachment 37854 [details] [review]
Factor patch,  not including whitespace patch.

One distro-switch to Ubuntu and here's the patch.  It factors out the
tab_entry_new method and doesn't address the whitespace "problems".  I'll
submit something else for that.

Copy-paste this to your .emacs to solve the coding style problems:

(defun havoc-c-style()
  (interactive)
  (c-set-style "gnu")
  (c-set-offset 'arglist-cont-nonempty 'c-lineup-arglist)
  (setq indent-tabs-mode nil)
  (call-interactively 'untabify)
  )

Then when editing metacity code type M-x havoc-c-style <Ret> and your C code
should be good. Three things with the patch: You are using both char * and gchar
* in tab_entry_new() and that just can't be good. :) And you have replaced the
g_list_prepend()/g_list_reverse idiom() with g_list_append(). For some odd
reason g_list_append() is O(n) while g_list_prepend is O(1), not that it should
matter much for performance since the list of windows should be fairly small,
and g_list_append() is one line shorter. More serious is this:

markup = g_strdup_printf ("<b>%s</b>", g_markup_escape_text (tmp, -1));

Doesn't it make g_markup_escape_text() leak memory because there is no g_free?

Patch no longer applies cleanly...

Created attachment 66383 [details] [review]
Factors out tab_entry_new

Patch should now apply cleanly.  I've addressed Björns comments but not replaced the g_list_prepend()/g_list_reverse idiom() as there dosn't seem to be a major time penalty as the lists are small.  If I used the idiom I'd have to put the extra temporary GList back.  But I'll do whatever's suggested.

Basically the patch applies and it factors out the tab_entry creation.  I'd consider this bug closed even if you decide not to apply the patch.

Thanks Aidan, I committed with a couple small changes (you added a couple tabs, plus I noticed one or two other additional things I wanted to cleanup).

2006-06-10  Elijah Newren  <newren gmail com>

	Patch from Aidan Delaney to tidy tabpopup.c by factoring out
	tab_entry_new().  #166890.

	* src/tabpopup.c (tab_entry_new): new function,
	(meta_ui_tab_popup_new): use tab_entry_new() to remove a big chunk
	of code, plus a few other small cleanups.