Skip to content

Commit

Permalink
Game Events/Manager: added function to encapsulate iteration-by-id in…
Browse files Browse the repository at this point in the history
…terface

This allows manager::iteration to be made private in order to restrict access to handler iterators
within the manager class and not outside it.
  • Loading branch information
Vultraz committed May 20, 2017
1 parent 1005843 commit be665e2
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 19 deletions.
10 changes: 10 additions & 0 deletions src/game_events/manager.cpp
Expand Up @@ -204,6 +204,16 @@ void manager::write_events(config& cfg) const
wml_menu_items_.to_config(cfg);
}

void manager::execute_on_events(const std::string& event_id, manager::event_func_t func)
{
iteration iter(event_id, *this);

while(handler_ptr hand = *iter) {
func(*this, hand);
++iter;
}
}

game_events::wml_event_pump& manager::pump()
{
return *pump_;
Expand Down
6 changes: 4 additions & 2 deletions src/game_events/manager.hpp
Expand Up @@ -41,7 +41,7 @@ namespace game_events {
/// If a second manager object is created before destroying the previous
/// one, the game will crash with an assertion failure.
class manager {
public:
private:

This comment has been minimized.

Copy link
@CelticMinstrel

CelticMinstrel May 20, 2017

Member

Could just delete the line as private is the default access level for classes.

This comment has been minimized.

Copy link
@Vultraz

Vultraz May 20, 2017

Author Member

There are more private class members below the subclass definition.

This comment has been minimized.

Copy link
@GregoryLundberg

GregoryLundberg May 20, 2017

Contributor

I prefer explicitly stating public/protected/private. For example, the default for struct is public but the default for class is private. If someone accidentally switches class/struct for some reason, having visibility explicit avoids issues.

/// This class is similar to an input iterator through event handlers,
/// except each instance knows its own end (determined when constructed).
/// Subsequent dereferences are not guaranteed to return the same element,
Expand Down Expand Up @@ -92,7 +92,6 @@ namespace game_events {
game_data * gamedata_;
};

private:
// Performs an assertion check to ensure these members are not null.
friend void event_handler::disable();

Expand Down Expand Up @@ -121,6 +120,9 @@ namespace game_events {
const std::string& type = std::string());
void write_events(config& cfg) const;

using event_func_t = std::function<void(game_events::manager&, handler_ptr)>;
void execute_on_events(const std::string& event_id, event_func_t func);

game_events::wml_event_pump & pump();

game_events::wmi_manager& wml_menu_items()
Expand Down
11 changes: 5 additions & 6 deletions src/game_events/menu_item.cpp
Expand Up @@ -329,14 +329,13 @@ void wml_menu_item::update_command(const config& new_command)
// If there is an old command, remove it from the event handlers.
if(!command_.empty()) {
assert(resources::game_events);
manager::iteration iter(event_name_, *resources::game_events);
while(handler_ptr hand = *iter) {
if(hand->is_menu_item()) {

resources::game_events->execute_on_events(event_name_, [=](game_events::manager& man, handler_ptr ptr) {
if(ptr->is_menu_item()) {
LOG_NG << "Removing command for " << event_name_ << ".\n";
resources::game_events->remove_event_handler(command_["id"].str());
man.remove_event_handler(command_["id"].str());
}
++iter;
}
});
}

// Update our stored command.
Expand Down
18 changes: 7 additions & 11 deletions src/game_events/pump.cpp
Expand Up @@ -552,20 +552,16 @@ bool wml_event_pump::operator()()

if ( event_id.empty() ) {

// Initialize an iteration over event handlers matching this event.
manager::iteration handler_iter(event_name, *impl_->my_manager);

// While there is a potential handler for this event name.
while ( handler_ptr cur_handler = *handler_iter ) {
// Handle events of this name.
impl_->my_manager->execute_on_events(event_name, [=](game_events::manager&, handler_ptr ptr) {
DBG_EH << "processing event " << event_name << " with id="<<
cur_handler->get_config()["id"] << "\n";
// Let this handler process our event.
process_event(cur_handler, ev);
// NOTE: cur_handler may be null at this point!
ptr->get_config()["id"] << "\n";

++handler_iter;
}
// Let this handler process our event.
process_event(ptr, ev);

// NOTE: ptr may be null at this point!
});
}
else {

Expand Down

13 comments on commit be665e2

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

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

tags @GregoryLundberg since relevant

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

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

@GregoryLundberg ok, so looking at this further it looks like handler_list iterators are restricted to the manager::iteration class, which is now private to the manager. How should I go from here?

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

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

You should stop and put this aside until 1.14 is released.

@GregoryLundberg
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. If we go forward with odd/test even/release, this should be a 1.15 or 1.17 milestone.

Don't want to break such a central function.

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

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

1.17 wat.

@GregoryLundberg
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it took 20 years to get the mess we have, it's not gonna get fixed in a year.

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

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

Well it damn well not take another 20.

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

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

Could you explain the mess fully, though. Looking at it it seems like the mess is rather localized.

@GregoryLundberg
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, one valid approach would be to create a new header for the feature which is JUST for the feature, remove it from the original headers and chance down all the places which break (a lot will but most will be fixable by removing an unneeded reference).

I fear it's a Rabbit Hole, though. And a Gordian Knot.

As I recall when I was pulling it out, last Autumn, I found the feature spread through two or three headers which pulled in a lot of unrelated stuff and were referenced all over the place by a huge number of totally unrelated things.

What I did was a header and rough implementation and the goal was a clean compile which was usable but only included the header where it was absolutely needed. I remember compile after compile tracing down where to remove the unneeded references and fixing the sudden lack of things which were totally unrelated, came along for the ride, and broke something when suddenly missing.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

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

If you really must continue working on this, start a branch and don't merge it until after 1.14 is branched off (and then only if it works, of course).

@GregoryLundberg
Copy link
Contributor

Choose a reason for hiding this comment

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

Second,

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

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

Admittedly, there is much to be done. However, I'm only looking at the handler_list and smart_list classes right now and how the former can be simplified.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

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

Sure. Do it on a branch.

Please sign in to comment.