Re: [jokosher-devel] [PATCH] Patch for Launchpad bug 85938 - Usability for selection-based editing

[Laszlo Pandy <laszlok2 gmail com>]

Sorry, I haven't been ignoring this I am just really busy. There is also 
a few bugs in my new levels data code which is causing exceptions in the 
fade point function. I want to make sure I fix those before I commit 
this patch because they are making it hard to test everything entirely, 
and I don't want to blame your patch for something I broke. :)

I have a few comments below, but they are just minor things. Don't worry 
about creating a new patch, I will change them when I commit it. If I 
have any other comments later on I will let you know.

Tom Halligan wrote:
> Woops silly me - I have indeed been using Eclipse, with some third-party 
> SVN plugin. I just assumed it was doing things right - obviously not!
>
> Anyway, here's a new patch with your suggestions duly noted and taken 
> care of:
> 1) (I assume you meant EventLaneViewer.py for this - EventViewer does 
> indeed have a lot of changes) No, nothing changed in EventLaneViewer.py, 
> must have knocked return by mistake or something :P
>
> 2) Fixed!
>
> 3) I called it DummySplitEvent because it 'emulates' splitting, if you 
> see what I mean. But you're right, it was a bad name, now it's called 
> 'CopySelection'.

I like that name. :)

> 4) Done - now works rather well if I do say so myself :P I also included 
> a few try...excepts in __UpdateFadeLevels, as I kept receiving 
> exceptions and broken functionality caused by the iterFadePoints.next() 
> bits when using the new copy functionality. It should work fine now :)

Every python programmer should know that next() will throw a 
StopIteration exception when there are no more elements left. If an 
exception is thrown here it means the timestamps on the level data does 
not match the length of the track. Basically it means something screwed 
up somewhere. And for testers, throwing an exception is really annoying 
and makes them complain. Just last week Elleo complained about loading 
not working after a split, because there was a StopIteration exception.

Certainly it is a very good idea to catch this exception, and I plan to 
put it in before release. But for right now I'm not gonna catch the 
exceptions because it will make these bugs go undetected. I'm pretty 
sure the exceptions you were getting is related to how you said this 
patch is cosmetic and doesn't effect the audio. Something is inconsistent.

If you feel like working on this go ahead. But if not don't worry I make 
sure it works.

> Like I said in the last post - these things only work when you 
> shift-select on an event, then right click. They don't currently work 
> from the Edit menu.
>
> Tom

Yeah maybe you would like to work on that edit menu too. Feel free to 
discuss here or on IRC, we might be able to reduce the code duplication 
in there.

Thanks again for the patch,
Laszlo