Code review comment for lp:~mixxxdevelopers/mixxx/features_softtakeover

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Looks good, thanks Sean

Minor nitpick in midiscriptengine.cpp

 if(cot != NULL) {
- cot->slotSet(newValue);
+ if (!m_st.ignore(group,name,newValue)) cot->slotSet(newValue);
     }

You should probably fold the two if statements into one.

Also one line if statements, while they are concise are often problematic 1) because it's easy to overlook that it's even there and 2) because there are no braces to protect against someone adding another statement mistakenly. I have a slight preference to not have one-line or brace-less if's anywhere in Mixxx if possible since they can be a source of bugs.

Go ahead and merge to trunk -- we haven't been strict at all about the feature freeze this time around and this only affects controllers with it explicitly enabled so I don't see a problem.

thanks again,
rj

review: Approve

« Back to merge proposal