After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 437469 - Doxer support for bullet lists in man pages
Doxer support for bullet lists in man pages
Status: RESOLVED FIXED
Product: beast
Classification: Other
Component: docs
SVN trunk
Other Linux
: High enhancement
: m0.7
Assigned To: Beast Maintainers
Beast Maintainers
Depends on:
Blocks:
 
 
Reported: 2007-05-10 14:53 UTC by Stefan Westerfeld
Modified: 2010-06-10 12:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Doxer bullet lists for man pages patch (6.39 KB, patch)
2007-05-10 14:55 UTC, Stefan Westerfeld
none Details | Review
Doxer bullet lists for man pages patch, 2nd attempt (6.26 KB, patch)
2007-06-21 14:25 UTC, Stefan Westerfeld
none Details | Review
Doxer bullet lists for man pages patch, 3rd attempt (6.96 KB, patch)
2007-08-17 21:33 UTC, Stefan Westerfeld
none Details | Review
Doxer bullet lists for man pages patch, 4th attempt (7.39 KB, patch)
2007-09-16 21:59 UTC, Stefan Westerfeld
none Details | Review
Same patch, better manpage (21.17 KB, patch)
2007-11-10 20:14 UTC, Stefan Westerfeld
none Details | Review

Description Stefan Westerfeld 2007-05-10 14:53:53 UTC
Here is a patch for doxer (SVN r4339), making it possible to use

@itemize{bullet}

in man pages. I've also included the beginnings of a bsewavetool.1 manpage, so that you can try the feature.
Comment 1 Stefan Westerfeld 2007-05-10 14:55:22 UTC
Created attachment 87958 [details] [review]
Doxer bullet lists for man pages patch

This patch was made without the usual -b, as python sources are whitespace sensitive.
Comment 2 Tim Janik 2007-05-19 16:35:06 UTC
Comment on attachment 87958 [details] [review]
Doxer bullet lists for man pages patch

>Index: doxer/ManGenerator.py
>===================================================================
>--- doxer/ManGenerator.py	(Revision 4335)
>+++ doxer/ManGenerator.py	(Arbeitskopie)
>@@ -2,6 +2,7 @@
> #
> # Doxer - Software documentation system
> # Copyright (C) 2006 Tim Janik
>+# Copyright (C) 2007 Stefan Westerfeld
> #
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
>@@ -142,6 +143,7 @@ class ManGenerator:
>   def __init__ (self, manstream):
>     self.mstream = manstream
>     self.call_stack = []
>+    self.list_state = []

hm, this looks a *lot* like duplicating state information. and duplicate data bookkeeping is always bad (hard to maintain, debug and get right in general).
you should be able to get at the list state, simply by walking the parent nodes of an item, grep for call_stack in HtmlGenerator.py to find existing examples.

>   # transformation functions
>   def lookup_handler (self, tagname, tagdict):
>     handler = tagdict.get (tagname)
>@@ -308,9 +310,40 @@ class ManGenerator:
>     self.mstream << '\n'
>     transformer (node, keyword_dict, NODE_TEXT)
>     self.mstream << '\n\n.PP\n'
>+  def doxer_list_close_item (self):
>+    if self.list_state[-1] == 'item(none)':
>+      self.list_state.pop()
>+    elif self.list_state[-1] == 'item(bullet)':
>+      self.mstream << '\n.LP\n'
>+      self.list_state.pop()
>+  def doxer_list (self, data, transformer, node, keyword_dict):
>+    list_type = 'list(bullet)'
>+    tokens = node.arg
>+    while tokens:
>+      arg, tokens = DoxiParser.split_arg_from_token_list (tokens)
>+      arg = DoxiParser.plain_text_from_token_list (arg).strip()
>+      if   arg == 'BULLET':     list_type = 'list(bullet)'
>+      elif arg == 'bullet':     list_type = 'list(bullet)'
>+      elif arg == 'none':       list_type = 'list(none)'
>+      else:
>+        raise SemanticError ("%s:%u: Unknown list type: %s" % (node.fname, node.fline, arg))
>+    self.list_state += [ list_type ]
>+    self.mstream << '\n'
>+    transformer (node, keyword_dict, NODE_TEXT)
>+    self.doxer_list_close_item()
>+    self.mstream << '\n\n.PP\n'
>+    self.list_state.pop()
>   def doxer_item (self, data, transformer, node, keyword_dict):
>-    self.mstream << '\n.TP\n'
>-    transformer (node, keyword_dict)
>+    self.doxer_list_close_item()
>+    if self.list_state[-1] == 'list(none)':
>+      self.mstream << '\n.TP\n'
>+      self.list_state += [ 'item(none)' ]
>+    elif self.list_state[-1] == 'list(bullet)':
>+      self.mstream << '\n.IP " *" 3\n'
>+      self.list_state += [ 'item(bullet)' ]
>+    else:
>+      raise SemanticError ("%s:%u: list structure cannot be interpreted (unexpected '%s')" % (node.fname, node.fline, self.list_state[-1]))

huh? corrct me if i'm wrong, but adding this statement suddenly breaks man page generations that used to work before, right? i.e. man pages with list styles that you don't directly support.

>+    transformer (node, keyword_dict, NODE_TEXT)
>   def bold (self, tagname, transformer, node, keyword_dict):
>     self.mstream << r'\fB'
>     transformer (node, keyword_dict)
>@@ -370,7 +403,7 @@ class ManGenerator:
>     'doxer_newline'     : linebreak,
>     'doxer_visible'     : doxer_visibility,
>     'doxer_hidden'      : doxer_visibility,
>-    'doxer_list'        : pass_block,
>+    'doxer_list'        : doxer_list,
>     'doxer_deflist'     : pass_block,
>     'doxer_item'        : doxer_item,
>     'doxer_table'       : pass_block,
Comment 3 Stefan Westerfeld 2007-06-21 14:25:45 UTC
Created attachment 90397 [details] [review]
Doxer bullet lists for man pages patch, 2nd attempt

Here is a new version of the patch. I use the call stack to figure out the list type, as suggested. It doesn't reduce the number of state variables (because I need one state variable to know whether I need to close a previously rendered item or not), but it hopefully makes the code a bit more clear.

As for unsupported list types, such as "barf" or "circle", the code will produce a warning on stderr, but treat these types as "none" lists. This will help us to get the information that we may need to enhance the code with new rendering strategies when someone actually starts using these list types, while still failing a bit more gracefully than my previous implementation did.
Comment 4 Tim Janik 2007-06-27 17:12:10 UTC
pling.
Comment 5 Tim Janik 2007-07-07 15:32:54 UTC
Comment on attachment 90397 [details] [review]
Doxer bullet lists for man pages patch, 2nd attempt

>===================================================================
>--- doxer/ManGenerator.py	(revision 4341)
>+++ doxer/ManGenerator.py	(working copy)
>@@ -2,6 +2,7 @@
> #
> # Doxer - Software documentation system
> # Copyright (C) 2006 Tim Janik
>+# Copyright (C) 2007 Stefan Westerfeld
> #
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
>@@ -308,9 +309,41 @@ class ManGenerator:
>     self.mstream << '\n'
>     transformer (node, keyword_dict, NODE_TEXT)
>     self.mstream << '\n\n.PP\n'
>+  def doxer_list_current_type (self):
>+    for node in reversed (self.call_stack):
>+      if node.name == 'doxer_list':
>+	tokens = node.arg

please use spaces instead of tabs, without that, scopes in python patches aren't distinguishable.

>+	ltype = 'none'
>+	while tokens:
>+	  arg, tokens = DoxiParser.split_arg_from_token_list (tokens)
>+	  arg = DoxiParser.plain_text_from_token_list (arg).strip()
>+	  if   arg == 'BULLET':     ltype = 'bullet'
>+	  elif arg == 'bullet':     ltype = 'bullet'
>+	  elif arg == 'none':       ltype = 'none'
>+	  else:
>+	    print >> sys.stderr, "%s:%u: warning: manual page backend does not support list type: %s" % (node.fname, node.fline, arg)
>+	return ltype
>+    raise SemanticError ("%s:%u: doxer_current_list_type called in non-list context" % (node.fname, node.fline))
>+  def doxer_list_close_item (self):
>+    if self.list_item_open and self.doxer_list_current_type() == 'bullet':
>+      self.mstream << '\n.LP\n'
>+    self.list_item_open = False
>+  def doxer_list (self, data, transformer, node, keyword_dict):
>+    # for supporting embedded lists, it might be necessary to do something with the old value of self.list_item_open
>+    self.list_item_open = False

i'm sorry, but that's a fairly bad comment, "do something"? comments should express intentions. what did you mean to say here? the comment and code even contradict each other (since you're actually overwriting the old value).

>+    self.mstream << '\n'
>+    transformer (node, keyword_dict, NODE_TEXT)
>+    self.doxer_list_close_item()
>+    self.mstream << '\n\n.PP\n'
>   def doxer_item (self, data, transformer, node, keyword_dict):
>-    self.mstream << '\n.TP\n'
>-    transformer (node, keyword_dict)
>+    self.doxer_list_close_item()
>+    self.list_item_open = True

hm, i really don't like the unconditional list item handling here. this would be the right thing to do for a function named doxer_list_item, but this function is also called in other contexts. it'd be better to move this logic into a list item specific function and then adapt the function table. 

>+    list_type = self.doxer_list_current_type()

there maybe multiple lists open, and any list type could be interesting (and we're not part of a temporal context here), so that function probably is better named: doxer_list_get_innermost_type() (it should be s/get/find/ if the function wouldn't barf on out-of-list contexts).

>+    if list_type   == 'none':	self.mstream << '\n.TP\n'
>+    elif list_type == 'bullet': self.mstream << '\n.IP " *" 3\n'
>+    else:
>+      raise SemanticError ("%s:%u: bad list type" % (node.fname, node.fline))

this is redundant, doxer_list_current_type() already decides on throwing exceptions for list types or not. also "bad" isn't a good word to use for error messages, it doesn't tell the user "what's bad?" about the type. "invalid" or "unknown" would have been more appropriate. but what you really want here is either to simply remove the raise, or add an assertion if you want to be strict.

>+    transformer (node, keyword_dict, NODE_TEXT)
>   def bold (self, tagname, transformer, node, keyword_dict):
>     self.mstream << r'\fB'
>     transformer (node, keyword_dict)
>@@ -370,7 +403,7 @@ class ManGenerator:
>     'doxer_newline'     : linebreak,
>     'doxer_visible'     : doxer_visibility,
>     'doxer_hidden'      : doxer_visibility,
>-    'doxer_list'        : pass_block,
>+    'doxer_list'        : doxer_list,
>     'doxer_deflist'     : pass_block,
>     'doxer_item'        : doxer_item,
>     'doxer_table'       : pass_block,
Comment 6 Stefan Westerfeld 2007-08-17 21:33:19 UTC
Created attachment 93874 [details] [review]
Doxer bullet lists for man pages patch, 3rd attempt

I have addressed all the issues you had with the last patch.
Comment 7 Tim Janik 2007-08-22 11:54:18 UTC
Comment on attachment 93874 [details] [review]
Doxer bullet lists for man pages patch, 3rd attempt

>Index: doxer/ChangeLog
>===================================================================
>--- doxer/ChangeLog	(revision 4364)
>+++ doxer/ChangeLog	(working copy)
>@@ -1,3 +1,7 @@
>+Thu May 10 16:36:49 2007  Stefan Westerfeld  <stefan@space.twc.de>
>+
>+	* ManGenerator.py: Added support for bullet style lists.
>+
> Fri Jan  5 00:54:29 2007  Tim Janik  <timj@gtk.org>
> 
> 	* qxmlparser.py (DoxygenXMLParser.parse_field): ignore
>Index: doxer/ManGenerator.py
>===================================================================
>--- doxer/ManGenerator.py	(revision 4364)
>+++ doxer/ManGenerator.py	(working copy)
>@@ -2,6 +2,7 @@
> #
> # Doxer - Software documentation system
> # Copyright (C) 2006 Tim Janik
>+# Copyright (C) 2007 Stefan Westerfeld
> #
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
>@@ -308,9 +309,51 @@ class ManGenerator:
>     self.mstream << '\n'
>     transformer (node, keyword_dict, NODE_TEXT)
>     self.mstream << '\n\n.PP\n'
>+  def doxer_list_get_innermost_type (self):
>+    for node in reversed (self.call_stack):
>+      if node.name == 'doxer_list':
>+        tokens = node.arg
>+        ltype = 'none'
>+        while tokens:
>+          arg, tokens = DoxiParser.split_arg_from_token_list (tokens)
>+          arg = DoxiParser.plain_text_from_token_list (arg).strip()
>+          if   arg == 'BULLET':     ltype = 'bullet'
>+          elif arg == 'bullet':     ltype = 'bullet'
>+          elif arg == 'none':       ltype = 'none'
>+          else:
>+            print >> sys.stderr, "%s:%u: warning: manual page backend does not support list type: %s" % (node.fname, node.fline, arg)
>+        return ltype
>+    raise SemanticError ("%s:%u: doxer_current_list_type called in non-list context" % (node.fname, node.fline))

the semanticerror text introduces a hidden dependency, so that doxer_list_get_innermost_type() may only be called by doxer_current_list_type. if that was the case, it should be an inner function. it's probably better to use neutral wording:
    raise SemanticError ("%s:%u: list bullet type encountered in non-list context" % (node.fname, node.fline))

>+  def doxer_list_close_item (self):
>+    if self.list_item_open and self.doxer_list_get_innermost_type() == 'bullet':
>+      self.mstream << '\n.LP\n'
>+    self.list_item_open = False
>+  def doxer_list (self, data, transformer, node, keyword_dict):
>+    # The following implementation does not support embedded lists (an example
>+    # for such a list would be using @itemize inside @itemize). If the need for
>+    # embedded lists in manpages arises later, the following line (and other
>+    # things) will need to be changed, because it does not preserve the old "is
>+    # item opened" state which could be set from the parent list, and would
>+    # need to be restored after the end of the doxer_list function:

thanks for fixing the comment. however, as the comment outlines, the code obviously needs fixing here. we should at least keep our structures intact for nested lists, regardless of what the manpage markup for nested lists will look like.

>+    self.list_item_open = False
>+    self.mstream << '\n'
>+    transformer (node, keyword_dict, NODE_TEXT)
>+    self.doxer_list_close_item()
>+    self.mstream << '\n\n.PP\n'
>   def doxer_item (self, data, transformer, node, keyword_dict):
>-    self.mstream << '\n.TP\n'
>-    transformer (node, keyword_dict)
>+    assert (len (self.call_stack) >= 2) # item can only occur inside lists and deflists
>+    if self.call_stack[-2].name == 'doxer_list': 
>+      self.doxer_list_close_item()
>+      self.list_item_open = True
>+      list_type = self.doxer_list_get_innermost_type()
>+      if list_type   == 'none':   self.mstream << '\n.TP\n'
>+      elif list_type == 'bullet': self.mstream << '\n.IP " *" 3\n'
>+      else:
>+        assert (False)                  # unsupported bullet type

this has to be the worst error message a user could possibly encounter (it doesn't even have a message about "unsupported functionality" or anything).
if the assert can possibly be triggered, it should be something like a SemanticError instead that helps users. if it cannot be triggered, there's no point in having it here.

>+      transformer (node, keyword_dict, NODE_TEXT)
>+    else:                               # doxer_deflist
>+      self.mstream << '\n.TP\n'
>+      transformer (node, keyword_dict)
>   def bold (self, tagname, transformer, node, keyword_dict):
>     self.mstream << r'\fB'
>     transformer (node, keyword_dict)
Comment 8 Stefan Westerfeld 2007-09-16 21:59:42 UTC
Created attachment 95689 [details] [review]
Doxer bullet lists for man pages patch, 4th attempt

Changes since last review:
* removed fatal error conditions (assert, raise) and replaced it by graceful failure (wrong document output with warning) at two places. It makes use of doxer_warn_if_fail() of which I have supplied a proper implementation - inclusion in the framework - in bug #437469. However, to make this independent and not a release blocker, I have supplied a one-liner here.

* preserve old list item opened state
Comment 9 Stefan Westerfeld 2007-09-16 22:05:01 UTC
(In reply to comment #8)
> [...] It makes use of doxer_warn_if_fail() [...]
It makes use of doxer_warn_if_reached(), actually. Also note that like g_assert_if_reached(), this is not for "error handling", that is, situations that could possibly occur ever, on any input file, as long as the program is correct. I use it for program correctness validation, that is, these can only be triggered (the statements can only be reached) if the program itself has a bug.
Comment 10 Stefan Westerfeld 2007-11-10 20:14:00 UTC
Created attachment 98888 [details] [review]
Same patch, better manpage

I've created a full manual page with markup from bsewavetool --help by hand. The rest of the patch is the same as the last one (adds the necessary bullet support to doxer). The code example at the end of the manual page is not as readable as it is with bsewavetool --help (see comments in .doxi file) the rest should be more readable due to markup.

I didn't change the text, so it is the same as in bsewavetool --help.
Comment 11 Stefan Westerfeld 2009-04-24 09:16:39 UTC
(In reply to comment #8)
> Changes since last review:
> * removed fatal error conditions (assert, raise) and replaced it by graceful
> failure (wrong document output with warning) at two places. It makes use of
> doxer_warn_if_fail() of which I have supplied a proper implementation -
> inclusion in the framework - in bug #437469.

I didn't mention the correct bug. Bug #477275 contains the proper implementation for inclusion in the framework.
Comment 12 Tim Janik 2010-04-06 10:51:49 UTC
(In reply to comment #10)
> Created an attachment (id=98888) [details] [review]
> Same patch, better manpage

Thanks, I think long term we should really move to a wiki based approach here and use the new Doxer. But for the upcoming Beast release, it'd be really good to have this in, so please add this patch to your staging tree and send me a merge request at some point.
Comment 13 Stefan Westerfeld 2010-06-10 12:25:46 UTC
Branch bug-437469 merged, closing the bug.