Skip to content
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

Refactor implementation of observer pattern in low-level classes #449

Merged
merged 2 commits into from May 14, 2019

Conversation

@ubruhin
Copy link
Member

commented May 12, 2019

Many objects need to get notified about changes in other objects, for example when clicking CTRL+Z, the undo stack reverts changes in objects and the GUI needs to get notified about these changes. For higher level classes we do this with Qt's signal/slot mechanism as it is easy to use and very safe.

But this works only in classes derived from QObject and I'm not sure if it's a good idea to have even low-level classes derived from QObject. A typical LibrePCB project contains many thousands of lower level objects, this might lead to performance issues when deriving them all from QObject. In addition, inheritance is often a pain and thus it's better to keep the inheritance hierarchy as simple as possible.

Until now, the lower level classes had implemented the observer pattern by declaring an interface for observers and register/unregister methods. But that was also a bit annoying since it leads to a lot of code and multiple inheritance. And it was a bit dangerous since objects and their observers weren't automatically disconnected if either the object or the observer was destroyed. So it was easy to access dangling pointers.

This PR replaces the observer pattern with an implementation which doesn't need any inheritance and is much more safe to use. Accessing dangling pointers should no longer be possible since the objects and their observers are automatically disconnected if either the object or the observer is destroyed.

See implementation in libs/librepcb/common/signalslot.h (docs here).

Required for #446

@ubruhin ubruhin added this to the 0.1.2 milestone May 12, 2019

@ubruhin ubruhin self-assigned this May 12, 2019

@ubruhin ubruhin added this to In Progress in Improve code quality May 12, 2019

@ubruhin ubruhin force-pushed the refactor-observer-pattern branch 2 times, most recently from 53950f9 to df269e6 May 12, 2019

@ubruhin ubruhin force-pushed the refactor-observer-pattern branch from df269e6 to c35eab1 May 12, 2019

@ubruhin ubruhin marked this pull request as ready for review May 12, 2019

@ubruhin ubruhin force-pushed the refactor-observer-pattern branch 3 times, most recently from 63f8bb6 to a0dc7e9 May 13, 2019

@ubruhin ubruhin force-pushed the refactor-observer-pattern branch from a0dc7e9 to b866745 May 14, 2019

@ubruhin ubruhin merged commit 1c9c64b into master May 14, 2019

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ubruhin ubruhin deleted the refactor-observer-pattern branch May 14, 2019

@ubruhin ubruhin moved this from In Progress to Done in Improve code quality May 14, 2019

ubruhin added a commit that referenced this pull request Jul 8, 2019

Merge pull request #449 from LibrePCB/refactor-observer-pattern
Refactor implementation of observer pattern in low-level classes
(cherry picked from commit 1c9c64b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
You can’t perform that action at this time.