For review: Term display helper

[Fergus Henderson]

On 27-Feb-1998, Waye-Ian CHIEW <wchi at students.cs.mu.OZ.AU> wrote:
> Hello.
> 
> This is the term display helper, which takes univ terms and displays them in a
> choice of pretty formats.
> 
> It is not finished and not really useable;  some critical additions, like
> the scripting language and the interactive browser, haven't been implemented
> yet. 
> 
> -- Ian!!

Uh, the scripting language?!?

Does it make the coffee too?  How about a builtin mail reader? ;-)

> # ============= FRAMES ==============

> FRAMES
>
>     CONCEPTS

This file would benefit from some introductory text explaining
the context; e.g. "This file documents ... <something> ...
whose intended purpose is ... <something> ...
The stuff documented here is implemented in file <foo>.m."

What you have in this file looks OK, although it seems like
an ambitious design -- it might be better to scale back
to something a little simpler, at least for a first implementation.

> # ============= SPECS ==============
...
> X      Each node of the visibility tree shows:
> X      -  The format in which a term is to be displayed in.
> X      -  Whether the term will be displayed collapsed or expanded.

I think it should be more than just a binary choice.
The user might want to limit display to a certain depth, or
a certain number of characters, for example.
Does the current design allow for this?

> INTERFACE
> X
> X   The display helper will be a Mercury library.
> X
> X      portray(Term, Visible, Prefs, Output, State0, State).
> X      portray(term::in, visible::in, preference::in,
> X	      io__output_stream::in, io__state::di, io__state::uo) is det.
> X
> X   Displays a term noninteractively.
> X
> X      browse(Term, Visible0, Visible, Prefs0, Prefs, State0, State).  
> X      browse(term::in, visible::in, visible::out, 
> X	     preference::in, preference::out, 
> X	     io__state::di, io__state::uo) is det.
> X
> X   Invokes the interactive browser. 
> X
> X   'Term' is the term to be displayed. 
> X
> X   'Visible' is the visibility tree.  The interactive browser also
> X   returns a user-modified visibility tree.  The caller must supply
> X   and store the visibility tree.  
> X   
> X   'Prefs' is the user preference map.  The interactive browser also
> X   returns user-modified user preferences.  The caller must supply and
> X   store the preference map.
> X   
> X   'Output' is the output text stream.  The browser does not need this
> X   argument -- it always reads user input from standard input and
> X   writer to standard output.

There should be some way of getting a default visibility tree
and a default user preference map.  (Oh, I see later on that
the interface does actually include these.  But they're not
mentioned here.)

It would be convenient to supply versions of portray and browse
that use these defaults.

> DISPLAY SCRIPTS
> X
> X   A simple scripting language can be attached to any visibility tree
> X   node.  It's a user-definable way of fine-controlling the display of
> X   data types.

Ah.  Now I understand why you want a scripting language.

An alternative approach would be to provide some Mercury-level
hook which the user could use to control visibility of
a particular type, e.g. using typeclasses.

I guess this doesn't work yet because we don't have a good way
of doing these sort of hooks in Mercury.  Typeclasses aren't quite
enough, we also need dynamic type-class casts (overlapping instance
declarations would be another way of achieving the same effect, but
we don't have them either).

Still, I think it would probably be best to leave off the scripting
language for the moment.

> X   For example:
> X   
> X      A list type:
> X
> X         :- type list(T)        --->
> X                        cons(T, list(T))
> X                ;       nil.
> X                
> X      
> X         cons   -> block([subterm(show([1])), show([2])]).
> X
> X         nil    -> nop.

What would the output be?

> X      A tree-based map:

Ditto.

> INTERACTIVE COMMAND SET
> X
> X   The interactive browser is intended to be a way of setting user
> X   preferences.
> X
> X   The browser loops and accepts a small set of user commands from
> X   input.  It calls 'portray' display terms.

s/display/to display/

> X   + <path>
> X   expand <path>
> X      Temporarily collapses the term at <path>.
> X
> X   - <path>
> X   collapse <path>
> X      Temporarily expands the term at <path>.  

I think you have your "collapses" and "expands" mixed up.

Also, does expand expand just one level, or does it expand recursively?
(I presume the former, but it is not clear.)

> X   cd <path>
> X      Changes the current path to <path>.  

I'm not sure that it would be a good idea to reuse "cd" as the name
for this command.  Something else might be a better idea.
Perhaps "goto".

> X   chroot <path>
> X      Changes the root of the term tree to <path>. 
> X
> X   attach <path> <script>
> X      Attaches the display script <script> to the visibility tree node
> X      of the term at path <path>.
> X      
> X   quit
> X      Quits and returns the user-modified visibility tree and
> X      preferences to the caller.
> X      
> X   quit! 
> X      Quits and returns the original visibility tree and user
> X      preferences to the caller.

It would be better if there were also short versions of some of those
commands (e.g. `q' and `q!' for `quit' and `quit!').

> %  File:  frame.m
> %
> %  This module implements text frames, a way of organising structured text on
> %  screen using nested rectangular frames.  It is an implementation of only a
> %  small subset of the frames specification.
> %  It defines a central data type, frame, and a set of low-level functions to
> %  convert them into strings ready for output.

I suggest you add a pointer to the FRAMES file,
e.g. "See the file FRAMES for further documentation.".

> :- type frame --->
> X		whitespace
> X	;	filler(char)

Some comments for these, particularly `filler', would be helpful.

> X	%  The alignment of a frame inside its parent, horizontally or
> X	%  vertically.  'least' is toward the top-left corner;
> X	%  'most' is toward the bottom-right corner.
> :- type frame__alignment --->
> X		least
> X	;	center
> X	;	most
> X	.

I think `top_right' and `bottom_left' would be better names.

> X	%  frame__measure(Frame) returns the size of Frame, in characters.
> :- func frame__measure(frame) = frame__size.
> :- mode frame__measure(in) = out is det.

What does this mean for multi-line frames?
Does this count newlines?

> frame__rasterise(clipping(Child, size(Clipping_width, Clipping_height)), 
> X		size(Space_width, Space_height)) = Stringlist :-
> X	Unpadded_stringlist = frame__rasterise(Child, 
> X		size(Clipping_width, Clipping_height)),
> X	Stringlist = frame__pad_lower_right(Unpadded_stringlist,
> X		size(Space_width, Space_height)).

I think this code would be much easier to read if you did some
common sub-expression elimination on the sizes:

  frame__rasterise(clipping(Child, Clipping_size), Space_size) = Stringlist :-
	Unpadded_stringlist = frame__rasterise(Child, Clipping_size),
	Stringlist = frame__pad_lower_right(Unpadded_stringlist, Space_size).

The same comment applies in lots of other places.

In places where you do need to deal with the height and width explicitly,
then it might even be clearer to use `height' and `width' functions
(I'm not sure about this, but it may be worth considering).

> %  File:  frames.m

I think it is a bit confusing to have two different modules
call `frame' and `frames'.  I would prefer it if you renamed
one of them.

> frames__border(Child, Direction, Least_corner_char, Border_char, Most_corner_char)

This line is >79 columns.  Please wrap it.

> X		= Frame :-
> X	(
> X		(Direction = left ; Direction = right) ->
> X		Border_direction = down,
> X		Anti_border_direction = up
> X	;
> X		Border_direction = right,
> X		Anti_border_direction = left
> X	),

The layout of this if-then-else is a bit strange.
Please follow the layout suggested in the Mercury developers
coding guidelines.

> :- module portray.

> :- pred portray(...

The name `portray' has an existing meaning in Prolog,
as the "hook" predicate used by `print'.
The current plan is to eventually make it mean the same thing in Mercury.
(Although arguably it might be better to use `print_hook';
that would allow the use of a consistent convention of
using `foo_hook' as the hook predicate for `foo'.)

So it might be better to use a name other than `portray'.
Unfortunately most of the good ones -- e.g. `print', `write', `display',
and `format' -- are already taken.  I guess you could use `show'.  Hmm...  

I guess it might eventually be a good idea to use this code for
`io__print', so maybe you could call it `print'.

Anyone else have any suggestions/comments about this naming issue?

> :- module portray_test.
> main -->
> X	{ 
> X		L0 = ["March"		- 3,
> X		      "September"	- 9,
> X		      "May"		- 5,
> X		      "February" 	- 2,
> X		      "November"	- 11,	 
> X		      "January" 	- 1,
> X		      "June"		- 6,
> X		      "December"	- 12,
> X		      "July"		- 7,
> X		      "April"		- 4,
> X		      "August"		- 8,
> X		      "October"		- 10],
> X		pair_unzip(L0) = L1 - L2,
> X		map__init(T0),
> X		map__det_insert_from_corresponding_lists(T0, L1, L2, T)

This is only a test case, so it doesn't really matter a lot, but
you could just use `map__from_assoc_list(L0, T)'.
	
> %  File:  term_interface.m
> %
> %  This module implements 

Implements what?
You need to document this module and the predicates it exports.

> term_interface__builtin(Univ) :-
> X	term__univ_to_term(Univ, Term),
> X	Term = term__functor(Const, _, _),
> X	( Const = term__integer(_) ;
> X	  Const = term__float(_) ;
> X	  Const = term__string(_) ).

This is not an adequate way of testing for builtins.
int, float, and string are not the only builtins; others include

	character
	array(T)
	c_pointer
	io__state
	univ
	type_info

--------------------------------------------------

OK, so there is some work to do still.
But nevertheless, that looks like an excellent start!

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.