Lmms: Reorganising LMMS, renaming editors and classes and everything in between (cont. #120)

[russiankumar]

If you are here because your PR is mentioned then check the instructions at the bottom of the comment.

This is an issue continued on from #120. We're mostly following on from what was decided there, though if you have something to say, feel free to.

These are the draft steps that we will have done one-by-one in order to achieve this massive task:

• [x] Freeze code on December 5th, 2020
• [ ] Clear PR backlog by January 15th, 2021
  
  
  
  • [x] Ping devs of older PRs via the table in this issue
  
  
  • [ ] Review and test PRs requiring this
  
  
  • [ ] Close all stale PRs for rebase post-reorg
  
  
• [ ] Format code...
  
  
  
  • [x] with format PRs (#5791, _request to not push any more such PRs_)
  
  
  • [ ] with Clang + CI (#4690, comment)
  
  
• [ ] Preparatory changes
  
  
  
  • [ ] Clear out space for renamings (_will expand on this step after discussion_)
  
  
• [ ] Rename PRs
  
  
  
  • [x] Rename MidiTime -> TimePos (#5684)
  
  
  • [ ] Rename Pattern* -> Clip* [* stands for suffixes eg. ".cpp", "TCO", whatever else that starts with that phrase]
  
  
  • [ ] Rename BB* -> Pattern*
  
  
  • [ ] Rename AutomationPattern* -> AutomationClip*
  
  
  • [ ] Rename FX-Mixer -> Mixer
  
  
  • [ ] Rename any other remaining editors
  
  
• [ ] Other major changes
  
  
  
  • [x] Split Track.cpp into one file per class (#5806)
  
  
  • [ ] Reorganise all files into better places & folders (_requires some discussion_)
  
  
  • [ ] Add a namespace for LMMS (an example)
  
  
  • [ ] Move to newer library versions (eg. C++17, _needs discussion_)
  
  
• [ ] Reopen code for fresh PRs (or rebased old ones)

Feel free to drop suggestions, things you would want to see, etc. here. If you have something that would require a major changed in code like this renaming project (like maybe moving to C++17?) then please do tell us, because I think it's better to finish all major changes in one go rather than have a bunch of code freezes for each change.

---

The section where I mentioned your PR

If your PR was mentioned by this issue, then ping back on your PR and mention this issue so I can change the state from "Waiting for Response" to whatever it really is. I'll probably ping all PRs which haven't responded.

Edit: Considering that new PRs are being added and merged, I'm changing this list to be a tracker of older PRs so that they don't get lost in pages and pages of PRs.

I've not tagged some PR authors because it was their first PR, and the tag system didn't allow me to tag them yet. These PRs are almost all stale and inactive for at least a year.

Expand table of PRs

| PR | Last Active | Has conflicts? | Stale/Active/Ready |
|---|---|---|---|
| #2068 | - | - | Waiting for Response from @michaelgregorius |
| #2505 | - | - | Waiting for Response |
| #2944 | - | - | Waiting for Response from @BaraMGB |
| #3115 | - | - | Waiting for Response from @liushuyu |
| #3117 | - | - | Waiting for Response |
| #3134 | - | - | Waiting for Response from @liushuyu |
| #3224 | - | - | Waiting for Response from @tonychee7000 |
| #3467 | - | - | Waiting for Response from @liushuyu |
| #3532 | 2019 | No | (Feature Merge) Stale |
| #4027 | - | - | Waiting for Response from @tresf |
| #4067 | - | - | Waiting for Response from @tresf |
| #4232 | 2019 | Yes | (Feature Merge) Stale |
| #4357 | - | - | Waiting for Response |
| #4367 | - | - | Waiting for Response from @SecondFlight |
| #4369 | - | - | Waiting for Response from @SecondFlight |
| #4397 | 2020 | Yes | In Progress/Ready |
| #4412 | - | - | Waiting for Response |
| #4441 | - | - | Waiting for Response from @BaraMGB |
| #4443 | 2020 | Yes | (Feature Merge) In Progress |
| #4571 | - | - | Waiting for Response |
| #4635 | - | - | Waiting for Response from @DouglasDGI |
| #4662 | 2020 | Yes | Stale/In Progress |
| #4674 | 2018 | No | (Feature Merge) Stale/Ready |
| #4690 | - | - | Waiting for Response from @JohannesLorenz |
| #4894 | 2020 | No | Ready (needs review) |
| #4994 | - | - | Waiting for Response from @Reflexe |
| #5000 | - | - | Waiting for Response from @DouglasDGI |
| #5098 | - | - | Waiting for Response from @michaelgregorius |
| #5147 | 2020 | No | In Progress |
| #5158 | - | - | Waiting for Response from @artur-twardowski |
| #5161 | - | - | Waiting for Response from @DouglasDGI |
| #5166 | - | - | Waiting for Response from @iansannar |
| #5168 | - | - | Waiting for Response from @DouglasDGI |
| #5174 | 2020 | Yes | In Progress |
| #5197 | 2019 | No | Stale |
| #5202 | 2020 | Yes | Ready (needs review) |
| #5204 | 2020 | Yes | Stale/Ready (needs discussion) |
| #5229 | 2020 | No | In Progress (needs testing) |
| #5230 | 2019 | No | Ready (waiting for #5229) |
| #5236 | 2020 | No | Stale/Ready |
| #5239 | 2019 | Yes | Stale |
| #5247 | - | - | Waiting for Response from @Reflexe |
| #5261 | 2020 | Yes | Stale (needs review) |
| #5274 | - | - | Waiting for Response from @fschuelke |
| #5291 | 2020 | No | Dropped |
| #5292 | 2020 | No | Ready (needs review & testing) |
| #5356 | - | - | Waiting for Response from @artur-twardowski |
| #5357 | - | - | Waiting for Response from @komodor1 |
| #5436 | - | - | Waiting for Response from @Reflexe |
| #5458 | - | - | Waiting for Response from @DouglasDGI |
| #5500 | - | - | Waiting for Response from @artur-twardowski |
| #5502 | - | - | Waiting for Response from @thmueller64 |
| #5503 | 2020 | No | In Progress (waiting for review) |
| #5510 | - | - | Waiting for Response from @tresf |
| #5516 | - | - | Waiting for Response from @EmoonX |
| #5522 | 2020 | Yes | Ready (needs review and testing) |
| #5524 | 2020 | No | Ready (needs review) |
| #5544 | 2020 | No | Stale/In Progress |

[qnebra]

Best way to know that PR is active or not is just to peek into commit history. If there is nothing happen in more than 1 year then PR is stale.

[russiankumar]

When there's 87 PRs... it would be a lot easier to have devs help too. Don't wanna be doing the whole thing myself xD

[claell]

Hm, I know that #5261 is probably not stale, however I don't know whether the dev currently has time. The problem with some PRs is that it takes much time until the code is reviewed and when that finally happens the dev is no longer free to address the requested changes. Two weeks might be a pretty tough goal, but let's see.

Can we use some automation to see which PRs will be affected and maybe also some automation for the refactoring that can be easily applied to outstanding PRs? I think about using something like documented commands for a Unix terminal with regular expressions for everything. That way, the refactoring is both easier to understand, to adjust and reproducible. It then could be applied to open PRs pretty easily.

[russiankumar]

which PRs will be affected

I'm pretty sure every PR will be affected considering the size of these changes

documented commands for a Unix terminal with regular expressions for everything.

Yes, that has to be what will be used (like maybe a find+replace across all files) because doing everything manually will take forever. If you have suggestions on how to go about this, would love to hear them.

EDIT: #5261 has actually been ready for review and merge for quite some time, but it was ignored because of how big it is and in its present state it even has conflicts. If the dev pings back saying that they can work on it soon then we can extend the two-week deadline for the PR to be ready for merge.

[Spekular]

With regards to my PRs:

• #5117 isn't stale, I intend to get it merged before this reorganization (edit: it's merged!)
• #5427 is a draft so it can stay marked as stale
• #5524 is in limbo. Its pretty much in the same state as #5117, it's just less important to me to get merged

[russiankumar]

Updated, thanks!

[Spekular]

In regards to this

If you have something that would require a major changed in code

I say this is a good opportunity to split the Track classes (including subclasses like SampleTrack, AutomationTrack) into separate files. Those files are huge right now and among other things it makes it harder to keep track (pun) of which members come from which classes. Potentially making compilation more granular is a big plus too.

[russiankumar]

Yeah, it definitely is HUGE. How about three files (+ includes): Track (Track & TrackView), TrackWidget (TrackOperationsWidget, the likes) and Clip (current TCO and TCOView -> Clip & ClipView)

[Spekular]

I'm a fan of one file per class, but if that's not a popular proposal then three would at least be a step in the right direction.

[claell]

Yes, that has to be what will be used (like maybe a find+replace across all files) because doing everything manually will take forever. If you have suggestions on how to go about this, would love to hear them.

Maybe we can come up with one or several bash scripts that contain the relevant commands.

[russiankumar]

Do approvals from people without write-access count towards mergeability? Github says not but maybe the mods here take it differently?

[IanCaio]

My PRs:

• #5503 : Stale. It's functional and there are not any requested changes, but I think it didn't raise much interest in people. Wouldn't mind if it didn't get merged.
• #5547 : Not stale. Functional and no current requested changes unresolved. Just waiting for review to get merged (it's a small PR so it shouldn't be a hard review).
• #5581 : Not stale. Functional and no current requested changes unresolved. Relatively recent PR, so still under testing and review.

I think the idea of renaming the editors and maybe reorganizing some things is good, but I want to express a small concern:

There are not that many active devs with merge priviledges and the amount of PRs makes their job difficult. Specially if we balance that there are PRs that are well written, making them easier to review, but others can be a bit of a mess and take way longer (or even might not be mergeable). Also, some PRs are as small as _< 50_ lines, others have _> 1000_ lines of code. Trying to shorten the review process can be dangerous in the sense that some bugs or badly written code can be merged, and then we will have a big mess to deal with later. Simply allowing people with no write-access approvals count to merge PRs is also dangerous in that sense, because we can't be sure everyone knows the coding guidelines and code base well enough to make a good decision, or even how committed they are in making a good review. That being said, I think one week is a __way__ too optimistic timeline for finishing the PR reviews.

As for the process of doing the refactor itself, I see two ways of it being done:

• Someone can take the burden and work on the refactor alone, but frequently be updating the PR with the changes and waiting for reviews from those particular updates before continuing. That would make the reviewing process easier, because mistakes would be corrected from the beginning before they grow more complex. It's also easier to review a couple hundred lines weekly than thousands of lines in a month.
• We can see how many devs/contributors volunteer to the cause, discuss in very fine details the changes that will be made and assign files for each volunteer with a timeline (a light one that takes into account that most of us have jobs and life appointments as well). At the end of that timeline, we all submit those changes so they can be reviewed for a while. New files are assigned and so it goes.

I particularly prefer the second option, this doesn't sound like a one man job.

I don't know if I'm sounding too precautions, this refactor is a really good thing. I just don't want to see it cost the current stability we have on LMMS.

[Veratil]

My PRs:

• #5123 approved but needs testing, no conflicts
• #5124 approved but needs testing, no conflicts
• #5174 need to update IRT one review, has conflicts easy to resolve
• #5196 needs to be reviewed and tested, has conflicts easy to resolve
• #5204 Proof of concept, meant for discussion, but works (at the opening of the request)
• #5229 Step 2 in removing global automation from #403, needs a lot of testing, no conflicts
• #5230 Step 3 in removing global automation from #403, needs testing after #5229 is merged, no conflicts
• #5253 we should probably merge this before I keep adding to it 😂, no conflicts

[he29-net]

Ping all authors of all current PRs asking them to wind up within two weeks since now (done by the table)

@russiankumar Just FYI, this "ping" did absolutely nothing in my case. I happen to be obsessively reading the GitHub log on Discord, so I noticed this issue, but I imagine most of the PR authors have better things to do and won't notice anything since it did not generate an e-mail notification.

So by marking all PRs that don't get any response as "stale", we may be simply discarding a lot of good and useful code from authors that are no longer active and won't notice a mention appeared on their PR -- if they have been waiting for test and review for months (or even years), I would assume they just gave up and forgot about it. Some of the PRs could be unfinished WIP, sure, but I wouldn't rely just on the fact that the author did not reply to a ping. Some of them may not even be able to, for whatever reason (lost the account, fell off a cliff, signed an NDA in their new job, ...).

The PR backlog is a _huge_ problem for LMMS in my opinion (in a way, there is already an issue talking about this: #4935) -- it slows down development if someone needs to wait for another PR to get through, it generates conflicts in newer PRs when something old finally gets merged, it demotivates new devs who have to wait a long time to see their contribution accepted, and it demotivates experienced devs who just see a big pile of work that won't stop growing.

So it's great that someone took the initiative, thanks! But I doubt it can be solved within a few weeks just "by the way" as a few bullet points in this issue. The backlog exists precisely because reviews take a lot of time and effort, and we have just a handful of experienced devs who can do it, all with their own lives, time budgets and motivations. Clearing the backlog will probably require quite some team effort and several months at least. That is, if we want to avoid the "mark everything as stale and flush it down the toilet" approach. Authors of the affected PRs would probably think twice before investing time into making another contribution that could end up the same...

The code freeze could be good idea (but only for new features, not bug fixes), it could cool things down enough to give the dev team time to work on the backlog. Although at the same time, it could discourage new potential devs, who would be unable to submit their contributions.

It's just a hard problem to crack either way.. :-/

---

As for PRs where I'm involved:

• #5522 is my own and waiting for testing and review (but it seems to have a new merge conflict);
• #4397 is from curlymorphic and waiting for merge of my branch to the PR and subsequent review of the changes (PhysSong already reviewed the original code and I already did some testing when fixing / finishing the PR).

[russiankumar]

Thanks for the PR updates, everyone.

Many of you have pointed out a concern that two weeks is nowhere near enough to finish reviewing and merging as many PRs as possible. I am convinced, so I suppose time till mid to end of August should be enough? That essentially does leave one month and a bit for everything.

As @he29-net pointed out, waiting for too less or too long are both problems. It's in our best interests to probably find a middle ground and finish merging most PRs. It's also enough time for all devs to respond (if they chance upon their PRs anytime soon). If they don't respond, then I can drop a message on their PRs to see whether they can finish it up or not. It's definitely bad if we ignore someone's hard work and just dump it. Since most probably these PRs will not be able to be merged after these changes, now has to be the time we do it.

It's probably going to involve a lot of work, and given the fact that write-access devs are few and usually busy, won't be an easy thing. But then again, it's best to solve the issue now before it expands even more into a big mess, discouraging new devs from developing.

I hope this shall leave enough time for most devs to ping back, and for as many (reviewed and tested) PRs to merge.

[softrabbit]

I noticed this issue by luck, really. It's not like I keep going back to read my PRs every now and then, I've always expected relevant activity to throw a mail in my direction.

Anyway, I'm guilty of these:

5236: Stale, I guess. It took 5 months, but a better way to do it appeared. Which kinda points at the long process being a good thing?

5239: Stale. Something I totally forgot about and never got around to finishing. Doesn't seem like a big thing to do properly after the changes.

[claell]

Since there is no email notification sent out the current way, maybe you can ping the devs by adding them to the first comment on this issue @russiankumar, like I tagged you in this comment. Hopefully that should trigger an email notification so the relevant devs are aware of this.

[russiankumar]

@claell sure, for those PRs which haven't responded I'll do as you say.

@softrabbit, could you clarify on whether you will finish the PRs or not? I'm marking them as stale in case you do want to restart work on them and get them ready for merge.

[tresf]

Ping all authors of all current PRs asking them to wind up by mid/end August

Let's not do this. Just focus on the large, active ones. No need to turn it into a circus

[russiankumar]

Okay, as you say.

[tresf]

@DomClark made a good point on Discord about the formatting PR, that we should pair that with a CI style enforcement-type job to keep the style guidelines from drifting. Might be an important precursor to large refactoring.

[russiankumar]

I think it's better to do that after we merged all the PRs that are going to be merged, then we don't need to redo it each time a PR is merged.

[tresf]

Yes as part of or as compliment of the code formatting PR.

[Cyp]

Didn't know #5304 hadn't been merged. Just fixed some merge conflicts.

[thmueller64]

5502: Ready to review.

5549: Ready from my side, the PR fixes the issue. I'll try to incorporate the suggestions by @Veratil (gradients instead of colors) in time, but those aren't strictly necessary for fixing the issue.

[RebeccaDeField]

5588 is going to be merged within the week, which will, in turn, make #4255 fine to close out (was going to after mine is merged in but if we want to speed things along I guess we can do so now?).

[he29-net]

@russiankumar I just noticed your table says that 4397 waits for 5522 -- I never said anything like that, these are completely unrelated PRs. o_O

4397 is waiting for someone with write access, who can merge my branch (where I finished and fixed the PR) into the branch of the original author (who can't work on it anymore).

[Spekular]

5131 might be a candidate to include in the refactor.

[claell]

Ping all authors of all current PRs asking them to wind up by mid/end August

Let's not do this. Just focus on the large, active ones. No need to turn it into a circus

At least pinging the authors is only fair. Maybe they don't need to wind up their PRs until then, but not notifying them appropriately is not nice imho, especially given the fact that the reason many PRs are still open is partly on LMMS side (no offense).

In addition to notifying, I think it would be nice to make incorporating the made changes to outstanding PRs as easy as possible (as suggested earlier for example by having a bash script that contains all commands needed for the refactor and only has to be applied to the codebase).

[tresf]

but not notifying them appropriately is not nice imho, especially given the fact that the reason many PRs are still open is partly on LMMS side (no offense).

My point was already made in a few words. If PRs are waiting excessive times on reviewers, that's a valid concern, but I'm not sure it's related to pinpointing "large, active ones". 80/20 rule should apply. If that's "not nice", it's not personal, it's just part of large changes. Perhaps to put it more bluntly, no large change ever happens without making someone uncomfortable or upset.

That said, if anyone has PRs that should have been merged a long time ago, it's probably best to start a discussion on #devtalk on Discord. We might be able to promote a few contributors to help review and merge assuming the current devs are onboard with it.

[tresf]

having a bash script that contains all commands needed for the refactor and only has to be applied to the codebase

Note, if there's real conflicts, this is rarely a one-liner but devs are always happy to help over on #techtalk. <3

[claell]

Alright, maybe I got the few words wrong and misunderstood your goal. I guess we agree on notifying the devs in general, then and your point was rather about whether it makes sense to try everything to merge before the deadline, even where the effort might not be worth the benefit.

Regarding the bash script: The idea was to have a script that should apply the refactoring to the branch of a PR and thus resulting in alot less merge conflicts that would need manual tackling.

[tresf]

The idea was to have a script that should apply the refactoring to the branch of a PR and thus resulting in alot less merge conflicts that would need manual tackling.

GitHub has a visual merge conflict resolution that helps, but in my experience, if it's in conflict with GitHub it'll be in conflict on the command line and there's generally no series of small, short commands that fixes it (but rather the painstaking process of hand-editing the conflicts). If I'm wrong, please share.

whether [or not] it makes sense to try everything to merge before the deadline, even where the effort might not be worth the benefit.

Well, as an example, I was tagged for #5510 which is just a few lines and it's something that -- according to the reviewer -- is feature incomplete, so won't get revisited for a while. Rebasing it is isn't a problem at all. I can't speak on behalf of all of them though.

[russiankumar]

@claell actually considering the fact that we're doing more than renaming, we're actually rearranging files in new directories and splitting large files into more files and stuff, I really doubt whether it's possible to have a bash script do that.

I will update the table with updates ASAP, sorry for the delay.

[russiankumar]

Considering that the number of PRs is steadily holding at 80-ish, I'm not very sure if we will be able to resolve them all soon (especially with new PRs still being made). I suppose that it would make sense to make a call on whether to postpone this or entirely focus on closing existing PRs (and actually do as decided), else this change will never happen.

[JohannesLorenz]

Dom and me just finished #5517, feel free to mark it as solved/remove it from list.

[Veratil]

Merged #5253 (finally!)

[russiankumar]

Great, thanks for the ping!

[claell]

If I'm wrong, please share.

We seem to talk about different things here. I was proposing a script that could apply the changes to PRs, so that no merge conflicts (or at least less) would occur.

I agree that there are probably several open PRs that are not ready to merge in the current state and with the author having no time to complete it currently (plus maybe also not so hard to rebase). I also share the opinion that those requests can be skipped then. I only was suggesting to give the authors a chance to get it in by making sure they are aware of this approach and assisting if they want to get it merged beforehand.

considering the fact that we're doing more than renaming, we're actually rearranging files in new directories and splitting large files into more files and stuff, I really doubt whether it's possible to have a bash script do that.

I think it is possible also when it comes to rearrangement of files etc. The question is whether those scripts will also work with all open PRs since their codebase is different. At least for the renaming things I suggest to use a script, though. It still might be useful to also do the rest with a script to ensure reproducible and defined results.

[Veratil]

Merged #5123

[JohannesLorenz]

For many of the above PRs, "Waiting for response" is not too helpful. Most of the users here won't be aware of their own PRs until their names are listed there. For example, did @DouglasDGI realize that he still has (the long awaited) #5000 open (it's really a pain that this PR never got to master...)?

[JohannesLorenz]

Some devs asked if we should get a list of really important PRs. From the PRs that I know:

• ~#5619 and #5536 should be fixed before reorganizing, because they're both large (and both probably easy to finish)~
• ~#5532 can be finished right after #5536 is being finished~
• #4894 maybe
• #4662 - I might need to rebase the PR so (almost) only new files are changed
• #4635 - I need to help @DouglasDGI , maybe @Spekular can help, too
• #4443 - Maybe (review and testing is missing - it has 86 files and is named "Refactoring ...")

Two other PRs with "refactor" in it are candidates:

• #5627
• #5573

[russiankumar]

Most of the users here won't be aware of their own PRs until their names are listed there.

I'd decided not to tag all the devs by the request of @tresf. I can do that if required, though.

Two other PRs with "refactor" in it are candidates

I'm sorry, I think I mentioned 'refactor' in the title in the wrong context... Nevertheless I still think it is an important PR.

[IanCaio]

As per requested on Discord, here are my other PRs besides the one listed on Johannes previous comment:

PRs that refactor code:

• #5660 Improvement to the upgrade routines

PRs that add new features:

• #5581 Adds support for MIDI CC events inside LMMS

The feature PR is one that I consider very interesting for LMMS, because it allows automating some plugins that are currently not automatable (at least not without some really complex workarounds). But recently a bug was found, which I might need some help figuring out, since it is probably related with other areas of the code. Anyways, even though a new deadline wasn't set yet I believe it should be fixed by then. The refactor one is finished, but requires testing.

There's another feature PR open but I didn't bother listing it because it adds a feature that is not consistent in all operating systems.

[tresf]

macOS LV2: #5532 can be finished right after #5536 is being finished

I don't see how it's related to this PR. It's two one-liners, which should be easily mergeable after this is merged.

4027 - I have to finish this. It's open for so long. Shame on me!

Well, it's a lot of trial and error and you're doing a lot :D. I'm also not sure it's warranted prior to this PR since it really only changes a few files (deletes a directory and adds some new upgrade routines). If it's postponed, I don't think it will make the project as a whole suffer. I think it's a trivial PR to rebase if done manually (resetting to HEAD and hand-committing the few minor changes).

Edit: You can assign the post-merge rebasing to me to lessen the burden and speed up this PR.

[DomClark]

Would this be a good opportunity to introduce a namespace for LMMS, as mentioned in https://github.com/LMMS/zynaddsubfx/pull/16?

[JohannesLorenz]

macOS LV2: #5532 can be finished right after #5536 is being finished

I don't see how it's related to this PR. It's two one-liners, which should be easily mergeable after this is merged.

Agreed, but I put this on the TODO list to not forget #5532. It can be done immediatelly after #5536, so having it here won't stop us.

4027 - I have to finish this. It's open for so long. Shame on me!

I think it's a trivial PR to rebase if done manually.

Agreed. I see it now after I cleaned up this branch. I removed it from the above list.

[claell]

Just saw #4893 mentioned. Unfortunately I don't have the time needed to invest into that in the foreseeable future. Feel free to close, takeover or leave stale :)

[russiankumar]

@claell ~considering that renaming FxMixer to Mixer is part of this issue's agenda, and that you have already had some success with it, I'd vouch for keeping it open but stale, so that we have a headstart later on. If anyone is against this idea, you could close the PR.~

On second thoughts, there have been many changes and additions to the FxMixer classes, so it would probably be wise to start over later on. I'll close it for now, and we can reopen it if anything.

[Spekular]

Just noticed the "Create a new branch for all changes" point and I'd like to argue against it. If master is truly frozen there's no benefit to a separate branch. If master isn't truly frozen then working on a separate branch will just delay merge conflicts into the future, worsening them.

[russiankumar]

Sure! I had left it there with a request for discussing it, so if anyone else has anything to say, please do. I myself wouldn't mind either method.

[IanCaio]

The current deadline we have on clearing the PR log was December, but we didn't do anything to prevent new PRs to be submitted. Not sure Github allows closing the PR section temporarily, but if not maybe an announcement that PRs from date X forward will not be merged before the refactor and will need to be fixed later (could even create a label for it, like after-refactor, bug fixes being an exception).

I'm also guilty of submitting a couple difficult PRs to review recently and I'll try to compensate for that by focusing on reviewing from now on instead of writing more stuff :sweat_smile: . But what I mean is that if we are decided towards making this refactor happen soon maybe we will have to take some more active actions to set it in motion.

What I propose is:

• Every dev active on Discord or Github informs us if there's some pending PR they were already working on or something that they wish having merged before the feature-freeze that is not already in the PR log.
• We add it to a list and start the feature-freeze from that point on. Nothing besides the active PRs and those mentioned will be merged before the refactor.
• Set up a new deadline, since I think December might be a bit complicated at this point, focus on reviewing and merging the active PRs and the ones on the list we made earlier. It doesn't even need to be a short deadline, as long as the feature-freeze is happening we would always be getting closer to our goal. The importance of making an announcement (or ideally locking the PR section) is that we'd have to be a bit strict about the new PRs not being included.
• Set the refactor in motion.

I'm a newcomer on collaborating with a bigger project, so I'm not sure the best way to proceed with the division of tasks during the refactor itself. But I'd probably suggest we initially divide the work among the participating devs and have a week of coding followed by one week of peer-review for everyone's weekly progress. Everyone would work on their own pace, but we would be adding the code in the trunk style proposed by @Spekular on the end of the review week and resume work from there.

Since each change is differently spread in the code base, we'd discuss of an appropriate way to do each incrementally before starting work.

[JohannesLorenz]

I would propose January 10 as a deadline, as many coders probably have free days around the new year and this year, probably no one is going on vacation anyways. If we are done earlier, that's no problem either.

Also, I would vote for critical fixes to be allowed (e.g. fixes against sound that may kill your speakers). Such fixes are seldom anyways.

[tresf]

I think you guys are being too kind. Let people rebase. 😈

[Spekular]

Not sure Github allows closing the PR section temporarily

Doesn't look like it, but maybe something like Repo Lockdown would work if it can be configured to only reject new PRs. It'd be annoying to go through and re-open the valid ones though, so in the end maybe it's better to manually close invalid ones.

Perhaps we could (temporarily) update the README and some of the wiki pages to alert people about feature freeze? Maybe even the "feature request" issue template.

Also, I would vote for critical fixes to be allowed (e.g. fixes against sound that may kill your speakers). Such fixes are seldom anyways.

Sounds reasonable to me as well.

[IanCaio]

Also, I would vote for critical fixes to be allowed (e.g. fixes against sound that may kill your speakers). Such fixes are seldom anyways.

For sure, I meant to be broad when I said bug fixes, I'd consider that one! I had to replace my laptop speakers recently and I DO NOT want to have to go through that again :laughing:

I think you guys are being too kind. Let people rebase. smiling_imp

Just don't make me rebase #5712, I beg you! :rofl:

Doesn't look like it, but maybe something like Repo Lockdown would work if it can be configured to only reject new PRs. It'd be annoying to go through and re-open the valid ones though, so in the end maybe it's better to manually close invalid ones.

Perhaps we could (temporarily) update the README and some of the wiki pages to alert people about feature freeze? Maybe even the "feature request" issue template.

I see, that would probably be a bit of a hassle. We should make a very visible announcement then, on both discord and here on Github (wiki pages like "How to contribute" and maybe the root README.md) and create that after-refactor label so we can add them to the PRs that get submitted after the announcement anyways.

[tresf]

I strongly disagree with any proposal that attempts to manage a large refactor by blocking PRs. Feature freezes are for merging, not for proposing change.

• Some believe the style change should go in first.
  
  
  
  • If that's the case, do it now.
  
  
• Some believe some larger PRs should be wrapped up before this is merged.
  
  
  
  • This was proposed in July. It's too late, they'll have to be rebased.
  
  

Most large PRs are done by people that are perfectly capable of refactoring and if they're not -- a dev can take an hour to rebase the valuable ones in the event that the OP won't. This is what @Sawuare and I did over at https://github.com/LMMS/lmms/pull/4232. It's tedious, but it's not difficult.

As far as I'm concerned the only people that get to hold this up are people that are very active in the PRs area and even then, the cutoff should be in days, not weeks.

If you really want to, we might be able to create a PR template that puts up a warning about this effort, but please, no more. Don't discourage help.

[tresf]

I'd consider that one! I had to replace my laptop speakers recently and I DO NOT want to have to go through that again 😆

I feel this is an edge-case that shouldn't drive decisions in this PR. If a speaker is damaged because of bad DSP code it can and will happen again, especially when it's 3rd party code. From what I understand this was testing LV2 and one plugin went haywire (#5767). I don't know how we can avoid this and it sucks it happened, but the project has money to help compensate you for your equipment loss. Perhaps In the future we can have an fx filter on master which avoids speaker-damaging audio, but that seems like a separate topic.

[IanCaio]

Valid points, I didn't think about the side-effect of discouraging new collaborators with the PR lock suggestion. We can go through with the announcement + labeling idea, if everyone agrees with it, so at least we know what is to be merged before and after the refactor.

I feel this is an edge-case that shouldn't drive decisions in this PR. If a speaker is damaged because of bad DSP code it can and will happen again, especially when it's 3rd party code. From what I understand this was testing LV2 and one plugin went haywire (#5767). I don't know how we can avoid this and it sucks it happened, but the project has money to help compensate you for your equipment loss. Perhaps In the future we can have an fx filter on master which avoids speaker-damaging audio, but that seems like a separate topic.

Oh, my blown speakers were completely unrelated to this issue! I'm really sorry for the misunderstanding, I was just joking about how hard it is to replace a laptop's speaker :flushed:

[M374LX]

4894 is ready for review.

[M374LX]

Another file needing to be refactored is main.cpp. I also suggest moving it from src/core to src not only because this would give more visibility to the file in which the execution of the program begins, but also because the file inevitably depends on code from both core and gui, even in an ideal world where core is fully independent from gui.

[JohannesLorenz]

Sorry for the close, I was just tired and added Fixes 5592 into the commit message of #5806 . Reopened.

[M374LX]

While there was previously a convention to leave four blank lines between each method implementation, the current conventions do not say anything about the number of blank lines.

I think one blank line between methods of the same class is enough, while we could use possibly two between different types of declarations on a file (like includes, forward declarations, and method implementations) and also between methods of different classes implemented in the same file. Example:

#include <QApplication>
#include <QWidget>

#include "foo.h"
#include "bar.h"


class Class1;
class Class2;


void Class1::method1()
{
}

void Class1::method2()
{
}


void Class2::method1()
{
}

void Class2::method2()
{
}

In the case of headers, I think one blank line between classes is enough:

class Class1
{
    void method1();
    void method2();
};

class Class2
{
    void method1();
    void method2();
};

[JohannesLorenz]

As there are new formatting PRs (whitespace fixes, variable renames ...) recently: I think we should stop writing new formatting PRs before we do the clang-format runs. It's only duplicate work and we want to avoid new PRs anyways.

[russiankumar]

Pinging for some things that are still open for discussion:

• Reorganise all files into better places & folders
  
  
  
  • What are the possible folders to be made?
  
  
  • Should we update the include folder? If so, how should it be organised?
  
  
• Clear out space for renamings
  
  
  
  • Are there any changes that need to be done in order for the mentioned renamings to be done?
  
  
• Moving to C++17
  
  
  
  • Is it worth it? What changes, if any, will have to be made to the codebase?
  
  

Feel free to add things to this comment if there's anything you need to be discussed.