[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Transient failure on Travis @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways. Travis build 3645. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/401434259. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Works on linux as expected :-) Regarding Spinbox vs Dropdown: Maybe i am too konservative for such changes ;) The Tooltip for 'Number of Players' says 'Select Item'. Maybe change it to 'Choose amount of players'? 'Set number of players'? 'Number of players'? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
I think centered would be a pain in the butt when setting player starting positions. I forgot the tooltip for the stating positions tool that kaputtnik mentioned, still need to do that bit. No idea why AppVeyor hasn't been triggered yet. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Review: Approve compile, test Work for me. the Window is initially at the top left, that Alert is always at the top left, would better like it to be centered, OTOH this dialog is used seldom. kaputtnick: can you confirm for Linux? Gun: if you trigger a build I could check on Windows. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Dropdown positioning should be fixed now, thanks for catching that one :) > i would prefer the old spinbox for 'Number of Players'. I think it fits > better with the other places where an amount could be chosen, e.g. for the > resources. This sort of argument means not having any dropdowns, because we can't overhaul everything at once. It makes sense to keep the Spinbox for the resources, but in other cases like map size and tool size, we should have dropdowns too. So, you'll need to make a stronger argument to convince me to have to click 7 times instead of 2 times if I want a map with 8 players. I'll listen if you have one ;) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Here on linux it looks like https://bugs.launchpad.net/widelands/+bug/1691339/+attachment/5161992/+files/player_options.png The tribe selection looks also some kind of misplaced: https://bugs.launchpad.net/widelands/+bug/1691339/+attachment/5161993/+files/player_options-tribe.png While the tribe selection is a good idea, i would prefer the old spinbox for 'Number of Players'. I think it fits better with the other places where an amount could be chosen, e.g. for the resources. The button for placing the starting position should get a tooltipp too :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Questions answered in-line. Will look into the positioning issue.
Diff comments:
>
> === modified file 'src/editor/ui_menus/player_menu.cc'
> --- src/editor/ui_menus/player_menu.cc2018-05-22 11:29:41 +
> +++ src/editor/ui_menus/player_menu.cc2018-07-08 12:09:41 +
> @@ -19,266 +19,313 @@
>
> #include "editor/ui_menus/player_menu.h"
>
> +#include
> +
> +#include
> #include
>
> #include "base/i18n.h"
> -#include "base/wexception.h"
> #include "editor/editorinteractive.h"
> #include "editor/tools/set_starting_pos_tool.h"
> #include "graphic/graphic.h"
> +#include "graphic/playercolor.h"
> #include "logic/map.h"
> #include "logic/map_objects/tribes/tribe_basic_info.h"
> -#include "logic/player.h"
> -#include "ui_basic/editbox.h"
> -#include "ui_basic/messagebox.h"
> -#include "ui_basic/textarea.h"
> -
> -#define UNDEFINED_TRIBE_NAME ""
> +#include "logic/widelands.h"
> +#include "profile/profile.h"
> +#include "ui_basic/checkbox.h"
> +#include "ui_basic/multilinetextarea.h"
> +
> +namespace {
> +constexpr int kMargin = 4;
> +// If this ever gets changed, don't forget to change the strings in the
> warning box as well.
> +constexpr Widelands::PlayerNumber max_recommended_players = 8;
> +} // namespace
> +
> +class EditorPlayerMenuWarningBox : public UI::Window {
> +public:
> + explicit EditorPlayerMenuWarningBox(UI::Panel* parent)
> + /** TRANSLATORS: Window title in the editor when a player has selected
> more than the recommended number of players */
> +: Window(parent, "editor_player_menu_warning_box", 0, 0, 500, 220,
> _("Too Many Players")),
> + box_(this, 0, 0, UI::Box::Vertical, 0, 0, 2 * kMargin),
> + warning_label_(_,
> + 0,
> + 0,
> + 300,
> + 0,
> + UI::PanelStyle::kWui,
> + /** TRANSLATORS: Info text in editor player menu
> when a player has selected more than the recommended number of players.
> Choice is made by OK/Abort. */
> + _("We do not recommend setting more than 8 players
> except for testing "
> + "purposes. Are you sure that you want more than 8
> players?"),
> + UI::Align::kLeft,
> + UI::MultilineTextarea::ScrollMode::kNoScrolling),
> + /** TRANSLATORS: Checkbox for: 'We do not recommend setting more
> than 8 players except for
> + testing purposes. Are you sure that you want more than 8
> players?' */
> + reminder_choice_(_, Vector2i::zero(), _("Do not remind me
> again")),
> + button_box_(_, kMargin, kMargin, UI::Box::Horizontal, 0, 0, 2
> * kMargin),
> + ok_(_box_, "ok", 0, 0, 120, 0,
> UI::ButtonStyle::kWuiPrimary, _("OK")),
> + cancel_(_box_, "cancel", 0, 0, 120, 0,
> UI::ButtonStyle::kWuiSecondary, _("Abort")) {
> +
> + set_center_panel(_);
> +
> + box_.add(_label_, UI::Box::Resizing::kFullSize);
> + box_.add(_choice_, UI::Box::Resizing::kFullSize);
> +
> + button_box_.add_inf_space();
> + button_box_.add(_);
> + button_box_.add_inf_space();
> + button_box_.add(_);
> + button_box_.add_inf_space();
> + box_.add_space(kMargin);
> + box_.add(_box_, UI::Box::Resizing::kFullSize);
> + box_.add_space(kMargin);
> +
> +
> ok_.sigclicked.connect(boost::bind(::ok,
> boost::ref(*this)));
> + cancel_.sigclicked.connect(
> +boost::bind(::cancel,
> boost::ref(*this)));
> + }
> +
> + void ok() {
> + write_option();
> + end_modal(UI::Panel::Returncodes::kOk);
> + }
> +
> + void cancel() {
> + write_option();
> +
> end_modal(UI::Panel::Returncodes::kBack);
> + }
> +
> + void write_option() {
> + if (reminder_choice_.get_state()) {
> + g_options.pull_section("global").set_bool(
> +"editor_player_menu_warn_too_many_players", false);
> + }
> + }
> +
> +private:
> + UI::Box box_;
> + UI::MultilineTextarea warning_label_;
> + UI::Checkbox reminder_choice_;
> + UI::Box button_box_;
> + UI::Button ok_;
> + UI::Button cancel_;
> +};
>
> inline EditorInteractive& EditorPlayerMenu::eia() {
> return dynamic_cast(*get_parent());
> }
>
> EditorPlayerMenu::EditorPlayerMenu(EditorInteractive& parent,
> UI::UniqueWindow::Registry& registry)
> - : UI::UniqueWindow(, "players_menu", , 340, 400,
> _("Player Options")),
> - add_player_(this,
> - "add_player",
> - get_inner_w() - 5 - 20,
> - 5,
> - 20,
> - 20,
> - UI::ButtonStyle::kWuiSecondary,
> -
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Review: Needs Fixing compile, test Sorry, that dropdown Window for "Number of players" always appears at the top left of the screen. Playing in a Window on OSX 1280 x 720 and in full screen mode. Does happen on Linux/Windows, too? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691339-editor-8-players. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
> menu.select_tool(menu.tools()->set_starting_pos, EditorTool::First);
> - menu.tools()->set_starting_pos.set_current_player(n);
> + menu.tools()->set_starting_pos.set_current_player(row);
>
> // reselect tool, so everything is in a defined state
> menu.select_tool(menu.tools()->current(), EditorTool::First);
> - update();
> +
> + // Signal player position states via button states
> + iterate_player_numbers(pn, map->get_nrplayers()) {
> + if (pn == row) {
> + rows_.at(pn -
> 1)->position->set_background_style(UI::ButtonStyle::kWuiPrimary);
> + rows_.at(pn - 1)->position->set_perm_pressed(true);
> + } else {
> + rows_.at(pn -
> 1)->position->set_background_style(UI::ButtonStyle::kWuiSecondary);
> + rows_.at(pn -
> 1)->position->set_perm_pressed(map->get_starting_pos(pn) !=
> +
> Widelands::Coords::null());
> + }
> + }
> }
>
> -/**
> - * Player name has changed
> - */
> -void EditorPlayerMenu::name_changed(int32_t m) {
> +void EditorPlayerMenu::name_changed(size_t row) {
> // Player name has been changed.
> - std::string text = plr_names_[m]->text();
> + const std::string& text = rows_.at(row)->name->text();
> EditorInteractive& menu = eia();
> Widelands::Map* map = menu.egbase().mutable_map();
> - map->set_scenario_player_name(m + 1, text);
> - plr_names_[m]->set_text(map->get_scenario_player_name(m + 1));
> + map->set_scenario_player_name(row + 1, text);
> menu.set_need_save(true);
> }
>
> === modified file 'src/ui_basic/dropdown.cc'
> --- src/ui_basic/dropdown.cc 2018-04-27 06:11:05 +
> +++ src/ui_basic/dropdown.cc 2018-07-08 12:09:41 +
> @@ -129,8 +129,13 @@
> }
>
> BaseDropdown::~BaseDropdown() {
> - // Listselect is already taking care of the cleanup,
> - // so no call to clear() needed here.
> + // The list needs to be able to drop outside of windows, so it won't
> close with the window.
> + // Deleting here leads to conflict with who gets to delete it, so we
> hide it instead.
> + // TODO(GunChleoc): Investigate whether we can find a better solution
> for this
Mhh, Is this a memeory leak, or just bad coding?
> + if (list_) {
> + list_->clear();
> + list_->set_visible(false);
> + }
> }
>
> void BaseDropdown::set_height(int height) {
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Continuous integration builds have changed state: Travis build 3645. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/401434259. Appveyor build 3444. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1691339_editor_8_players-3444. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
Continuous integration builds have changed state: Travis build 3640. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/401388874. Appveyor build 3439. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1691339_editor_8_players-3439. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands.
Commit message:
Refactored Editor Player Menu to use Box layout. Show dismissable warning if
more than 8 players are selected.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1691339 in widelands: "Inform map editors not to use more than 8 players"
https://bugs.launchpad.net/widelands/+bug/1691339
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1691339-editor-8-players/+merge/349092
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1691339-editor-8-players into lp:widelands.
=== modified file 'src/editor/CMakeLists.txt'
--- src/editor/CMakeLists.txt 2017-11-20 13:50:51 +
+++ src/editor/CMakeLists.txt 2018-07-08 07:14:33 +
@@ -103,6 +103,7 @@
map_io
map_io_map_loader
notifications
+profile
random
scripting_coroutine
scripting_lua_interface
=== modified file 'src/editor/ui_menus/player_menu.cc'
--- src/editor/ui_menus/player_menu.cc 2018-05-22 11:29:41 +
+++ src/editor/ui_menus/player_menu.cc 2018-07-08 07:14:33 +
@@ -19,235 +19,283 @@
#include "editor/ui_menus/player_menu.h"
+#include
+
+#include
#include
#include "base/i18n.h"
-#include "base/wexception.h"
#include "editor/editorinteractive.h"
#include "editor/tools/set_starting_pos_tool.h"
#include "graphic/graphic.h"
+#include "graphic/playercolor.h"
#include "logic/map.h"
#include "logic/map_objects/tribes/tribe_basic_info.h"
-#include "logic/player.h"
-#include "ui_basic/editbox.h"
-#include "ui_basic/messagebox.h"
-#include "ui_basic/textarea.h"
-
-#define UNDEFINED_TRIBE_NAME ""
+#include "logic/widelands.h"
+#include "profile/profile.h"
+#include "ui_basic/checkbox.h"
+#include "ui_basic/multilinetextarea.h"
+
+namespace {
+constexpr int kMargin = 4;
+// If this ever gets changed, don't forget to change the strings in the warning box as well.
+constexpr Widelands::PlayerNumber max_recommended_players = 8;
+} // namespace
+
+class EditorPlayerMenuWarningBox : public UI::Window {
+public:
+ explicit EditorPlayerMenuWarningBox(UI::Panel* parent)
+ : Window(parent, "editor_player_menu_warning_box", 0, 0, 500, 220, _("Too Many Players")),
+ box_(this, 0, 0, UI::Box::Vertical, 0, 0, 2 * kMargin),
+ warning_label_(_,
+ 0,
+ 0,
+ 300,
+ 0,
+ UI::PanelStyle::kWui,
+ /** TRANSLATORS: Info text in editor player menu */
+ _("We do not recommend setting more than 8 players except for testing "
+ "purposes. Are you sure that you want more than 8 players?"),
+ UI::Align::kLeft,
+ UI::MultilineTextarea::ScrollMode::kNoScrolling),
+ /** TRANSLATORS: Checkbox for: 'We do not recommend setting more than 8 players except for
+ testing purposes. Are you sure that you want more than 8 players?' */
+ reminder_choice_(_, Vector2i::zero(), _("Do not remind me again")),
+ button_box_(_, kMargin, kMargin, UI::Box::Horizontal, 0, 0, 2 * kMargin),
+ ok_(_box_, "ok", 0, 0, 120, 0, UI::ButtonStyle::kWuiPrimary, _("OK")),
+ cancel_(_box_, "cancel", 0, 0, 120, 0, UI::ButtonStyle::kWuiSecondary, _("Abort")) {
+
+ set_center_panel(_);
+
+ box_.add(_label_, UI::Box::Resizing::kFullSize);
+ box_.add(_choice_, UI::Box::Resizing::kFullSize);
+
+ button_box_.add_inf_space();
+ button_box_.add(_);
+ button_box_.add_inf_space();
+ button_box_.add(_);
+ button_box_.add_inf_space();
+ box_.add_space(kMargin);
+ box_.add(_box_, UI::Box::Resizing::kFullSize);
+ box_.add_space(kMargin);
+
+ ok_.sigclicked.connect(boost::bind(::ok, boost::ref(*this)));
+ cancel_.sigclicked.connect(
+ boost::bind(::cancel, boost::ref(*this)));
+ }
+
+ void ok() {
+ write_option();
+ end_modal(UI::Panel::Returncodes::kOk);
+ }
+
+ void cancel() {
+ write_option();
+ end_modal(UI::Panel::Returncodes::kBack);
+ }
+
+ void write_option() {
+ if (reminder_choice_.get_state()) {
+ g_options.pull_section("global").set_bool(
+ "editor_player_menu_warn_too_many_players", false);
+ }
+ }
+
+private:
+ UI::Box box_;
+ UI::MultilineTextarea warning_label_;
+ UI::Checkbox reminder_choice_;
+ UI::Box button_box_;
+ UI::Button ok_;
+ UI::Button cancel_;
+};
inline EditorInteractive& EditorPlayerMenu::eia() {
return dynamic_cast(*get_parent());
}
EditorPlayerMenu::EditorPlayerMenu(EditorInteractive& parent, UI::UniqueWindow
