Skip to content

Commit

Permalink
refactor
Browse files Browse the repository at this point in the history
  • Loading branch information
@markaren
markaren committed Feb 29, 2024
1 parent bfd9958 commit 3660569
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 17 deletions.
6 changes: 3 additions & 3 deletions include/threepp/core/EventDispatcher.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ namespace threepp {

struct FunctionalEventListener: EventListener {

FunctionalEventListener(): callback_([](const Event&) {}) {}
FunctionalEventListener(): callback_([](const Event&) {/*no op*/}) {}

void setListener(std::function<void(Event)> listener) {
callback_ = std::move(listener);
void setCallback(std::function<void(Event)> callback) {
callback_ = std::move(callback);
}

void onEvent(Event& event) override {
Expand Down
16 changes: 2 additions & 14 deletions tests/core/EventDispatcher_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,6 @@ using namespace threepp;

namespace {

struct LambdaEventListener: EventListener {

explicit LambdaEventListener(std::function<void(Event&)> f): f_(std::move(f)) {}

void onEvent(Event& event) override {
f_(event);
}

private:
std::function<void(Event&)> f_;
};


struct MyEventListener: EventListener {

int numCalled = 0;
Expand All @@ -47,7 +34,8 @@ TEST_CASE("Test events") {
MyEventListener l;

bool l1Called = false;
LambdaEventListener l1([&l1Called](Event& e) {
FunctionalEventListener l1;
l1.setCallback([&l1Called](const Event&) {
l1Called = true;
});

Expand Down

5 comments on commit 3660569

@bradphelan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are making things more complicated than you need

std::function<void(Event&)>

is already a typed erased function that can wrap any type of function.

 struct EventListener {

        virtual void onEvent(Event& event) = 0;

        virtual ~EventListener() = default;
    };

is exactly how std::function<void(Event&)> is implemented. So when you create a FunctionalEventListener which is a subclass of EventListener you could have just started with std::function<void(Event&)>.

@markaren
Copy link
Owner Author

@markaren markaren commented on 3660569 Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation choice has to do with the internal usage of the API.

Basically, this implementation forces a named object to be created that can later be used to remove the listener. It enables lamdas to be easily created and the internal implementation remains unchanged.

@bradphelan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked in detail at your internal implementation sorry but since lambda and std::function have become available I have never written functor type objects. Can you point me to the internal code that makes this a requirement?

@markaren
Copy link
Owner Author

@markaren markaren commented on 3660569 Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no hard requirement, but this change should offer the way of least resistance. I would be happy to receive an alternate PR if you are able.

@bradphelan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Here it is #237

Please sign in to comment.