search

juzzlin / Heimer

Heimer is a simple cross-platform mind map, diagram, and note-taking tool written in Qt.
GNU General Public License v3.0
889 stars 112 forks source link

Added: 'range-v3' library #201

Closed tcoyvwac closed 2 years ago

tcoyvwac commented 2 years ago

The range-v3 library adds a lot of features which eases the development for Heimer.

It is a Boost-licensed, header-only library and range-v3 is the main basis of the std::range functionality in C++20.

A ranged-based method of processing vectors and looped-code helps minimise errors due to the encouraged use of "piping" to process data.

An example of how the range-v3 library makes the Heimer code much easier to understand & streamlined is in:

As Heimer is keen to use the latest C++ coding standards (shown by commit: [d5956e0]) and more of the C++ standard library in its codebase (Issue: #200), range-v3 is a sustainable choice to adopt for this project until it can be converted to its native C++20 format.

I am happy to answer any questions as best as I can about this PR, and if needed, happy to add extra changes. :smile_cat: Thanks!

tcoyvwac commented 2 years ago

The PR may have issues with CMake and including the range-v3 library into the codebase. Any preferred advice while keeping the "contrib" method of external libraries, would be helpful & added. :smile_cat: I cannot access the project's CI logs, so please post any errors created so I can fix them.

juzzlin commented 2 years ago

I haven't had any time yet to check this out thoroughly, but it seems to be a bit overkill for a couple of actual changes. The policy so far has been to adopt new C++ standards when they are natively supported by all target platforms. However, if all the release builds pass then why not (in this case). I have been using the similar stream stuff in Java lately :p

tcoyvwac commented 2 years ago

Streams / Ranges are very friendly once one is used to them! :cat2:

There are future commits / PRs in the pipeline but needed the (adding) range-v3 commit (history) to be understandable. That is why for now, there is only this demo range-v3 commit at the moment (to help any / all readers of this PR).

The only problems for this PR was the CMakeLists.txt file as, after reading carefully, CMake standards for Heimer are not higher than 3.x.

The policy so far has been to adopt new C++ standards when they are natively supported by all target platforms. However, if all the release builds pass then why not (in this case).

I agree with you. So, if the release CI passes then... :smile_cat:

juzzlin commented 2 years ago

Btw, I would appreciate your full name in the commit messages.

juzzlin commented 2 years ago

How this should work is that first you take a complete range release, let's say https://github.com/ericniebler/range-v3/archive/refs/tags/0.11.0.tar.gz.

It must include all license files etc.

Then you extract the whole release under src/contrib/:

$ ls

Argengine  range-v3-0.11.0  SimpleLogger

And then add this to src/CMakeLists.txt (a bit like you already did):

add_subdirectory(contrib/range-v3-0.11.0 EXCLUDE_FROM_ALL)
include_directories(contrib/range-v3-0.11.0/include)

I tested that it works like this at least on my Ubuntu 20.04.

tcoyvwac commented 2 years ago

I'm sorry, I am anonymous on all of my commits I do in opensource projects. I understand your preference and Heimer is a great development project, however if this strongly unworkable for you, it would be unfortunate and would have to withdraw further collaboration.

Is there a critical concern with the current collaboration so far through GitHub?

juzzlin commented 2 years ago

I'm sorry, I am anonymous on all of my commits I do in opensource projects. I understand your preference and Heimer is a great development project, however if this strongly unworkable for you, it would be unfortunate and would have to withdraw further collaboration.

Is there a critical concern with the current collaboration so far through GitHub?

Well, it's not a blocker and you may proceed with this. It just could potentially be a bit difficult when it comes to copyright and licensing issues when you have code from anonymous contributors :p

tcoyvwac commented 2 years ago

How this should work is that first you take a complete range release, let's say https://github.com/ericniebler/range-v3/archive/refs/tags/0.11.0.tar.gz.

It must include all license files etc.

Then you extract the whole release under src/contrib/:

Thanks, this clarification helps as it was unsure if it would be acceptable to include another CMake project into your Heimer project so happy to change to this style. :smile_cat: The current "header include directory" was the compromise (to demonstrate the usage of range-v3 in Heimer).

Please note that the 0.11.0 release is quite "old" (7th Aug 2020), and this PR version is from range-v3 upstream:master 9aa032c.

tcoyvwac commented 2 years ago

In addition, please note that all header files in the range-v3 library have their required license attributions within them. The library's header files are made to be used "standalone", to be as flexible as possible for many development projects.

tcoyvwac commented 2 years ago

Humm, quick thought @juzzlin, if you are concerned about copyright issues to do with range-v3, would you like to commit the range-v3 library into your project (into src/contrib/ yourself) and then this PR will be edited, to only submit the changes / changeset inside of graph.cpp? :smile_cat:

That means that the PR changeset can & will become quite small, which could be more to your preference.

juzzlin commented 2 years ago

Humm, quick thought @juzzlin, if you are concerned about copyright issues to do with range-v3, would you like to commit the range-v3 library into your project (into src/contrib/ yourself) and then this PR will be edited, to only submit the changes / changeset inside of graph.cpp? 😸

That means that the PR changeset can & will become quite small, which could be more to your preference.

Yes, I can do that :)

tcoyvwac commented 2 years ago

Ok, thats great! :smile_cat: Then please merge the range-v3 library into the project, and when that happens, this PR will be blocked (from merging due to a merge conflict).

From there, I will know to re-adjust this PR, to only have the graph.cpp changeset. Once that is done, I will message and then you can merge those bespoke changes in with no problems.

juzzlin commented 2 years ago

I now cherry-picked your payload commit on top of my addition of range-v3, so consider this as merged.