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

RateControl/Position ScratchController: use std::unique_ptr, PollingControlProxy etc. #14058

Open
wants to merge 1 commit into
base: 2.5
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Dec 25, 2024

Just some preparation for #14059
Targeting 2.5 because #14059 (or an alternative) may become an appropriate bug fix.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

couple nits, address as you wish.

src/engine/controls/ratecontrol.cpp Outdated Show resolved Hide resolved
src/engine/controls/ratecontrol.cpp Outdated Show resolved Hide resolved
Comment on lines 196 to 197
m_pJog = std::make_unique<ControlObject>(ConfigKey(group, QStringLiteral("jog")));
m_pJogFilter = std::make_unique<Rotary>();
Copy link
Member

Choose a reason for hiding this comment

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

this is a refactoring, right? Should we rebase on main instead? I know I changed this line in main in #13853 so there are bound to be conflicts...

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a refactoring, right? Should we rebase on main instead?

Yes, and no: as metioned above, these are just preparational steps for the scratch fix #14059 which should go to 2.5 IMO

src/engine/positionscratchcontroller.h Outdated Show resolved Hide resolved
@ronso0 ronso0 force-pushed the ratecontrol-prep branch 2 times, most recently from 5112667 to 5b63d77 Compare December 27, 2024 23:24
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

src/engine/controls/ratecontrol.h Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

did another round and found another couple nits. Pls address and rebase squash afterwards so we can merge.

src/engine/positionscratchcontroller.h Outdated Show resolved Hide resolved
src/engine/positionscratchcontroller.h Outdated Show resolved Hide resolved
src/engine/positionscratchcontroller.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you. Waiting for CI.

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

Successfully merging this pull request may close these issues.

2 participants