Fix CoreServices initalisation order #4058

daschuer · 2021-07-05T12:54:28Z

This fixes the untranslated menu bar https://bugs.launchpad.net/mixxx/+bug/1934655 and is a prerequisite of the untranslated help output fix.

with a duration 5 ms, compared to 500 ms for initalize().

uklotzde

Only minor comments

src/coreservices.cpp

uklotzde · 2021-07-05T21:30:00Z

src/coreservices.cpp

@@ -102,7 +102,8 @@ inline QLocale inputLocale() {
 namespace mixxx {

 CoreServices::CoreServices(const CmdlineArgs& args)
-        : m_runtime_timer(QLatin1String("CoreServices::runtime")),
+        : m_pLV2Backend(nullptr),


We should initialize this plain pointer in the header with LV2Backend* m_pLV2Backend{};

EffectsBackends don't even belong here. This is fixed in #2618. DlgPrefLV2 has been removed.

Please do not touch any effects code before merging #2618. I refuse to fix any merge conflicts from others doing so.

We should initialize this plain pointer in the header with LV2Backend* m_pLV2Backend{};

In Mixxx we only header initialization with exceptions because this introduces a hidden static constructor.
https://github.com/mixxxdj/mixxx/wiki/Coding-Guidelines#non-static-data-member-initializers

I strongly disagree. It doesn't make any sense to repeat all plain pointer members and initialize them explicitly with nullptr. This introduces useless noise. It's much more safe to initialize them directly, especially if you have many of them, see WTrackMenu. By using managed pointers this issue will hopefully disappear over time.

We should revise the coding guidelines. Neither do I understand the motivation nor the issue that should be solved.

"...is used if the member is omitted from the member initializer list of a constructor".

What's wrong with providing a sensible default value to prevent uninitialized members???

This whole discussion about what to do with the pointer is moot. It is removed from this code already in #2618.

What's wrong with providing a sensible default value to prevent uninitialized members???

Nothing.

The question is just where it should be done.
The normal or lets say legacy place is the initialization list in the constructor.
If we keep this habit we have a single place to look at.

Here we have the case where some variables are const initialized and some from parameter values. So you need to look at two places. The header initialization causes an extra static inline constructor and forces recompile more files if we change that value.

The solution in WTrackMenu initializes many pointers twice. One time the stat inline constructor and one time in the real constructor. This is IMHO even more an issue, because the reader might think that he has to deal with a null pointer when he looks into the header which is not the case.

I am not opposing to adjust our policy if we have sensible new ideas. But for now I like to keep the current solution here.

I suggest to choose the header-only modification for minimizing the amount of changes and potential merge conflicts. In the end it doesn't matter, so let's be pragmatic.

Be-ing

merge #2618 first

daschuer · 2021-07-05T22:55:15Z

merge #2618 first

@Be-ing:

We have no policy to reject a PR, because it causes conflicts in other PRs. #2618 is not yet mature, so there is anyway something to do left. I am preparing a post, but because of the giant size it will take a while. If this introduces a merge conflicts, it will be trivial to solve, so this should be not a blocking issue for this PR.

@uklotzde:
What do you think? Just merge, or should I do the hassle of re-basing it out and seeing my IDE complaining longer because of @Be-ing concerns?

Be-ing · 2021-07-05T22:59:28Z

Just merge

Yes, just merge #2618, then please go ahead and rebase this.

Be-ing · 2021-07-05T23:08:18Z

I consider actively interfering with others' work while simultaneously being the only one demanding their work be held up to be hostile.

uklotzde · 2021-07-05T23:18:05Z

@Be-ing I don't expect any major merge conflicts. Otherwise I see myself unable to continue reviewing PRs or taking part in discussions to find a solution.

Holzhaus · 2021-07-06T00:25:05Z

I consider actively interfering with others' work while simultaneously being the only one demanding their work be held up to be hostile.

Daniel pointed out issues, but he's not "the only one demanding to your work to be held up". Don't make this personal. I doubt @daschuer is the only one that that has reservations about #2618 without resolving these issues or at least mentioning them in a TODO.

Anyway, I think it is of course possible to ask people nicely if they'd be willing to postpone their work to not cause merge conflicts in other PRs. But in general, it's the responsibility of the contributor to resolve merge conflicts if they arise.

Of course, huge PRs tend to stay open longer and simultaneously cause more merge conflicts. The only way to avoid this is to avoid huge PRs.

I agree with @uklotzde that the changes in this PR should should only cause trivial merge conflcits (if at all).

Be-ing · 2021-07-06T01:06:07Z

it's the responsibility of the contributor to resolve merge conflicts if they arise.

This is unhelpful.

Be-ing · 2021-07-06T01:35:33Z

Daniel pointed out issues, but he's not "the only one demanding to your work to be held up". Don't make this personal. I doubt @daschuer is the only one that that has reservations about #2618 without resolving these issues or at least mentioning them in a TODO.

This is also unhelpful. If you have objections, say them.

Be-ing · 2021-07-06T01:41:39Z

Don't make this personal.

There is an elephant in the room here. I cannot help but observe that I am treated differently. My objections to PRs get ignored (and I am not just talking about this PR, here are a few examples: #1064, #3074, mixxxdj/website#217), meanwhile @daschuer is allowed to indefinitely hold up PRs (for months at a time!) without any specific objections and hold up release blocking issues for months against the unanimous consensus of everyone else (remember the cue color debacle?). You all have let @daschuer hold up my PRs for weeks for nonsense as trivial as a code comment (#2920). I wish that was an exaggeration, but it's not, that literally happened. This is not fair and I'm not going to take it any longer. I will not keep participating in Mixxx if this is allowed to continue. There must be unbiased rules to prevent this or it will not be worth my time.

Be-ing · 2021-07-06T12:16:09Z

he's not "the only one demanding to your work to be held up"

@Holzhaus you approved #2618 months ago. If you have new objections, then explain them.

Holzhaus · 2021-07-06T19:08:31Z

he's not "the only one demanding to your work to be held up"

@Holzhaus you approved #2618 months ago. If you have new objections, then explain them.

Yes, I did approve months ago. Then @daschuer spent his free time to do an in-depth review and found issues that I overlooked. While you were away, I did spent some time towards fixing them. Not because I want Daniel to shut up, but because I agreed with him.

I think it's likely that not all remaining comments are merge blockers, but this should be discussed openly and then we agree what is in scope for this PR and what is not. If you believe the discussion drags on and goes in circles, it's possible to call for a vote. But unilaterally resolving review comments without pushing a fix is not how we want or should do reviews.

JoergAtGithub · 2021-07-06T19:28:32Z

Such discussions in PR comment sections, or Zulip, are neither productive nor does anybody feels good in the aftermath. I'm sure, that every single person here works hard to bring this project forward. But these discussions here result in excatly the opposite!
May I recommend, that you do a video conference with all the core devs. Maybe you can aggree on an conciliator, who decides about the next steps in case of conflicting opinions? Or you define a formal process (like a ballot) in case of conflicting views of different devs?

daschuer · 2021-07-06T19:35:01Z

Thank you.

Be-ing · 2021-07-06T22:10:52Z

@daschuer spent his free time to do an in-depth review

This was unwanted. I specifically asked @daschuer not to do this and he refused.

If you believe the discussion drags on

It has been at that point for a long time.

unilaterally resolving review comments without pushing a fix is not how we want or should do reviews.

I have pushed over 100 commits in response to unwanted reviews.

ywwg · 2021-07-07T00:20:20Z

Unwanted reviews? If you ask that other developers take the time to review your branch and someone does, it's not appropriate to object when they make comments and ask for changes. It is also extremely inappropriate to refuse to participate on other code reviews until your demands are met. I think we'd all like to find a way forward on 2618 but that will take working together.

Be-ing · 2021-07-07T00:23:39Z

Yes, demanding that everybody else wait indefinite amounts of time because you say you want to review despite 3 other reviewers already approving is not fair at all. And now I am being demanded to wait some indefinite amount of time for some unspecified objection to merging, yet @daschuer has found time to actively interfere with merging #2618 by making this PR.

Be-ing · 2021-07-07T00:28:28Z

It is also extremely inappropriate to refuse to participate on other code reviews until your demands are met.

When the reviewer has a history of incredibly insensitive demands on my code, such as holding up my work for weeks for a disagreement about a code comment, yeah, I don't want the review. I've been putting up with this for years, it's not okay, and it's not okay that everyone else tolerates it.

Be-ing · 2021-07-07T00:33:43Z

I have offered repeatedly to compromise by continuing to work with @daschuer on addressing remaining issues after merging #2618 to main, but he has ignored these offers and instead demanded that I continue to wait months, meanwhile merge conflicts pile up, and he does nothing to fix them, and everyone else keeps blaming me for how big the PR is while I did almost all the work for macOS (4 straight months of it!) to get 2.3 released.

Be-ing · 2021-07-07T00:39:19Z

Or you define a formal process (like a ballot) in case of conflicting views of different devs?

I have repeatedly put forth proposals for formal processes on Zulip and they have been largely ignored.

Be-ing · 2021-07-07T01:55:39Z

This endless obstruction was not tolerated in 2014 with #180, so I do not understand why it is now.

Holzhaus · 2021-07-08T14:53:04Z

There are merge conflicts now.

daschuer · 2021-07-08T15:09:01Z

Oh no ... fixed

daschuer · 2021-07-08T15:23:47Z

ready

uklotzde

Thank you for considering our recommendations in this special case!

uklotzde · 2021-07-08T15:46:52Z

@daschuer pre-commit issues?

daschuer · 2021-07-08T15:50:22Z

A white space issue. Force pushed.

uklotzde · 2021-07-11T07:08:22Z

@Be-ing Ping. I would like to get consent by everyone involved.

Holzhaus · 2021-07-22T19:39:12Z

src/coreservices.cpp

@@ -136,6 +132,25 @@ void CoreServices::initialize(QApplication* pApp) {
            m_cmdlineArgs.getLogLevel(),
            m_cmdlineArgs.getLogFlushLevel(),
            logFlags);
+}
+
+void CoreServices::preInitialize(QApplication* pApp) {


Is this method really necessary? Couldn't we just make it part of the CoreServices constructor?

Is there any need to create the CoreServices object before creating a QApplication? If not, we should move this into the constructor. Less possibilities to use this class wrong or miss an initialization step.

There does not seem to be a purpose for this method. Move the code directly into the constructor.

It is just documentation support.

I don't think this makes the code more readable; it makes me wonder why the function exists. I think it would be better to move the code into the constructor and add a comment: "These need to be initialized before the GUI. They are fast to initialize. Everything else is deferred to the initialize method so the GUI can show the progress."

I like the current state.

I don't to bike-shed, but

it only contains the body of the constructor, which now contains just this single invocation and

the method is public although it doesn't need to be.

The function is private.

src/coreservices.h

src/mixxxmainwindow.cpp

daschuer · 2021-07-25T11:15:26Z

Done!
@Be-ing: merge

coveralls · 2021-07-25T11:46:35Z

Pull Request Test Coverage Report for Build 1084024096

0 of 18 (0.0%) changed or added relevant lines in 1 file are covered.
11 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.01%) to 26.018%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/coreservices.cpp	0	18	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/engine/readaheadmanager.cpp	2	88.79%
src/engine/cachingreader/cachingreader.cpp	9	71.38%

Totals
Change from base Build 1083902170:	-0.01%
Covered Lines:	20004
Relevant Lines:	76886

💛 - Coveralls

ronso0 · 2021-07-25T13:45:26Z

@Be-ing Ping. I would like to get consent by everyone involved.

FWIW in german there's a distinction between
'Konsens' (consensus) = everyone involved must agree
'Konsent' (?) = one or more minor objections are considered, but the majority decisions is implemented while the cause for the objections will be resolved asap (fear of merge conflicts)

uklotzde · 2021-07-25T17:34:01Z

I have chosen the word consent deliberately.

See also: https://en.wikipedia.org/wiki/Disagree_and_commit

ywwg · 2021-07-26T19:50:20Z

In English:

"consensus": has multiple meanings, but implies a majority of stakeholders agree. Does not require 100% buy-in.
"everyone's consent": everyone must agree

"Consensus" is a complex term because there are specific systems called "Consensus Process" which have lots of details about decision-making. But in a general sense, it means "most people agree even if some have reservations, but nobody is dead-set against the idea"

daschuer · 2021-07-31T00:13:57Z

Conflicts are resolved now. Ready for merge! ?

Be-ing · 2021-07-31T00:19:15Z

src/coreservices.h

-    LV2Backend* m_pLV2Backend;
+    LV2Backend* m_pLV2Backend{};


unrelated change, please remove from this PR

daschuer · 2021-07-31T00:51:56Z

Done.

daschuer · 2021-08-01T09:09:17Z

OK, again done.

daschuer added 3 commits July 5, 2021 14:11

Fix uninitialized variable

e8541a5

Add function preInitialize() to CoreServices

ef2bcd1

Move Stats and translation init to peInitialize().

4ffe39c

with a duration 5 ms, compared to 500 ms for initalize().

uklotzde suggested changes Jul 5, 2021

View reviewed changes

Be-ing suggested changes Jul 5, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into lp1934655

2684e24

Move initalizer to the header

8a38e81

uklotzde approved these changes Jul 8, 2021

View reviewed changes

Fix unused warning

419a3df

daschuer force-pushed the lp1934655 branch from e2fe4d5 to 419a3df Compare July 8, 2021 15:48

Holzhaus requested a review from Be-ing July 13, 2021 07:57

Holzhaus reviewed Jul 22, 2021

View reviewed changes

daschuer added 2 commits July 25, 2021 12:48

Merge remote-tracking branch 'upstream/main' into lp1934655

14a61dc

move pCoreServices->preInitialize() to runMixxx

434f6cf

daschuer added 2 commits July 31, 2021 01:46

move preInitalize to the constructor and make init functions private

b1f0434

Merge remote-tracking branch 'upstream/main' into lp1934655

6780078

github-actions bot added the code quality label Jul 31, 2021

Be-ing reviewed Jul 31, 2021

View reviewed changes

Remove initalizing of m_pLV2Backend

acda4cc

Be-ing requested a review from Holzhaus July 31, 2021 22:36

daschuer added 2 commits August 1, 2021 11:07

inline CoreService::preInitalize()

9c9e00f

over runtime measurement to CoreServices c-tor

d0eb104

daschuer force-pushed the lp1934655 branch from 046dcf5 to d0eb104 Compare August 1, 2021 09:08

Be-ing merged commit ac4cacd into mixxxdj:main Aug 1, 2021

Be-ing mentioned this pull request Aug 1, 2021

Add FIXMEs for use of bpm.value() without checking #4165

Merged

daschuer mentioned this pull request Aug 18, 2021

Fix version detection in Findhidapi.cmake #4215

Merged

mixxxbot mentioned this pull request Aug 23, 2022

Menu bar is not translated #10463

Closed

Fix CoreServices initalisation order #4058

Fix CoreServices initalisation order #4058

Conversation

daschuer commented Jul 5, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing left a comment

Choose a reason for hiding this comment

daschuer commented Jul 5, 2021

Be-ing commented Jul 5, 2021

Be-ing commented Jul 5, 2021

uklotzde commented Jul 5, 2021

Holzhaus commented Jul 6, 2021

Be-ing commented Jul 6, 2021

Be-ing commented Jul 6, 2021

Be-ing commented Jul 6, 2021 • edited Loading

Be-ing commented Jul 6, 2021

Holzhaus commented Jul 6, 2021

JoergAtGithub commented Jul 6, 2021

daschuer commented Jul 6, 2021

Be-ing commented Jul 6, 2021

ywwg commented Jul 7, 2021 • edited Loading

Be-ing commented Jul 7, 2021 • edited Loading

Be-ing commented Jul 7, 2021 • edited Loading

Be-ing commented Jul 7, 2021 • edited Loading

Be-ing commented Jul 7, 2021

Be-ing commented Jul 7, 2021

Holzhaus commented Jul 8, 2021

daschuer commented Jul 8, 2021

daschuer commented Jul 8, 2021

uklotzde left a comment

Choose a reason for hiding this comment

uklotzde commented Jul 8, 2021

daschuer commented Jul 8, 2021

uklotzde commented Jul 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jul 25, 2021

coveralls commented Jul 25, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1084024096

💛 - Coveralls

ronso0 commented Jul 25, 2021

uklotzde commented Jul 25, 2021

ywwg commented Jul 26, 2021

daschuer commented Jul 31, 2021

Choose a reason for hiding this comment

daschuer commented Jul 31, 2021

daschuer commented Aug 1, 2021

uklotzde Jul 5, 2021 •

edited

Loading

Be-ing commented Jul 6, 2021 •

edited

Loading

ywwg commented Jul 7, 2021 •

edited

Loading

Be-ing commented Jul 7, 2021 •

edited

Loading

Be-ing commented Jul 7, 2021 •

edited

Loading

Be-ing commented Jul 7, 2021 •

edited

Loading

coveralls commented Jul 25, 2021 •

edited

Loading