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

[ryuukumar]

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:

• Freeze code on December 5th, 2020
• Clear PR backlog by January 25th, 2021
  • Ping devs of older PRs via the table in this issue
  • Review and test PRs to be merged before reorganization
    • MIDI range MKII (extracted from microtonal PR) #5868 MIDI Range
    • Support for alternative tunings and keyboard mappings #5522 Microtuner
    • Play automation pattern when midi controller connected #5657 Play automation pattern with controller
    • Feature: Carla automation  #5846 Carla Automation
    • Improved Oscillators -- fixed #5826 Improved Oscillators
    • Adds support for local paths and project bundles #5735 Local paths and project bundles
    • Upgrade the Flanger effect #5873 Update flanger effect
    • Automation Node fine tuning with double-click (Rewrites #5292) #5923 Automation fine tuning with double click
• Format code...
  • with format PRs (Fixes style on "SampleBuffer.cpp" and "SampleBuffer.h" #5791, request to not push any more such PRs)
  • with dos2unix (Run dos2unix #6198, must be done prior to clang-format)
  • with "include-what-you-use" (Remove useless includes (using include-what-you-use) #5860)
  • with Clang + CI (Add .clang-format and .clang-tidy #4690, comment, latest approach at Apply clang-format #6323) - It has been agreed on not doing an automatic format, but leave the decision to the authors of individual PRs
  • However, we do agreed on a few special clang-tidy fixes:
    • modernize-redundant-void-arg (https://clang.llvm.org/extra/clang-tidy/checks/modernize/redundant-void-arg.html)
    • modernize-use-bool-literals (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-bool-literals.html)
    • modernize-use-emplace (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html)
    • modernize-use-equals-default (clang-tidy: Apply modernize-use-equals-default everywhere #6450) (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
    • modernize-use-equals-delete (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-delete.html) no occurrences in our code
    • after modernize-use-equals-default has run, follow it up with performance-trivially-destructible (https://clang.llvm.org/extra/clang-tidy/checks/performance/trivially-destructible.html)
    • Update .clang-tidy (Update .clang-tidy #6449)
  • Get rid of the most clazy warnings. Currently, only Fix clazy warning: "Signature is not normalized." #6445 .
• Rename PRs
  • Rename MidiTime -> TimePos (Rename MidiTime to TimePos #5684)
  • 'Clip' rename
    • Rename TCO -> Clip (Rename TCO and related to Clip #6226)
    • Rename Pattern -> MidiClip (Rename TCO and related to Clip #6226 + PR for data file tag)
    • Rename *TCO and *TCOView -> *Clip and *ClipView [* stands for any suffix eg. BB, Automation] (Rename TCO and related to Clip #6226)
    • Rename BB* -> Pattern* (Rename BB to Pattern #6284)
    • Adjust DataFile (Rename BB and TCO to clip and pattern in save files #6309)
  • Mixer rename
    • Rename Mixer -> ? (Discussion at New name for "Mixer" class #6089)
    • Rename FX-Mixer -> Mixer
  • Rename any other remaining editors
• Other major changes
  • Split track classes into one file per class
    • Track.cpp (Split Track.cpp and Track.h #5806)
    • AutomationTrack.cpp (Split AutomationTrack.cpp into multiple files #5983)
    • BBTrack.cpp (Split BB Track classes into separate files #5985)
    • InstrumentTrack.cpp (Wait for Support for alternative tunings and keyboard mappings #5522)
    • Pattern.cpp (Split Pattern classes into separate files #5986)
    • SampleTrack.cpp (Split Sample Track classes into separate files #5987)
  • Reorganise all files into better places & folders (requires some discussion Source File Reorganization #5944)
    • Rename LedCheckbox.h to LedCheckBox.h (the class is named LedCheckBox), and using a common file naming scheme for stereo_enhancer... vs stereoenhancer... (same for stereo_matrix)
    • Move Mixer, Controller Rack, and Project Notes windows to the gui/editors folder removed since this folder was already well defined (Editor.cpp and subclasses)
    • Move dialogs (that open in an external window) into gui/dialog. Delete the now-empty gui/forms folder
    • Organize widgets folder (Possibly covered in Move widget files, add folder for track and instrument #6374)
      • Folder for instrument classes
      • Folder for track classes
      • Organize remaining classes by reusability or area of use?
  • Add a namespace for LMMS (PR Namespace lmms #6174 is ready to review, discussion at C++ Namespace layout #6086, an example)
• 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.

[ryuukumar]

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.

[ryuukumar]

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:

• Improved relative paths #5117 isn't stale, I intend to get it merged before this reorganization (edit: it's merged!)
• Keyboard shortcuts to preview/add sounds from sidebar #5427 is a draft so it can stay marked as stale
• Knife Tool for Sample Clips (Again) #5524 is in limbo. Its pretty much in the same state as Improved relative paths #5117, it's just less important to me to get merged

[ryuukumar]

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.

[ryuukumar]

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.

[ryuukumar]

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

[IanCaio]

My PRs:

• Adds a feature that captures the keyboard on the Piano Roll #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.
• Changes the behavior of "solo" so it doesn't mute Automation Tracks #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).
• Adds support for MIDI CC events inside LMMS #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.