FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

Carl Sorensen
Dear Frogs,

Here's a possible task for a frog.  I think it should be possible to do in a
relatively short time.

Any takers?


Thanks,

Carl

------ Forwarded Message
From: <[hidden email]>
Date: Mon, 30 Nov 2009 05:49:25 -0700
To: <[hidden email]>
Conversation: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext
for all  appropriate contexts
Subject: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for
all  appropriate contexts

Status: Accepted
Owner: v.villenave
CC: Carl.D.Sorensen
Labels: Type-Enhancement Priority-Medium Usability

New issue 918 by v.villenave: Enhancement: \RemoveEmpty*StaffContext for
all appropriate contexts
http://code.google.com/p/lilypond/issues/detail?id=918

%% There are currently shortcuts for \RemoveEmptyStaffContext and
%% \RemoveEmptyRhythmicStaffContext, but not for other contexts such as
%% DrumStaff or TabStaff.

\version "2.13.8"

nul = { R1*4 \break }

\layout {
   \context {
     \RemoveEmptyStaffContext % exists
   }
   \context {
     \RemoveEmptyRhythmicStaffContext % exists
   \context {
    % \RemoveEmptyDrumStaffContext  % doesn't exist
   }
   \context {
    % \RemoveEmptyTabStaffContext % doesn't either.
   }
}

\score {<<
   \new Staff {
     c''1 c'' c'' c'' \break |
     \nul c''
   }
   \new RhythmicStaff {
     c''1 c'' c'' c'' \break |
     \nul c''1
   }
   \new DrumStaff \drummode {
     tomml1 tomml tomml tomml \break |
     \nul tomml
   }
   \new TabStaff {
     c'1 c c c \break |
     \nul c'1
   }
>> }

% A task for a Frog, perhaps? :)

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings

------ End of Forwarded Message


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

Ian Hulin
Hi Carl,

Carl Sorensen wrote:
Dear Frogs,

Here's a possible task for a frog.  I think it should be possible to do in a
relatively short time.

Any takers?


Thanks,

Carl

  
Patch for review on Rietveld  http://codereview.appspot.com/164045

Cheers,
Ian

------ Forwarded Message
From: [hidden email]
Date: Mon, 30 Nov 2009 05:49:25 -0700
To: [hidden email]
Conversation: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext
for all  appropriate contexts
Subject: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for
all  appropriate contexts

Status: Accepted
Owner: v.villenave
CC: Carl.D.Sorensen
Labels: Type-Enhancement Priority-Medium Usability

New issue 918 by v.villenave: Enhancement: \RemoveEmpty*StaffContext for
all appropriate contexts
http://code.google.com/p/lilypond/issues/detail?id=918

%% There are currently shortcuts for \RemoveEmptyStaffContext and
%% \RemoveEmptyRhythmicStaffContext, but not for other contexts such as
%% DrumStaff or TabStaff.

\version "2.13.8"

nul = { R1*4 \break }

\layout {
   \context {
     \RemoveEmptyStaffContext % exists
   }
   \context {
     \RemoveEmptyRhythmicStaffContext % exists
   \context {
    % \RemoveEmptyDrumStaffContext  % doesn't exist
   }
   \context {
    % \RemoveEmptyTabStaffContext % doesn't either.
   }
}

\score {<<
   \new Staff {
     c''1 c'' c'' c'' \break |
     \nul c''
   }
   \new RhythmicStaff {
     c''1 c'' c'' c'' \break |
     \nul c''1
   }
   \new DrumStaff \drummode {
     tomml1 tomml tomml tomml \break |
     \nul tomml
   }
   \new TabStaff {
     c'1 c c c \break |
     \nul c'1
   }
  
}
      

% A task for a Frog, perhaps? :)

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings

------ End of Forwarded Message


---
----
Join the Frogs!


______________________________________________        
This email has been scanned by Netintelligence        
http://www.netintelligence.com/email


  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

Valentin Villenave
Administrator
On Mon, Nov 30, 2009 at 11:04 PM, Ian Hulin <[hidden email]> wrote:
> Patch for review on Rietveld  http://codereview.appspot.com/164045

Don't forget to document the new shortcuts, and to add an item to
changes.tely :)

Cheers,
Valentin

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

Carl Sorensen
Thanks, Ian.

The patch looks good to me as far as it goes.

On 11/30/09 4:31 PM, "Valentin Villenave" <[hidden email]> wrote:

> On Mon, Nov 30, 2009 at 11:04 PM, Ian Hulin <[hidden email]> wrote:
>> Patch for review on Rietveld  http://codereview.appspot.com/164045
>
> Don't forget to document the new shortcuts, and to add an item to
> changes.tely :)

Strictly speaking, there's not a requirement for developers to write their
own documentation (although we appreciate it).

However, there *is* a requirement for developers to write their own
regtests.  I didn't see any regtests (either new or modified) in the patch.

As far as changes.tely -- that's not listed in the CG.  So either we don't
need to edit it, or we need to change the CG.  The CG references NEWS, but
NEWS doesn't exist any more, so I'll change the CG.

Thanks,

Carl


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

Ian Hulin
Hi Carl,

Your last message came through in stereo stereo. . .

Carl Sorensen wrote:
Thanks, Ian.

The patch looks good to me as far as it goes.

On 11/30/09 4:31 PM, "Valentin Villenave" [hidden email] wrote:

  
On Mon, Nov 30, 2009 at 11:04 PM, Ian Hulin [hidden email] wrote:
    
Patch for review on Rietveld� http://codereview.appspot.com/164045
      
Don't forget to document the new shortcuts, and to add an item to
changes.tely :)
    

Strictly speaking, there's not a requirement for developers to write their
own documentation (although we appreciate it).

However, there *is* a requirement for developers to write their own
regtests.  I didn't see any regtests (either new or modified) in the patch.
  
That's because there don't seem to be any specifically for these functions.  There are two regression tests in which RemoveEmptyStaffContext is used:
 hara-kiri-percent-repeat.ly and hara-kiri-pianostaff.ly.
Nothing tests RemoveRhythmicStaffContext
I can add  DrumStaff, TabStaff and RhythmicStaff  and the corresponding RemoveEmpty*StaffContext statements quite easily in 
hara-kiri-percent-repeat.ly, but I'll need to add three new tests, hara-kiri-drums.ly, hara-kiri-rhythmicstaves.ly and hara-kiri-tabs.ly

Cheers,
Ian
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

Carl Sorensen



On 12/1/09 5:07 PM, "Ian Hulin" <[hidden email]> wrote:

> Hi Carl,
>
> Your last message came through in stereo stereo. . .
>
> Carl Sorensen wrote:
>>  
>> Thanks, Ian.
>>
>> The patch looks good to me as far as it goes.
>>
>> On 11/30/09 4:31 PM, "Valentin Villenave" <[hidden email]>
>> <mailto:[hidden email]>  wrote:
>>
>>  
>>  
>>>  
>>> On Mon, Nov 30, 2009 at 11:04 PM, Ian Hulin <[hidden email]>
>>> <mailto:[hidden email]>  wrote:
>>>    
>>>  
>>>>  
>>>> Patch for review on Rietveld� http://codereview.appspot.com/164045
>>>>      
>>>>  
>>>  
>>> Don't forget to document the new shortcuts, and to add an item to
>>> changes.tely :)
>>>    
>>>  
>>  
>>
>> Strictly speaking, there's not a requirement for developers to write their
>> own documentation (although we appreciate it).
>>
>> However, there *is* a requirement for developers to write their own
>> regtests.  I didn't see any regtests (either new or modified) in the patch.
>>  
> That's because there don't seem to be any specifically for these functions. 
> There are two regression tests in which RemoveEmptyStaffContext is used:
>  hara-kiri-percent-repeat.ly and hara-kiri-pianostaff.ly.
> Nothing tests RemoveRhythmicStaffContext
> I can add  DrumStaff, TabStaff and RhythmicStaff  and the corresponding
> RemoveEmpty*StaffContext statements quite easily in 
> hara-kiri-percent-repeat.ly, but I'll need to add three new tests,
> hara-kiri-drums.ly, hara-kiri-rhythmicstaves.ly and hara-kiri-tabs.ly
>

How about just writing a new regtest remove-empty-staff-context.ly that
demonstrates the removal of all of the staffs?

Thanks,

Carl


> Cheers,
> Ian

&������,
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FW: Issue 918 in lilypond: Enhancement: \RemoveEmpty*StaffContext for all appropriate contexts

Ian Hulin
Hi Carl,

Carl Sorensen wrote:

On 12/1/09 5:07 PM, "Ian Hulin" [hidden email] wrote:

  
Hi Carl,

Your last message came through in stereo stereo. . .

Carl Sorensen wrote:
    
 
Thanks, Ian.

The patch looks good to me as far as it goes.

On 11/30/09 4:31 PM, "Valentin Villenave" [hidden email]
[hidden email]  wrote:

  
 
      
 
On Mon, Nov 30, 2009 at 11:04 PM, Ian Hulin [hidden email]
[hidden email]  wrote:
    
 
        
 
Patch for review on Rietveld??? http://codereview.appspot.com/164045
      
 
          
 
Don't forget to document the new shortcuts, and to add an item to
changes.tely :)
    
 
        
 

Strictly speaking, there's not a requirement for developers to write their
own documentation (although we appreciate it).

However, there *is* a requirement for developers to write their own
regtests.  I didn't see any regtests (either new or modified) in the patch.
  
      
That's because there don't seem to be any specifically for these functions.??
There are two regression tests in which RemoveEmptyStaffContext is used:
??hara-kiri-percent-repeat.ly and hara-kiri-pianostaff.ly.
Nothing tests RemoveRhythmicStaffContext
I can add?? DrumStaff, TabStaff and RhythmicStaff?? and the corresponding
RemoveEmpty*StaffContext statements quite easily in??
hara-kiri-percent-repeat.ly, but I'll need to add three new tests,
hara-kiri-drums.ly, hara-kiri-rhythmicstaves.ly and hara-kiri-tabs.ly

    

How about just writing a new regtest remove-empty-staff-context.ly that
demonstrates the removal of all of the staffs?

  
Drat! By the time I got your mail I'd already done the mods and updated them for review.  The tests I've written all show the \RemoveEmpty*Context functions doing their stuff, and the hara-kiri-percent-repeat one shows they all respond the same way to the \repeat n percent command.

It would have been neater to go with your suggestion but I thought the reg tests had to test a single thing per file.

They're up on Rietveld http://codereview.appspot.com/164045/show for review.

Cheers,

Ian


Loading...