Skip to content

Commit

Permalink
deprecate needs_select=yes
Browse files Browse the repository at this point in the history
The behaviour of needs_select=yes event is rather strange and hard to understand:
When a menu items with needs_select=yes it clicked, it first fires the previous select event again but this time in a synced context, so that the select event was fires 2 times total on the playing client and one time at the other clients. It also the breaks the specification in the wiki that select events are not fired in synced contexts.

Also it might happen that the unit stands no longer at the position from where it was selected because it has moved causing unexpected behaviour in this case.

The new way to let other clients know the currents selected unit is the [sync_variable] tag which was added in 1.13.
  • Loading branch information
gfgtdf committed Dec 22, 2016
1 parent 1553cc6 commit c0c4bf3
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/game_events/menu_item.cpp
Expand Up @@ -93,6 +93,9 @@ wml_menu_item::wml_menu_item(const std::string& id, const config & cfg) :
use_wml_menu_(cfg["use_hotkey"].str() != "only"),
is_synced_(cfg["synced"].to_bool(true))
{
if(needs_select_) {
ERR_NG << "needs_select=yes is deprecated\n";
}
}

/**
Expand Down Expand Up @@ -309,8 +312,10 @@ void wml_menu_item::update(const vconfig & vcfg)
hotkey_updated = true;
}

if ( vcfg.has_attribute("needs_select") )
if ( vcfg.has_attribute("needs_select") ) {
ERR_NG << "needs_select=yes is deprecated\n";
needs_select_ = vcfg["needs_select"].to_bool();
}
if ( vcfg.has_attribute("synced") )
is_synced_ = vcfg["synced"].to_bool(true);

Expand Down

5 comments on commit c0c4bf3

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to check it twice? I'd think removing lines 96-98 would still be sufficient as a deprecation message.

@gfgtdf
Copy link
Contributor Author

@gfgtdf gfgtdf commented on c0c4bf3 Jan 8, 2017

Choose a reason for hiding this comment

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

I just added the message everyhere where 'needs_select' is read, i think that one of the code is run when the new menu item is created and the other one is run when an existent one is modified (e.g when a menu item with that id already existed in [set_menu_item]) but i am not sure.

@sapientN3T
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but in the second case you are firing a (misleading) deprecation warning even when needs_select=no, so they may search their code and not find it.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

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

No, it's being fired only if it was specified, I think? Regardless of its value.

@gfgtdf
Copy link
Contributor Author

@gfgtdf gfgtdf commented on c0c4bf3 Jan 25, 2017

Choose a reason for hiding this comment

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

updates errormessage fc40439

Please sign in to comment.