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

"misc" view now shows the model state, of the track use of master pitch #3753

Merged
merged 3 commits into from Aug 15, 2017

Conversation

Projects
None yet
5 participants
@serdnab
Contributor

serdnab commented Aug 6, 2017

Fix for #2612.
When an already created instance of InstrumentTrackWindow is used, the master pitch GroupBox isn't refreshed.
Now, if this is the case, when modelChanged() is invoked, the refresh is done.

@PhysSong PhysSong requested a review from zonkmachine Aug 6, 2017

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 6, 2017

Member

Good observation! I haven't tested it yet, but It should be work.

Member

PhysSong commented Aug 6, 2017

Good observation! I haven't tested it yet, but It should be work.

@gi0e5b06

This comment has been minimized.

Show comment
Hide comment
@gi0e5b06

gi0e5b06 Aug 6, 2017

getPitchGroupBox() should be renamed pitchGroupBox(), according to Qt and Lmms coventions.
Use the read accessor and not directly the field:
m_miscView->getPitchGroupBox()->setModel(&m_track->m_useMasterPitchModel);
should be:
m_miscView->pitchGroupBox()->setModel(m_track.useMasterPitchModel());
wgich requires to have the models mapped as properties, like volume.

BTW, thanks for the fix. If you spend a bit more time on refactoring the models/properties for the Track class, that would be great.

gi0e5b06 commented Aug 6, 2017

getPitchGroupBox() should be renamed pitchGroupBox(), according to Qt and Lmms coventions.
Use the read accessor and not directly the field:
m_miscView->getPitchGroupBox()->setModel(&m_track->m_useMasterPitchModel);
should be:
m_miscView->pitchGroupBox()->setModel(m_track.useMasterPitchModel());
wgich requires to have the models mapped as properties, like volume.

BTW, thanks for the fix. If you spend a bit more time on refactoring the models/properties for the Track class, that would be great.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Aug 7, 2017

Member

I tested this and it works like a charm 👍 But @gi0e5b06 is right it seems inconsistent with the rest of the software so that'd need to change before a merge.

Member

Umcaruje commented Aug 7, 2017

I tested this and it works like a charm 👍 But @gi0e5b06 is right it seems inconsistent with the rest of the software so that'd need to change before a merge.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 8, 2017

Member

I think @gi0e5b06 is right, too.

getPitchGroupBox() should be renamed pitchGroupBox()

Yes, it's our convention.

Use the read accessor and not directly the field

It is recommended, but I think it's not necessary.

Member

PhysSong commented Aug 8, 2017

I think @gi0e5b06 is right, too.

getPitchGroupBox() should be renamed pitchGroupBox()

Yes, it's our convention.

Use the read accessor and not directly the field

It is recommended, but I think it's not necessary.

@Umcaruje Umcaruje added this to the 1.2.0 milestone Aug 8, 2017

@zonkmachine zonkmachine removed their request for review Aug 10, 2017

@serdnab

This comment has been minimized.

Show comment
Hide comment
@serdnab

serdnab Aug 10, 2017

Contributor

getPitchGroupBox() should be renamed pitchGroupBox()

Yes, it's our convention.

Addressed

Contributor

serdnab commented Aug 10, 2017

getPitchGroupBox() should be renamed pitchGroupBox()

Yes, it's our convention.

Addressed

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong
Member

PhysSong commented Aug 12, 2017

@serdnab

This comment has been minimized.

Show comment
Hide comment
@serdnab

serdnab Aug 13, 2017

Contributor

Done

Contributor

serdnab commented Aug 13, 2017

Done

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Aug 15, 2017

Member

Tested! This fixes the issue.
Merge?

Member

zonkmachine commented Aug 15, 2017

Tested! This fixes the issue.
Merge?

@zonkmachine zonkmachine merged commit 153ab37 into LMMS:master Aug 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

zonkmachine added a commit that referenced this pull request Aug 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment