[mythtv] first contribution

Nigel Pearson nigel at ind.tansu.com.au
Thu Sep 20 03:02:51 UTC 2007


Thanks for your patch. Feel free to do some more!

(i.e. understanding & documenting all
       of the other nasty classes :-)



> I guess the last step is feedback?

Daniel has grabbed the ticket, so he may give you some
before he commits it, but here are my trivial comments:


1) Some whitespace changes slipped in there. e.g.
-    MythThemedMenuPrivate(MythThemedMenu *lparent, const char *cdir,
+    /**
+     * @param lparent menu that owns this instance
+     * @param cdir directory where theme is stored
+     * @param lstate corresponding settings of the theme
+     */
+   MythThemedMenuPrivate(MythThemedMenu *lparent, const char *cdir,

2) Some of the brief descriptions
are too long to be, er, "brief"?

I reckon if the brief is more than about 60 chars,
then it needs to be split into a brief,
and a more detailed paragraph.

In the generated Dox, this will be split between
the function summaries (the brief),
and the more detailed "Member Function Documentation"
(everything else)


3) A few of the comment blocks should
probably have a brief tag?  e.g.:
     /**
      * Set up UI according to the corresponding mythThemedMenuState
      */
     void SetupUITypes();


4) Some of the descriptions are a tiny-bit obvious,
and could maybe be removed?  e.g. this one:
     /**
      * @return true if the theme has been found
      */
     bool foundTheme(void);


--
Nigel Pearson, nigel at ind.tansu.com.au|     "They did the
Telstra Net. Eng., Sydney, Australia | white man power dance,
Office: 9202 3900    Fax:  9261 3912 |     and ... Shazam"
Mobile: 0408 664435  Home: 9792 6998 | Lois Lane - Smallville


More information about the mythtv-dev mailing list