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

Merge 2.3 into main: HotcueControl #3379

Closed
wants to merge 9 commits into from
Closed

Merge 2.3 into main: HotcueControl #3379

wants to merge 9 commits into from

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Nov 28, 2020

Individual setters/getters break encapsulation and are an anti-pattern. I have replaced them with use case related functions to resolve the merge conflicts:

  • HotcueControl::canPreview()
  • HotcueControl::isPreviewing()
  • HotcueControl::startPreviewing()
  • HotcueControl::stopPreviewing()

Please check if the side-effect --m_iCurrentlyPreviewingHotcues in the else if arm in CueControl::hotcueActivatePreview() is correct! I am unsure about the order of evaluation. We should try to avoid such brittle code tricks.

@uklotzde uklotzde added this to the 2.4.0 milestone Nov 28, 2020
# Conflicts:
#	src/engine/controls/cuecontrol.cpp
#	src/widget/whotcuebutton.cpp
@daschuer
Copy link
Member

I have pushed a merge commit in the mean time.
I have also a follow up branch that fixes reviewing. It is broken anyway.
Can we hold your changes back until this is solved and then re-base them onto the fix?
It is anyway not a good idea to hide refactoring in a merge commit.

@uklotzde uklotzde closed this Nov 28, 2020
@uklotzde
Copy link
Contributor Author

I have pushed a merge commit in the mean time.
I have also a follow up branch that fixes reviewing. It is broken anyway.
Can we hold your changes back until this is solved and then re-base them onto the fix?
It is anyway not a good idea to hide refactoring in a merge commit.

Sure

@daschuer
Copy link
Member

See: #3382

@uklotzde uklotzde deleted the hotcuecontrol_preview branch December 2, 2020 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants