T437 arrow key and del key functionality on selected track event #107

Merged
merged 9 commits into from Mar 30, 2012

2 participants

@ScottDowne

No description provided.

@secretrobotron

Let's declare this inside of the butter core file (src/butter-main.js) so it's a centralized concept.

@secretrobotron

selectedEvents.length can be cached for some free speed here (although small).

@secretrobotron

Why this check? Can nulls exist in the array?

@secretrobotron

No magic numbers, please. Just slap 'em in upper-case vars at the top of this file.

@secretrobotron

Especially because the moveFrameLeft (etc) function is in this module and this file, instead of using the butter namespace to call these, just cache the function in a var somewhere or declare a that = this for the same shortcut.

@secretrobotron

Magic numbers again.

@secretrobotron

Since you were worried a bit about perf, this loop and the loop below can probably benefit from caching selectedEvents here.

@secretrobotron
Mozilla member

I'm curious: is there a reason all of this code is in the timeline module? We it just because there was already a keypress event here?

Maybe it smarter to put this in the ui module.

@secretrobotron

You probably need to clear the selectedEvents array here, no?

@secretrobotron

Why are you checking for null as the first condition? Can these turn null somehow?

@secretrobotron

Think this is faster than just doing a push, sort?

@secretrobotron

How about keeping the index in a var instead of finding the event in this array each time? You can just keep that index updated when you add/remove a trackevent then.

Mozilla member

...or change it with this functionality, of course.

@secretrobotron
Mozilla member

It might make sense to put a bunch of this code right inside the TrackEvent class, since it owns cornOptions, and since it might be useful for people that use trackevents otherwise. Really, it's just the namespace that bothers me: butter.ui

@secretrobotron secretrobotron commented on the diff Mar 29, 2012
src/butter-main.js
@@ -45,7 +45,8 @@
icons: {}
},
_defaultTarget,
- _this = this;
+ _this = this
@secretrobotron
Mozilla member

missing comma!!! GLOBAL LEAK!!

This was a test. You passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@secretrobotron secretrobotron commented on the diff Mar 29, 2012
src/butter-main.js
@@ -405,6 +406,15 @@
} //if
},
enumerable: true
+ },
+ selectedEvents: {
@secretrobotron
Mozilla member

drop an "enumerable: true" in here so that we can see the property in firebug, etc.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@secretrobotron secretrobotron and 1 other commented on an outdated diff Mar 29, 2012
src/butter-main.js
@@ -405,6 +406,15 @@
} //if
},
enumerable: true
+ },
+ selectedEvents: {
+ get: function() {
+ return _selectedEvents;
+ },
+ set: function(selectedEvents) {
+ _selectedEvents = selectedEvents;
+ return _selectedEvents;
@secretrobotron
Mozilla member

I'm ... faaaairly certain returning anything here has no effect. Right?

I'm pretty sure I agree, but I was following the setter inside currentTime above it. I have seen returns in setter before, and I think it is to allow code like this:

var item = obj.item = 4;

With obj.item being a setter, but also returning the value? I will test this and get back to you.

Nope, not needed.

It knows to call the getter and setter when used like the above. I will remove the return.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@secretrobotron secretrobotron and 1 other commented on an outdated diff Mar 29, 2012
src/core/track.js
@@ -137,7 +137,6 @@ define( [
_trackEvents.target = _target;
} //if
_trackEvents.push( trackEvent );
- trackEvent.track = _this;
@secretrobotron
Mozilla member

Why did this get removed, actually?

Because I see the same line again 8 lines down. I actually removed that out of curiosity to see if both lines were needed, with intending to file it, but completely forgot.

I will ping you on irc and we can see if it is needed, and see if we should file it. Good catch though! I forgot I did that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@secretrobotron secretrobotron merged commit 65dc717 into mozilla:master Mar 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment