Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592
Comments
|
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. |
|
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 |
|
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. |
I'm pretty sure every PR will be affected considering the size of these changes
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. |
|
With regards to my PRs:
|
|
Updated, thanks! |
|
In regards to this
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. |
|
Yeah, it definitely is HUGE. How about three files (+ includes): |
|
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. |
Maybe we can come up with one or several bash scripts that contain the relevant commands. |
|
Do approvals from people without write-access count towards mergeability? Github says not but maybe the mods here take it differently? |
|
My PRs:
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:
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. |
|
My PRs:
|
@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:
|
|
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. |
|
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: |
|
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. |
|
@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. |
Let's not do this. Just focus on the large, active ones. No need to turn it into a circus |
|
Okay, as you say. |
|
@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. |
|
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. |
|
Yes as part of or as compliment of the code formatting PR. |
|
Didn't know #5304 hadn't been merged. Just fixed some merge conflicts. |
Aren't they done, or are there anything else left? |
|
Ooopsie! |
|
I think its about time to update the stable version to the nightly, its been years since that's been done and most Lmms users use the nightly version anyways, which many repos don't offer. actually, I don't think any repo offers the 1.3.0 build and that's super annoying for many of us. |
|
Yes, the nightly version offers better performance and the GUI is somehow crisper, less blurry. I use the nightly myself, and gave people the 1.3.0 installer when they ask me for a free music studio software. |
i didnt notice any changes in performance i just use it for the features |
Agreed. But back to the topic, what's the plan for the reorg now? I know there's many changes underway related to decoupling qt from core and other big low-level cleanup efforts that aren't part of this reorg really. Devs haven't been sleeping. There has probably been lot of discussion on discord, but I wanted to ask here. So what's the plan? Should we consider the reorg over and see if something can be done with the stale PRs. Ping the authors? Or should we just forget about that bullet point and let authors update their PRs in due time if they still have any interest? Is there anything left that I'm forgetting? |
|
Hmm, After allejok96 asked about what the plan is, it's been almost three months... Did the maintainers have forgotten this issue? Hope to see progress on this, everyone expects an update. |
Not a maintainer, but it seems as if the reorg is essentially complete. The team was held back a bit by a couple of clang-tidy PRs, but #6564 should be the last one, as I haven't heard from @JohannesLorenz or @irrenhaus3 about adding anymore. The plan after reorg is anybody's guess, but there are specific goals we have in mind, such as decoupling Qt from the core, as previously mentioned by allejok. I personally have ideas to start refactoring The maintainers most likely did not forget about the reorg, but I agree that an update on the plan forward would be ideal. |
|
I just went through all checks from clang-tidy that I would like to add, and non of them will have reorg-scope. So I am OK with finishing the clang-tidy-checks (and reorg). @allejok96 @DomClark @PhysSong @Veratil do you have any more wishes for clang-tidy-checks or re-org in general? Otherwise I would say we can finish it. |
|
If everyone's happy with the clang stuff I'd say lets move on. The main focus of our devs will probably still be related to refactoring and rewriting code, but IMHO this huge checklist can be closed. I don't know what that will mean for the post re-org PRs, but I find it hard to believe features will come raining down once we officially end the re-org, although it would be a welcome change 😄 As I see it the bottleneck of LMMS development hasn't been as much related to the re-org, as it's been to the lack of people with enough spare time and commitment to fix our big painful issues and - maybe even more important and boring - lack of people with the skills needed to review everything... Boring-code devs, this is for you 🏆 And for all you eager folks who's been sitting around and waiting for this to finish: don't get your expectations up too high. Instead read up on coding, and help us out in whatever way you can |
|
I'm happy to move on. I have no objection to applying more clang-tidy fixes in the future, but don't feel the need to keep them as part of this one ongoing effort. However, one thing I think would be worth doing is adding CI checks for the clang-tidy fixes that we have already applied. Otherwise there's no way to guarantee that we're still following those rules and we risk needing to do this all over again in the future. |
|
I also agree with @DomClark's suggestion. |
|
I will write a PR for the suggested |
#6619 . Would be nice if someone could review this one-file-PR, so we can finish re-org 😁 |
|
So, there is no more code to change for reorg 🎉 We are currently discussing in Discord (dev-only) how to proceed with the "Reopen code for fresh PRs (or rebased old ones)" point. |
|
Following our discussion on Discord, this re-org is now complete, and this issue can be closed. Congrats to everyone who worked on this project! 🎉 |
|
Does it mean that "track recording" (aka #3947) is now enabled ? |
|
Nope : cf #5990 (comment) |
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:
dos2unix#6198, must be done prior to clang-format)modernize-use-equals-delete (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-delete.html)no occurrences in our code.clang-tidy(Update.clang-tidy#6449)MidiTime->TimePos(Rename MidiTime to TimePos #5684)TCO->Clip(Rename TCO and related to Clip #6226)Pattern->MidiClip(Rename TCO and related to Clip #6226 + PR for data file tag)*TCOand*TCOView->*Clipand*ClipView[* stands for any suffix eg. BB, Automation] (Rename TCO and related to Clip #6226)BB*->Pattern*(Rename BB to Pattern #6284)DataFile(Rename BB and TCO to clip and pattern in save files #6309)Mixer-> ? (Discussion at New name for "Mixer" class #6089)FX-Mixer->MixerTrack.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)LedCheckbox.htoLedCheckBox.h(the class is namedLedCheckBox), and using a common file naming scheme forstereo_enhancer...vsstereoenhancer...(same forstereo_matrix)Move Mixer, Controller Rack, and Project Notes windows to theremoved since this folder was already well defined (Editor.cpp and subclasses)gui/editorsfoldergui/dialog. Delete the now-emptygui/formsfolderwidgetsfolder (Possibly covered in Move widget files, add folder for track and instrument #6374)instrumentclassestrackclasseslmms#6174 is ready to review, discussion at C++ Namespace layout #6086, an example)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
The text was updated successfully, but these errors were encountered: