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.

Sign up for GitHub

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

Jump to bottom

Macros Backend #4528

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Macros Backend #4528

wants to merge 6 commits into from

Conversation

xeruf
Copy link
Contributor

@xeruf xeruf commented Nov 17, 2021 •
edited
Loading

This is forked off from #2989 with the GUI parts removed, to be merged this week :)
@daschuer @Holzhaus @Be-ing

Holzhaus
Holzhaus requested changes Nov 17, 2021
View reviewed changes
src/library/dao/macrodao.cpp Outdated Show resolved Hide resolved
src/coreservices.cpp Outdated Show resolved Hide resolved
src/engine/sidechain/enginesidechain.cpp Outdated Show resolved Hide resolved
uklotzde
uklotzde suggested changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@uklotzde uklotzde left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR contains lots of unrelated and even wrong changes. Please revert them.

Examples:

  • DbId
  • Reorderings in Track
  • ...

@uklotzde
Copy link
Contributor

After reverting the unrelated changes the remaining commits should be stashed into a single commit before starting a review. I suppose reverting the unrelated changes in individual commits would even not be possible otherwise.

@xeruf
Copy link
Contributor Author

xeruf commented Nov 18, 2021

After reverting the unrelated changes the remaining commits should be stashed into a single commit before starting a review.

Do you really want to give up all history?

@uklotzde
Copy link
Contributor

After reverting the unrelated changes the remaining commits should be stashed into a single commit before starting a review.

Do you really want to give up all history?

Not worth it imho.

@xeruf xeruf mentioned this pull request Nov 18, 2021
Draft
@xeruf xeruf requested review from Holzhaus and uklotzde and removed request for Holzhaus November 18, 2021 09:30
@xeruf
Copy link
Contributor Author

xeruf commented Nov 18, 2021

Ready :)

@xeruf xeruf force-pushed the macros-backend branch 3 times, most recently from 17e8ff6 to 721a2d9 Compare November 18, 2021 09:44
@ronso0
Copy link
Member

ronso0 commented Nov 18, 2021

@xeruf While rebasing, did you make sure every commit builds? (git bisect)

@xeruf
Copy link
Contributor Author

xeruf commented Nov 18, 2021 •
edited
Loading

@xeruf While rebasing, did you make sure every commit builds? (git bisect)

there is only one commit, no need to bisect ;)

I am building right now locally.

@xeruf xeruf requested a review from Holzhaus November 18, 2021 09:52
@xeruf xeruf force-pushed the macros-backend branch 2 times, most recently from 51e5cea to 9c0f7f2 Compare November 18, 2021 12:38
uklotzde
uklotzde suggested changes Nov 18, 2021
View reviewed changes
src/util/db/dbid.h Outdated Show resolved Hide resolved
src/track/track.h
@@ -402,6 +406,7 @@ class Track : public QObject {
mixxx::TrackRecord newRecord,
mixxx::BeatsPointer pOptionalBeats = nullptr);

bool isDirty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid unrelated reorderings in huge feature PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you mark this comment as resolved while keeping the reordering??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had removed the unrelated reorderings, this function is clearly related.
Do you really prefer to keep the files messy rather than grouping functions with some logic, as you don't want separate cleanup PRs either?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reorderings could be done in a separate PR that does not modify the implementations. I already explained why.

src/track/track.cpp Show resolved Hide resolved
src/track/track.cpp Show resolved Hide resolved
src/track/macroaction.h Outdated Show resolved Hide resolved
src/track/macro.h Outdated Show resolved Hide resolved
src/track/macro.cpp Outdated Show resolved Hide resolved
src/track/macro.cpp Outdated Show resolved Hide resolved
src/proto/macro.proto Show resolved Hide resolved
res/schema.xml Show resolved Hide resolved
@xeruf
Copy link
Contributor Author

xeruf commented Nov 18, 2021

Now I have some weird issues: Every test individually succeeds, but when running them collectively with

/usr/bin/ctest mixxx-test --output-on-failure -R Macro.*

It always hangs at 7/10, regardless of which test is executed as 7th. I am highly confused.

@xeruf
Copy link
Contributor Author

xeruf commented Nov 19, 2021

As said, I would like to merge this week, I am not sure how much time I will find after that.
I don't know why Ubuntu fails now - Windows, mac and local build all run fine.

@ronso0
Copy link
Member

ronso0 commented Nov 19, 2021

maybe because clazy fails? https://github.com/mixxxdj/mixxx/runs/4263351165?check_suite_focus=true#step:9:741

uklotzde
uklotzde reviewed Nov 20, 2021
View reviewed changes
src/track/track.cpp Outdated Show resolved Hide resolved
uklotzde
uklotzde reviewed Nov 20, 2021
View reviewed changes
src/track/macroaction.h Outdated Show resolved Hide resolved
uklotzde
uklotzde reviewed Nov 20, 2021
View reviewed changes
src/track/macro.h Outdated
/// A Macro stores a list of MacroActions as well as its current state and label.
class Macro {
public:
static QByteArray serialize(const QList<MacroAction>& actions);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why these functions for serializing/deserializing one of the members are public? I would expect them to handle the entire class and not only an isolated aspect.

The minimum change is renaming them as serializeActions()/deserializeActions(), but there should be a more appropriate design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the actions are the macro, this is what is needed to serialize it. Sure, they can be private, though I don't know what that adds.

Copy link
Contributor

Choose a reason for hiding this comment

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

The macro entity is more than a list of actions. The list of actions is just the part that gets serialized.

Btw, repetition does not make wrong claims right. I am not going to explain everything twice.

uklotzde
uklotzde reviewed Nov 20, 2021
View reviewed changes
src/track/macro.cpp Outdated Show resolved Hide resolved
uklotzde
uklotzde reviewed Nov 20, 2021
View reviewed changes
src/track/macro.cpp Outdated Show resolved Hide resolved
uklotzde
uklotzde reviewed Nov 20, 2021
View reviewed changes
src/test/signalpathtest.h Outdated Show resolved Hide resolved
@xeruf
Copy link
Contributor Author

xeruf commented Nov 21, 2021

@uklotzde sorry for me being slow-witted at times, my little C++ experience has already gotten rusty again 😅
I have addressed the mentioned architectural issues as well as I can now.

@JoergAtGithub
Copy link
Member

@uklotzde sorry for me being slow-witted at times, my little C++ experience has already gotten rusty again sweat_smile I have addressed the mentioned architectural issues as well as I can now.

@xeruf Did you forgot to commit this changes?

@xeruf
Copy link
Contributor Author

xeruf commented Nov 23, 2021

Right, got me there :)

@xeruf xeruf requested a review from uklotzde November 23, 2021 15:38
@JoergAtGithub
Copy link
Member

Why was only one single CI test executed here? Maybe you need just rerun it?

@xeruf
Copy link
Contributor Author

xeruf commented Jan 15, 2022

No idea. I only find time to look into this every few months, and unfortunately always some new nitpicks come up, delaying this feature for over a year by now...

@JoergAtGithub
Copy link
Member

You need to fix the merge conflict, before it executes CI

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Apr 16, 2022
@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Sep 5, 2023
@ronso0 ronso0 marked this pull request as draft October 18, 2023 08:31
@ronso0 ronso0 mentioned this pull request Apr 23, 2024
Open
@JoergAtGithub JoergAtGithub mentioned this pull request Apr 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Awaiting requested review from Holzhaus

@Swiftb0y Swiftb0y Awaiting requested review from Swiftb0y

@uklotzde uklotzde Awaiting requested review from uklotzde

Assignees
No one assigned
Labels
build code quality library ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xeruf @uklotzde @ronso0 @JoergAtGithub @Holzhaus @Swiftb0y