-
-
Notifications
You must be signed in to change notification settings - Fork 992
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.
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
Move as many headers as possible out of the include folder #7341
base: master
Are you sure you want to change the base?
Conversation
This PR touched some submodules as well, we don't want to include those changes. Additionally, I believe when discussing moving the headers it was to alongside the source files (same folder as), and not just into an Also there is a merge conflict. You should rebase the changes on master. |
I do appreciate your PR and also the fact that |
Thanks for the comments! @Rossmaxx whenever this fits your schedule. You guys are making a lot of nice improvements and I don't want to step in at an inconvenient time. I agree this effort will take more than moving files around, but wanted to keep changes to a minimum so that it is not painful for review. I thought it would be nice to only move here, as no bugs would be introduced this way and it would still be a step that would get us closer to the work in my prototype. Working on my proof of concept is fun, but my goal was to prove this route for separation had some merit, and now that that's done I feel continuing to build upon that dirty code might be a bit of a waste of time... Another idea I had was to replicate the prototype on top of latest master but in a simpler and safer way, but I might spend too long and then deviate too much again 😅 |
@jsmtux i have another proposal but it might be counter intuitive. Would you mind maintaining a seperate fork till we release a 2.0 (android only because we don't want to become competition) ? And you can upstream any improvements, but keep the rearrangement stuff within your fork till we open it up. We will tell you and we can merge your project in at a convenient time. |
Isn't core/gui separation exactly the kind of thing we'd want included in a 2.0 release? Either way, maintaining a separate branch indefinitely while waiting for something as big as 2.0 is a huge amount of effort, and I think it makes much more sense to merge gradual steps like this as early as possible. |
ba826b0
to
776c66b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I felt looking at the PR.
@@ -720,7 +720,7 @@ endif() | |||
# people simply updating git will still have this and mess up build with it | |||
FILE(REMOVE include/lmmsconfig.h) | |||
|
|||
FILE(GLOB LMMS_INCLUDES "${CMAKE_SOURCE_DIR}/include/*.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's a good idea to have the headers in the same folder as the sources. No seperate src/xyz/include
needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead everything could go to src/xyz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I see it src/xyz/include
is a very important distinction, because it marks what can be used from the outside. Let's imagine they were already separate static libraries (which will happen at some point), then in the cmake we'd do something like
ADD_LIBRARY(lmms_core STATIC <all the cpp>)
TARGET_INCLUDE_DIRS(lmms_core PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
That way, all elements that will need to be used from gui
would go into include
whilst the rest would just go into src/core
. If you have a look at my proof of concept branch you'll see that the exported headers were part of a separate library lmms_core_interface
, but anyhow this ends up being, I think it is important to separate the headers that are used internally to the target and those that are used from outside.
My plan for the follow up PR will be to start moving everything that is possible out of src/xyz/include
into src/xyz
. Especially for gui
, we will be able to start moving most of the headers for src/gui
. The reason I didn't start here is because moving let's say ClipView.h
to src/gui/clips
involves changing all the includes from
#include "ClipView.h"
to
#include "clips/ClipView.h"
and I didn't want to change any code for this first PR.
What I'll try to do as soon as I get some time is to do that next part I just described so that you'll be able to see both PRs. I think that be the last step where I'll need to move a lot of files around, so I guess it'll be nice to do both parts as close as possible. If you feel like it'd be nice to do right away I can do everything on this PR, though.
@@ -131,5 +131,12 @@ set(LMMS_SRCS | |||
core/midi/MidiPort.cpp | |||
core/midi/MidiWinMM.cpp | |||
|
|||
core/tracks/AutomationTrack.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous PR you made, I saw you created seperate cmake lists for core/*
, which might help in cleanup, though the splitting up of targets, that's for further future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, i believe tracks
belong to the gui/tracks
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About tracks
, it is a bit problematic indeed. All the elements in tracks
inherit from Track
. Song
and PatternStore
are TrackContainers
which contain a std::vector<Track>
forming the backbone of the core library.
However, as you said, there is part of each of these track types that does belong in gui
which is
virtual gui::TrackView * createView( gui::TrackContainerView * view ) = 0;
That's indeed one of the parts that will need some consideration when doing the core
/gui
separation. In my proof of concept I used TrackViewFactory
to uncouple it, as you can see in TrackViewFactory.cpp. The idea is that whenever the gui needs to display a track of a given type it now calls track->createView()
on it whereas in the future it will do something like trackViewFactory->createView(track, trackContainer)
. Probably we won't end with the same solution this time around, but the important is that no element from the gui
namespace resides in core
anymore.
For the moment I'd vote for keeping this class inside core
, as well as all other elements that do common things even if they need to get some parts removed sooner or later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About separate cmake lists... I'll do that 👍
776c66b
to
564bbe0
Compare
Headers are moved to src/core/include or src/gui/include where appropriate. This is the first change towards a separation between gui and core. No code changes are present in this commit. The output binaries should be equivalent to the previous commit.
564bbe0
to
a16c322
Compare
In response to your comments I've gone a bit further with the change. I think it is good to open the discussion, but the last commit could be reverted in any case. |
a16c322
to
d5a98ad
Compare
Headers are moved to src/core/include or src/gui/include where appropriate. This is the first change towards a separation between gui and core.
No code changes are present in this commit. The output binaries should be equivalent to the previous commit.