Skip to content

Conversation

Stephen-Seo
Copy link
Contributor

Before, adding a new track to the beat+bassline editor would have the default size regardless
of the width of other tracks.

This change fixes this, where the newly added track will have the
same width as existing ones.

This bug is mentioned in issue #2476 .

Before, adding a new track would have the default size regardless
of the width of other tracks.

This change fixes this, where the newly added track will have the
same width as existing ones.
@Stephen-Seo
Copy link
Contributor Author

I accidentally left in two "std::cout" debug prints in src/gui/editors/BBEditor.cpp .
On merge, it would be best to remove those lines as I don't think I can redo a pull request.

@Spekular
Copy link
Member

@Stephen-Seo just update your branch and the PR should update as well.

I don't feel qualified to review the code, but nice job adding this!

@tresf
Copy link
Member

tresf commented Jun 28, 2016

@Stephen-Seo just update your branch and the PR should update as well.

Correct, we don't modify code on merge, this will need to be fixed in your branch and as @Spekular stated, this PR will be updated to reflect your changes.

@Stephen-Seo
Copy link
Contributor Author

Ok, I removed the accidental debug prints. Hopefully everything else is ok.

Pattern* p = static_cast<Pattern *>( ( *it )->getTCO( m_bbtc->currentBB() ) );
steps = p->getSteps();
break;
}
}

std::cout << m_bbtc->currentBB() << std::endl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Stephen-Seo
Copy link
Contributor Author

I will fix new tracks in the BB for all BBs in the next commit to my fork.

As for why I'm using the signal/slot path, I need references to the other tracks (Patterns?) in the BB which is accessible from BBEditor to get their current size for the recently added track to have the same size.

The bug was that newly added tracks to the BeatBasslineEditor would not
have the same size as pre-existing tracks in the BBE.

Previous fix only worked for the current BeatBassline, not others.

This commit fixes this bug for all BBs.
@Stephen-Seo
Copy link
Contributor Author

Ok, the fix now works for all existing BBs.

My understanding is that increasing the size of a track in the BB requires a call to Pattern::addSteps().
I also need to know the size of existing Patterns with the added const function Pattern::getSteps().
I don't think I can do either from within the scope of a Track.

I think an alternate way of fixing this could be for "addSteps" and "removeSteps" in Pattern to actually call a "setSteps" which is called for each Pattern, ensuring they are the same size. This would also need to be called for newly added Patterns (they inherit from TCOs) which means the size also must be stored somewhere. Probably in the class that manages Patterns?

@Stephen-Seo
Copy link
Contributor Author

After a lot of thought, I'm second guessing the validity of my pull request.

It appears that the Patterns/TCOs are hooked up directly to the buttons that increase/decrease the size. A more involved fix could have something keep track of the size per BB in between the buttons and the Patterns/TCOs. Newly added Tracks would refer to this in-between object and would resize themselves, or the in-between object would keep track of newly added Tracks to resize them. This kind of fix may be better in terms of design.

Do you think I should have another go at it?

@jasp00
Copy link
Member

jasp00 commented Jun 30, 2016

It should be simpler. I am confident that this can be solved in the constructor of Pattern. m_steps can be set to the right value using information from _instrument_track. Have a look at trackContainer() and getTCOs().

@Stephen-Seo
Copy link
Contributor Author

As jasp00 suggested I made the fix in the constructor of Pattern. I'm closing this PR because I made the fix in another branch. Thus, I will make another PR with that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants