Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix lost control links in copied automation patterns #4723

Merged
merged 2 commits into from Dec 22, 2018

Conversation

Projects
None yet
5 participants
@PhysSong
Copy link
Member

PhysSong commented Dec 9, 2018

As I said in #3781, I decided to work around the bug temporarily, at least in stable-1.2. I believe we can fix underlying issues as well, but the required change is too large for stable releases.
I think the ultimate solution is to decouple journaling and automation(see #3781 (comment)). I'll create a new issue to keep track on it once this PR gets merged.

Fixes #3781, fixes #3359.

Fix lost control links in copied automation patterns
This hack should be removed once the automation system gets fixed.
{
// FIXME: Remove this block once the automation system gets fixed
// This is a temporary fix for https://github.com/LMMS/lmms/issues/3781
o = Engine::projectJournal()->journallingObject( *k + ( 1 << 23 ) );

This comment has been minimized.

@jasp00

jasp00 Dec 11, 2018

Member

It is dangerous to mess with ProjectJournal internals like this. You should request new ids and copy data if necessary.

This comment has been minimized.

@jasp00

jasp00 Dec 11, 2018

Member

I see, you are probing ids. Anyways, use the ProjectJournal.

This comment has been minimized.

@PhysSong

PhysSong Dec 11, 2018

Author Member

I also think this is a bad design, but this is the minimal fix for the bug which doesn't involve possibly dangerous structure changes for stable releases.
I want to remove this one in master as soon as I get my work completed.

This comment has been minimized.

@jasp00

jasp00 Dec 11, 2018

Member

Please make it less minimal. Do not expose EO_ID_MSB.

This comment has been minimized.

@PhysSong

PhysSong Dec 12, 2018

Author Member

I didn't want to do so. However, changing ProjectJournal may affect other stuff than automation. I can address this, but it will make the temporary fix much complicated.

This comment has been minimized.

@PhysSong

PhysSong Dec 19, 2018

Author Member

Okay. BTW, @jasp00, please note that you will probably make double work. I already have a fix with new designs and what I need to do is just cleaning them up.

This comment has been minimized.

@DomClark

DomClark Dec 19, 2018

Member

I think it makes more sense to argue using comments, rather than to argue using commits. We can merge or close once we've reached a consensus on what the code should look like.
Is the desired change something as simple as, e.g.

jo_id_t ProjectJournal::idFromSave( jo_id_t id )
{
	return id | EO_ID_MSB;
}

?

This comment has been minimized.

@PhysSong

PhysSong Dec 20, 2018

Author Member

I'm not sure if we need that, but that could be a solution.

This comment has been minimized.

@PhysSong

PhysSong Dec 21, 2018

Author Member

@jasp00 If you're comfortable with the @DomClark's suggestion, I will use that for now.

This comment has been minimized.

@jasp00
@qnebra

This comment has been minimized.

Copy link

qnebra commented Dec 11, 2018

Possible regression with projects which single kind of parametr from multiple tracks connected to one automation track. For further checking.

@PhysSong PhysSong merged commit e1d9d89 into LMMS:stable-1.2 Dec 22, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PhysSong PhysSong deleted the PhysSong:autocopy-tmp branch Dec 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.