search

simulton / QSchematic

A library that allows creating diagrams such as flowcharts or even proper engineering schematics within a Qt application.
https://simulton.com
MIT License
231 stars 60 forks source link

v2: add compilation firewall on non-template public interfaces #57

Open bjornstromberg opened 7 months ago

bjornstromberg commented 7 months ago

so i've been looking some at the code and one thing that hit me was the lack of 'compilation firewalls' / 'pimpl idioms'

since its a library it really should be firewalled since a new member variable in the interface would force recompilation all the way up the chain that use the library.

it also has pretty heavy use of templates, so i guess its a tradeoff on speed vs abi compatibility.

anyway the reasoning either way should be made clear in the documentation what abi compatibility can be expected on {patch, point, major} release

some quick examples..

QSchematic::View has exposed members for zoom, those should be hidden so they have to be triggered the correct way, and trigger a update of view. if they are supposed to be static const values, well they should be const static values.. private members, well if we add or remove one member, that will break the size of the object unless recompiled.

QSchematic::Scene also have exposed members in header, so same issue here..

i can go on with QSchematic::Settings..

myself I'll use it as a static lib, so it kinda does not matter for me, but it is also buildable as a shared lib, then it does matter, reason i use it as static lib is just this reason, i can't know when the abi will break, so static was the sane solution, and i guess most people use it/will use it as a static lib for this reason.

Tectu commented 7 months ago

Your observations and statements are correct. This library started off as a hobby project of mine when I had notably less experience than nowdays - No point in hiding it. This is a short coming of this library as of today. I'm trying to improve things step-by-step with minimal breakage between minor release but something like this certainly needs to go into a v2 major release.

All current consumers of this library I know of are linking statically to it for pretty much this reason.

bjornstromberg commented 7 months ago

Your observations and statements are correct. This library started off as a hobby project of mine when I had notably less experience than nowdays - No point in hiding it.

i would be alot more worried if you had not learned anything over the years so to say..

This is a short coming of this library as of today. I'm trying to improve things step-by-step with minimal breakage between minor release but something like this certainly needs to go into a v2 major release. All current consumers of this library I know of are linking statically to it for pretty much this reason.

well its a good reason for breaking stuff for v2, lets see if we cant get enough known issues for getting atleast started on designing v2 during 2024.

bjornstromberg commented 7 months ago

a draft on this one will be prioritized since it will be a blocker for anything else getting done, plan to get an initial draft on this under februari.