Re: PATCH: Refactor script-column.cc for improved reading and fewer lines

[Carl Sorensen]

On 12/4/09 9:24 AM, "David Kastrup" <address@hidden> wrote:

> Carl Sorensen <address@hidden> writes:
> 
>> Following up on David's comments, I've refactored script-column.cc.
>> 
>> I've also added a combination of Script and TextScript objects to the
>> script-column, to ensure that the two priorities work together properly.
>> 
>> Please review the code at
>> 
>> http://codereview.appspot.com/166057
> 
> I have a very hard time picking the code apart.  default_outside_staff
> and last_outside_staff are used somewhat interchangeably, likely one of
> the two should go.

No.  Default_outside_staff is the incoming (default) outside_staff_priority
of the previous grob.  Last_outside_staff is the outside_staff priority of
the previous grob *after* it's been adjusted to reflect the script_priority
differences.

> 
> What effects on the priorities does the
> Side_position_interface::add_support function have, if any?

None, AFAICS.  It adds an unordered grob to a Pointer_group_interface (see
lily/side-position-interface.cc).


> 
> Could you describe in simple words what the behavior is supposed to
> achieve?  If you do that, I promise to submit some simple code that does
> that.
> 

Take three text_scripts, all with outside_staff_priority of 450, and with
script_priorities of 201, 202, 203.

Convert them to three text_scripts with outside_staff_priorities of 450,
451, 452, so that the script_priority is moved to outside_staff priority.



> I've unwrapped a bit of code of the form
> 
> for (a;b;c) {
>   if (condition)
>     short something
>   else {
>     Large something
>   }
>   z
> }
> 
> into
> 
> for (a;b;z,c) {
>   if (condition) {
>     short something
>     continue;
>   }
>   Large something
> }
> 
> which helps a bit keeping track of what happens where.  But the flurry
> of variables and tests used does not really make a picture that is
> coherent enough for me to be sure I get the idea behind the code.
> 
> If you could list the cases that should be handled and the exceptions,
> that would help.  After all, this should likely boil down to something
> which can be explained in a few sentences rather than "Some magic
> involving the following properties happens".  So that people have a
> chance to manipulate the properties and get predictable results.
> 

Sort the script-column list by script_priority (lowest first)

Loop through all script objects in the script column list:

For each item in script column:

If it's the first object, set the default_outside_staff_priority to this
object's script priority

Else
    Check the previous object's current outside_staff_priority.
    If it's not a number, use Side_position_interface::add_support to create
the appropriate relationship between the previous grob and the current grob.
    If it is a number, what we do depends on the outside_staff_priority of
the current grob and the default_outside_staff_priority of the previous grob
     If there is no oustide_staff_priority, then set the
outside_staff_priority to the previous grob's outside_staff_priority plus a
small delta (currently 0.1).
     If the two priorities are different, then the ordering will be right if
we just use the outside_staff_priority, so we don't need to do anything
special.
     If the two priorities are the same, then set the current grob's
outside_staff_priority to (previous grob's outside_staff_priority) +
(current grob's script_priority) - (previous grob's script_priority).  This
maintains the same relative difference in priorities when we change the
controlling priority from script_priority to outside_staff_priority.

Note: It *may* work that we could just set the current grob's
outside_staff_priority to the previous grob's outside_staff_priority plus a
small delta (0.1), just as we do if there is no outside_staff_priority for
the current grob.  This would simplify the code, but it would change the
amount of relative priority difference (which may not matter at all).

We then need to store the current grob's unadjusted outside_staff_priority
as the default_outside_staff_priority.

Thanks for your feedback,

Carl