Re: [Audacity-devel] I pulled three of Pokechu22's 12 commits...

[Pokechu22 <pok...@gm...>]

First, thanks for the partial merge; those are some of the more useful
base commits.

However... 5badb91 shouldn't have been needed.  I didn't add that
call; it's been there since 2010 according to git blame (and none of
the pulled commits modified UpdateGain).  I don't know how that would
have worked before my changes, weird.

Assuming that the link error you're referring to is due to a missing
TrackInfo::GainSlider(int) (and not due to PortMidi not existing), the
fix for it occurs in the huge TrackPanel commit (69b30e2), and more or
less has to occur there unfortunately.

To explain what's going on here (since the need for a slider isn't
obvious if you haven't gotten it to work), here's a quick explanation
of the more subtle EXPERIMENTAL_MIDI_OUT changes.

Note tracks gain 2 (technically 1) new properties.  Here's a
screenshot: http://i.imgur.com/OofBNcm.png.  The more obvious one is
the toggle for individual channels, which actually exists even without
EXPERIMENTAL_MIDI_OUT, in a broken way - on master, you can toggle
channels by clicking, even though the buttons aren't visible.
Enabling EXPERIMENTAL_MIDI_OUT lets you see the buttons.

But that's not not the one that causes link problems.  There's also a
slider, which is used to control the volume of note tracks.  For wave
tracks, this slider controls gain; for note tracks, it controls
relative velocity of notes (mVelocity).  To be confusing, the getters
and setters for that called it Gain (perhaps to be the same as wave
tracks), but the ranges on it are very different - gain on wave tracks
ranges from -36 dB to +36 dB, while velocity goes from -50
(http://i.imgur.com/zuWEFXN.png) to +50
(http://i.imgur.com/1yRxHp9.png).  (Actual velocity in midi files
ranges from 0 to 127 if I recall correctly).  There's also a different
style for the slider.

In the past, both the gain slider for wave tracks and the velocity
slider for note tracks were put in an array, and GainSlider(int) was
used to index that array.  It then changed the style of the slider to
a gain or velocity slider (which changed the range and the tooltip) as
needed.  Oh, and it generated and stored a bogus offset rectangle
because GainSlider used a different location than the velocity slider
needed, which makes hit tests complicated.

The array was removed in 8f773342 because it really didn't help
anything, and GainSlider(int) was replaced with a variant that took a
Track and generated a new slider.  However, that only was done for
wave tracks; the velocity slider code wasn't touched (and still used
SetStyle).  Far later, in b28ec295, GainSlider(int) was re-declared
(but not defined) to get EXPERIMENTAL_MIDI_OUT to compile (but not
link).  What _should_ have happened was a dedicated
VelocitySlider(NoteTrack*) method, but that didn't happen.

So, to get everything to work, either a horribly hacky
reimplementation of GainSlider(int) needs to be re-added (including
the offset rectangle), or dedicated VelocitySlider code needs to be
created.  I chose the latter, for obvious reasons.  That does mean a
lot of changes to TrackPanel, but it's needed to get things to compile
in a way that isn't a total hack.

Unfortunately it's not something that can be easily isolated and
cherry-picked; if it were I'd have done it that way.

--Poke

On Mon, Mar 27, 2017 at 1:44 AM, Paul Licameli <paul.licameli@...> wrote:
> Also I committed 5badb91 which was needed to complete compilation fixes for
> fix-midi-output on Mac, though I am not sure if the fix is correct.
>
> With those changes I can compile and link fix-midi-output; whereas, I can
> compile but not link master with EXPERIMENTAL_MIDI_OUT.
>
> I want to figure out where the fix for linkage happens in the
> fix-midi-output branch and cherry-pick that part of it onto master too.
>
> PRL

I'm glad that the individual commits are useful; it's a good habit
I've gotten into.  (I do squash fixes into one commit, but I try to
keep the changes mostly broken down).

The naming actually was correct... when the commit was made.  But it
depended on a commit before that changed the names from r to rect (in
addition to some other changes there, such as making the method return
void), which don't work in isolation.  Leaving the return type as int
and keeping the name as 'r' is correct for the commit in isolation,
and I've already rebased to apply the renaming afterwards.

--Poke

On Mon, Mar 27, 2017 at 1:31 AM, Paul Licameli <paul.licameli@...> wrote:
> ... but with some amandment of one of them.
>
> These are commits for fixing conditional compilations and preferring
> wxASSERT to assert.
>
> In one place I made compilation fixes by making arguments const-reference
> instead of value.  I found the commit did not completely fix the
> compilation for me because a variable was still named wrong.
>
> Thank you for breaking your work into many little commits, each making a
> sensible step by itself that is meant to keep the project in some
> meaningful state.  I strive to do similar with big projects.
>
> PRL