Cutting into multiple headers

[MCWertGaming]

This pull-request resolves #67 and cuts terminal.h and base_terminal.h into multiple smaller headers.

Things that need to be done:

• cut headers
• move source code into source files
• fix windows includes
• remove not required includes from files for windows
• test macos and windows

[certik]

I would recommend not to lump too many things into this PR, otherwise it will be very hard to review and make sure things work at the CI. So let's just do the "cut headers" here, make sure all CI tests pass, we'll review it and get it in. Then we'll implement the next task in a new PR.

The Windows test is failing currently.

[MCWertGaming]

Yes, I have removed that part of my todo and will move that into an issue. I have currently split everything into 4 headers and moved some static funktions out of the BaseTerminal class. Nothing big. After a small cleanup of the windows includes, this PR is ready.

[certik]

Awesome. Please ping me once tests pass.

[certik]

Here is my idea what the goal should be to refactor the header files:

• Create cpp and header files
• The cpp file should have all the implementations, the header file should be just the declarations
• All platform specific code (i.e. all ifdefs) should be in a cpp file and preferably just one small cpp file. The rest of cpp files and all header files should not contain any platforms specific stuff.

The current master is close to this ideal, i.e., the base header file is small and contains platform specific stuff, the main header file contains most of the functionality and is platform independent. We just have to move things into cpp files.

[MCWertGaming]

Yes, I have moved most things into source files. I think it would be possible to move the platform dependent code into the tools.hpp. I haven't moved the input.hpp into a source file yet, because It would take ages and will make it difficult to cut it on the next step. I would just let that one how it is and work on that later. I have used #pragma once because #if def would create an include guard per file. I don't think that there will be any conflicts with other libraries so I could just change that if you prefer that.

Also i'm working on fixing the cpp-terminalConfig.cmake file, because it's not referencing the source files. Do you have more experience in that? I don't get how i'd make it work. Wouldn't it be enough to just do a add_subdirectory(), or is the cmake module important? @certik

When that's done, i'll look into fixing the windows includes to make the integration tests working again.

[certik]

I never know how to do cmake off top of my head, you can try to google it, otherwise if you get stuck let me know and we can have a look at that together to figure it out.

[MCWertGaming]

I have fixed cmake now. The last thing now is to fix the windows includes. I would create an issue on the platform dependent stuff to keep this PR small.

[MCWertGaming]

This branch is now ready @certik.

[MCWertGaming]

@certik how is the state on this PR?

[certik]

I forgot it was ready, sorry about that. Let me review this soon. I need to check the branch out and test locally.

[MCWertGaming]

So, What's the state on this PR? @certik
We could also make a fork of this repository, I mean #122 showed that some people might prefer having a project with just two headers instead of a whole cmake managed project with source files and all the other stuff. I would also like to recreate some parts of the code to move all system dependant parts (especially all ifdef's) into a local private function and all internally used functions into inline functions as that would decrease the overhead of calling functions.

What do you think? Rather keeping this simple and creating a more complicated version, or just further developing this into something more complicated?

[MCWertGaming]

Complicated in terms of the build system, having source and header files means that you should consider using it as an cmake sub project while you can currently just copy the two headers and integrate them easily into your build system even if you don't use cmake at all (like make or something like that).

Also that wouldn't mean that here is no work ongoing, I would port back most things, credit you and we can do it together if you want.

[certik]

Hi @MCWertGaming I didn't have time to review immediately and then forgot about it. I am sorry.

I want to get somebody to review, for a big change like this. Let me try to get to it this week. We can maybe also meet over video and review together if you have time.

[MCWertGaming]

No problem for taking you time, it's just that I was not sure what the state on this one was. Great thing if some other people look over this, it's not ready yet anyway as some things need further work, like one header is not splited yet and all system dependant code is not moved into a seperate header yet. Maybe we shoudln't merge it into the master branch but into something like cppterminal-next? Like a temporary "master" until the base is finally ready?

Also we can surely review this PR together! I have still holidays this week so I have much time, Next week goes only the evening because of my school schedule. But we will definietly find a day! Just send me an email.

Thank you really much!

[MCWertGaming]

I would also like to discuss some other things like replacing the current unit testing with gtest and simplifying the BaseTerminal and Terminal classes / the UTF8 conversions.

[certik]

I sent a private email to setup a video meeting to get this finished.

[MCWertGaming]

Sure!

[certik]

Now when #124 is in, you can now pick a part of this PR, and create a new small PR that we can review and merge. And step by step we can get most of these changes in.

[MCWertGaming]

sure!

[MCWertGaming]

looks like there are no other relevant changes. closing this.