[Widelands-dev] [Merge] lp:~widelands-dev/widelands/watchwindow-fixes into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/watchwindow-fixes into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
Cool, thanks for testing and fixing! Travis went through as well. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
Review: Approve test / compile Bug #1553699? is fixed with this branch, just reproduced this on bzr7903[trunk], and found it fixed it here in bzr7904[watchwindow-fixes] @Miroslav Remák: thanks for fixing this. -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
@Klaus Halfmann Also, did you notice the reproduction steps I posted in bug #1553699? Can you try these steps with the version used in the bug report (bzr7866) to see if they produce a crash and if so, try the same steps with this branch? -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
> I am still missing some comments about the basic > workings of this (very sepcial) window. and I have some questions: > > * why is uint8_t as index, a plain unsigned int would not make a difference? Old code. We actually think that using int is even better than unsigned int, because it prevents overflow errors. > * why is there no visual response when adding the last view fails? Because nobody has noticed or taken care of it. Maybe we should pop up a WLMessageBox? > I cannot reproduce #1553699 directly, as the filesaveversion sha changed, uhm: > > roaddata: road 455: [...watchwindow-fixes/src/economy/request.cc:172] request: > [...watchwindow-fixes/src/logic/map_objects/tribes/requirements.cc:51] > requirements: > > UnhandledVersionError: > > I must tray with some new game. I guess the easiest way for testing now is to just merge this, so we have a higher chance of the error occurring if it still does. So, I'll mark the bug as "Fix Committed" after merging, and we can reopen the bug if it happens again. -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
I am still missing some comments about the basic workings of this (very sepcial) window. and I have some questions: * why is uint8_t as index, a plain unsigned int would not make a difference? * why is there no visual response when adding the last view fails? I cannot reproduce #1553699 directly, as the filesaveversion sha changed, uhm: roaddata: road 455: [...watchwindow-fixes/src/economy/request.cc:172] request: [...watchwindow-fixes/src/logic/map_objects/tribes/requirements.cc:51] requirements: UnhandledVersionError: I must tray with some new game. -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
New changes still LGTM - Happy :) -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
Ill try to reproduce #1553699 whit this perhaps today ... -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
Review: Approve LGTM :) I think it would also be great when a new location is added, that the window activates that new location. It doesn't do that in trunk either though, so it's up to you if you want to work on this in this branch. -- https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/watchwindow-fixes. ___ 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/watchwindow-fixes into lp:widelands
Miroslav Remák has proposed merging
lp:~widelands-dev/widelands/watchwindow-fixes into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/watchwindow-fixes/+merge/289573
- Fixes some oddities such as view duplication
- Possibly fixes bug #1553699 (probable cause: std::erase used on invalid
position, which results in undefined behavior)
- Highlights the current view button, as it's difficult to tell which view is
selected
- Cleans up the code a bit
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/watchwindow-fixes into lp:widelands.
=== modified file 'src/wui/watchwindow.cc'
--- src/wui/watchwindow.cc 2016-02-18 18:27:52 +
+++ src/wui/watchwindow.cc 2016-03-19 23:00:17 +
@@ -62,8 +62,8 @@
boost::signals2::signal warp_mainview;
void add_view(Widelands::Coords);
- void next_view(bool first = false);
- void show_view(bool first = false);
+ void next_view();
+ void show_view();
Point calc_coords(Widelands::Coords);
void save_coords();
void close_cur_view();
@@ -76,7 +76,8 @@
private:
void do_follow();
void do_goto();
- void setview(uint8_t index);
+ void view_button_clicked(uint8_t index);
+ void set_current_view(uint8_t idx, bool save_previous = true);
MapView mapview;
uint32_t last_visit;
@@ -98,7 +99,8 @@
UI::Window(, "watch", x, y, w, h, _("Watch")),
mapview (this, 0, 0, 200, 166, parent),
last_visit(game().get_gametime()),
- single_window(init_single_window)
+ single_window(init_single_window),
+ cur_index(0)
{
UI::Button * followbtn =
new UI::Button
@@ -128,7 +130,7 @@
"-", std::string(),
false);
view_btns[i]->sigclicked.connect
-(boost::bind(::setview, this, i));
+(boost::bind(::view_button_clicked, this, i));
}
UI::Button * closebtn =
@@ -146,7 +148,7 @@
warp_mainview.connect(boost::bind(::move_view_to_point, , _1));
add_view(coords);
- next_view(true);
+ set_current_view(0, false);
}
/**
@@ -180,16 +182,8 @@
}
// Switch to next view
-void WatchWindow::next_view(bool const first) {
- if (!first && views.size() == 1)
- return;
- if (!first)
- save_coords();
- if (first || (cur_index == views.size() - 1 && cur_index != 0))
- cur_index = 0;
- else if (cur_index < views.size() - 1)
- ++cur_index;
- show_view(first);
+void WatchWindow::next_view() {
+ set_current_view((cur_index + 1) % views.size());
}
@@ -212,8 +206,23 @@
}
}
+void WatchWindow::set_current_view(uint8_t idx, bool save_previous)
+{
+ assert(idx < views.size());
+
+ if (save_previous)
+ save_coords();
+
+ if (single_window) {
+ view_btns[cur_index]->set_perm_pressed(false);
+ view_btns[idx]->set_perm_pressed(true);
+ }
+ cur_index = idx;
+ show_view();
+}
+
// Draws the current view
-void WatchWindow::show_view(bool) {
+void WatchWindow::show_view() {
mapview.set_viewpoint(views[cur_index].view_point, true);
}
@@ -345,12 +354,10 @@
/**
* Sets the current view to @p index and resets timeout.
*/
-void WatchWindow::setview(uint8_t index)
+void WatchWindow::view_button_clicked(uint8_t index)
{
- save_coords();
- cur_index = index;
+ set_current_view(index);
last_visit = game().get_gametime();
- show_view();
}
/**
@@ -366,13 +373,8 @@
return;
}
- uint8_t const old_index = cur_index;
- next_view();
- std::vector::iterator view_it = views.begin();
- for (uint8_t i = 0; i < old_index; ++i)
- ++view_it;
- view_btns[cur_index]->set_enabled(false);
- views.erase(view_it);
+ views.erase(views.begin() + cur_index);
+ set_current_view(cur_index % views.size(), false);
toggle_buttons();
}
___
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
