search

OpenOrienteering / mapper

OpenOrienteering Mapper is a software for creating maps for the orienteering sport.
https://www.openorienteering.org/apps/mapper/
GNU General Public License v3.0
399 stars 106 forks source link

Refactoring #740

Open 02JanDal opened 8 years ago

02JanDal commented 8 years ago

Quite some time since I last touched the source of this project...

Since these would be quite big changes I want to discuss them a bit before I even start to consider doing any of them:

I believe that Mapper is in dire need of some refactoring. All the below is done on latest master at the time of writing (c0713b5d343bfaf9ed2ab1c89dcf7ccff7a03fdf).

Files

Additionally there are many files that currently contain way more than a single class, object.h for example contains 2 large, 2 medium and a few smaller classes. These should probably also be split into separate files, which might change how the directory layout should be made.

Classes

The list goes on and on.

There are also many classes that have their own save/load functions, it might make sense to extract this functionality too.

There are also several cases of classes that should probably be in part put into a QAbstractItemModel subclass. Most of the *Container classes suggested above would make sense for this, or at least have a model class each that can be used to view them. Both TemplateListWidget and ColorWidget contain a QTableWidget that they need to keep updated, additionally to doing their actual job: handling the UI. The model handling part in these classes should be replaced by QTableView with a QAbstractItemModel subclass.

Summary

Doing all this is a huge amount of work, especially splitting the classes. But I think it would be worth it. Doing this not only would improve the code base and improve build times, but also bring new features almost for free (adding map parts to the template widget as mentioned in #721 would take no more than maybe an hour or so once the template list is a model).

Once the mentioned changes are through there will likely be more patterns visible, yielding more things to refactor, but these big boulders need to get out of the way first.

I'd be happy to help out on whatever parts of this I can (will take me a while to get into the code base again), but first I want to know your thoughts on this.

Abbe98 commented 8 years ago

This would also make it a lot easier for possible new contributors to get started :+1:

dg0yt commented 8 years ago

There is no question that a major refactoring of some classes would be a good thing. But I guess you still underestimate the required effort (like probably both of us underestimated the effort of cleaning up after merging the map parts pull request).

The major challenges are

Indeed there is no point in refactoring without more automatic tests, and writing tests first is a good way to understand and document what is going on.

There are some lower hanging fruits to start with, such as

02JanDal commented 8 years ago

Well, the first steps, splitting and moving files, shouldn't alter any behaviour (unless there's code that depends on in which file it is placed, which I really, really don't hope there is), so that should be doable without tests. Once that's done we have smaller units (less classes per file at least), so writing tests should be somewhat easier at that point.

For writing tests; would including and using Catch be possible? It's assertions are way nicer to work with than those in QtTestLib, though it's not as integrated into Qt (most stuff still works though).

Also, to actually make the current tests useful for refactoring they should be checked through, autosave for instance takes almost 30s...

On me underestimating the work behind this: possible, but I don't think it should be done all at once. Small steps (split/move files, more tests, refactor code related to the tests, more tests, refactor code related to those tests, etc.).

02JanDal commented 7 years ago

So while #822 slowly moves forward, a list of stuff I want to do in the near future (in rough order):

The following would be roughly per directory, so would likely result in several branches/PRs in parallel

Another thing that might make sense eventually (not yet, I want to see it become a bit more mature) is replace the current 3rdparty dependency handling by conan, but that is to far into the future to make this list.

@dg0yt Any thoughts on the above?

dg0yt commented 7 years ago

Basically, I am reluctant to add more 3rd-party modules without clear benefit. And some changes really only make sense if there is more than one developer.

Give LaurentGomila/qt-android-cmake or the files included in ECM a try in an attempt to replace QMake, thus being able to clean up the build system.

I do not have much trouble with qmake. In the past, this was the most convenient way to try different "kits" in Qt Creator. OTOH I would appreciate Android being fully supported from CMake.

Do we currently support iOS (there are similar projects for it, but I don't own a device for testing that)?

No, and until yesterday I would have said it does not make much sense: Apple won't let GPL software into the app store, and thus it would be effectively impossible to distribute the app. However, there seems to be a new law suit challenging Apple's exclusive role in this game...

Add support for generating code coverage information to the build system using lcov/llvm-cov

I'm fine with that.

Set up Travis-CI (AppVeyor maybe in the future) and either codecov or coveralls for coverage upload (any preferences for which coverage service?)

openSUSE Build Service is my CI. Given the range of linux distributions covered, it catches a lot of issues. But its for packaging, it is not the kind of service Travis-CI is.

Include Catch for writing tests

What will we gain?

Add more unit tests (I'll say at least 50% coverage as an arbitrary goal, will likely need to be adjusted once we see how much coverage there already is)

That's a lot of work...

Split files, generally one class per file

Fine.

Other refactors (possibly separate PR from the above two)

Generally you will need a lot of PR, or every other work has to stop, or your big fat PR cannot be applied.

Replace Q{Tree,List,Table}Widget by real model/views

The focus should be on improving the core model first, not the UI.

Split large classes

... after having the unit tests

Another thing that might make sense eventually (not yet, I want to see it become a bit more mature) is replace the current 3rdparty dependency handling by conan, but that is to far into the future to make this list.

I looked at conan shortly. I'm not conviced I would make use of it. There is work in progress on bootstrapping the 3rd-party dependencies from a separate CMake superbuild project. Yes, just CMake, everything else can be pulled in. I need to solve a different problem: managing different trees of source code and binary results efficiently, including parallel builds, over a long period of time.

02JanDal commented 7 years ago

Basically, I am reluctant to add more 3rd-party modules without clear benefit. And some changes really only make sense if there is more than one developer.

We'll at least for a few things there's more than one now :wink:. Additionally changes that don't necessarily help a solo developer will likely help others who are interested in contributing but haven't yet.

Give LaurentGomila/qt-android-cmake or the files included in ECM a try in an attempt to replace QMake, thus being able to clean up the build system.

I do not have much trouble with qmake. In the past, this was the most convenient way to try different "kits" in Qt Creator. OTOH I would appreciate Android being fully supported from CMake.

I can fully see the benefit of continuing to use qmake. Maybe at least reduce the parts built by it to a single main.cpp file and let the rest be a library? The reason I'd want to do this is because right now I don't really feel comfortable changing stuff in the build system for the risk of breaking the qmake part (there already was a smaller issue with that in the first PR), additionally it'd open up for putting some stuff into their own libraries (or at least own CMakeLists.txt) in the future.

Set up Travis-CI (AppVeyor maybe in the future) and either codecov or coveralls for coverage upload (any preferences for which coverage service?)

openSUSE Build Service is my CI. Given the range of linux distributions covered, it catches a lot of issues. But its for packaging, it is not the kind of service Travis-CI is.

Yeah, the reason for Travis would be checking PRs, uploading of coverage etc. This might be possible to set up with OBS though, but that's not something I'd be able to help with.

Include Catch for writing tests

What will we gain?

A nicer way to write tests (slightly less code overhead since no class, WAY better assertions (you just write "normal" boolean expressions (a == b) and it'll also print out the values of the variables). It's also distributed as a single header, so trivial to include.

Add more unit tests (I'll say at least 50% coverage as an arbitrary goal, will likely need to be adjusted once we see how much coverage there already is)

That's a lot of work...

As I said, arbitrary, and one directory at a time.

Other refactors (possibly separate PR from the above two)

Generally you will need a lot of PR, or every other work has to stop, or your big fat PR cannot be applied.

Ofc, but depends on how big the changes in each area are.

Replace Q{Tree,List,Table}Widget by real model/views

The focus should be on improving the core model first, not the UI.

The issue is that the code for the core is currently in the UI code. Doing this would move business logic to core, where it belongs.

Split large classes

... after having the unit tests

Yes, all changes that could result in changes of behaviour (both in code and to the user) will have at least a decent amount of unit test coverage.

Another thing that might make sense eventually (not yet, I want to see it become a bit more mature) is replace the current 3rdparty dependency handling by conan, but that is to far into the future to make this list.

I looked at conan shortly. I'm not conviced I would make use of it. There is work in progress on bootstrapping the 3rd-party dependencies from a separate CMake superbuild project. Yes, just CMake, everything else can be pulled in. I need to solve a different problem: managing different trees of source code and binary results efficiently, including parallel builds, over a long period of time.

I haven't quite understood what this superbuild thingy is for, nor why it is needed, but I'll try to keep my hands of this area for now.

dg0yt commented 7 years ago

Give LaurentGomila/qt-android-cmake or the files included in ECM a try in an attempt to replace QMake, thus being able to clean up the build system.

I do not have much trouble with qmake. In the past, this was the most convenient way to try different "kits" in Qt Creator. OTOH I would appreciate Android being fully supported from CMake.

I can fully see the benefit of continuing to use qmake. Maybe at least reduce the parts built by it to a single main.cpp file and let the rest be a library? The reason I'd want to do this is because right now I don't really feel comfortable changing stuff in the build system for the risk of breaking the qmake part (there already was a smaller issue with that in the first PR), additionally it'd open up for putting some stuff into their own libraries (or at least own CMakeLists.txt) in the future.

If we don't proceed too fast, the qmake changes can follow the CMake changes. It is a second-class citizen.

Set up Travis-CI (AppVeyor maybe in the future) and either codecov or coveralls for coverage upload (any preferences for which coverage service?)

openSUSE Build Service is my CI. Given the range of linux distributions covered, it catches a lot of issues. But its for packaging, it is not the kind of service Travis-CI is.

Yeah, the reason for Travis would be checking PRs, uploading of coverage etc. This might be possible to set up with OBS though, but that's not something I'd be able to help with.

Any reasonable integration with all these services will need Travis. I just won't touch it until I fully understand the implications, especially when it comes to granting privileges. (I looked into it a few times but wasn't satisfied. Cannot say if information was too poor or I was too tired.)

Include Catch for writing tests

What will we gain?

A nicer way to write tests (slightly less code overhead since no class, WAY better assertions (you just write "normal" boolean expressions (a == b) and it'll also print out the values of the variables). It's also distributed as a single header, so trivial to include.

I have looked at the web page, and I still asked the question. I do see some real benefits, but not the items you mention... Not sure how much you worked with our tests so far.

02JanDal commented 7 years ago

Regard Catch; well, I wouldn't rewrite the existing tests (at least not for now, way later if ever), but I've generally felt with Catch that you can focus more on writing tests and less on boilerplate and syntax.

dg0yt commented 7 years ago

How to add a new feature with minimal code in MapEditorController: b2d598e. PIMPLing MapEditorController is still needed in order to decouple feature units from map_editor_controller.h.