widelands

Merge lp:~widelands-dev/widelands/bug-1697242-fileview into lp:widelands

  1. bug-1697242-fileview
  2. Merge into trunk
Proposed by GunChleoc
Status: Merged
Merged at revision: 8414
Proposed branch: lp:~widelands-dev/widelands/bug-1697242-fileview
Merge into: lp:widelands
Diff against target: 342 lines (+35/-68)
16 files modified
src/editor/ui_menus/categorized_item_selection_menu.h (+1/-1)
src/editor/ui_menus/main_menu_map_options.cc (+1/-1)
src/ui_basic/fileview_panel.cc (+8/-11)
src/ui_basic/fileview_panel.h (+1/-5)
src/ui_basic/tabpanel.cc (+16/-24)
src/ui_basic/tabpanel.h (+0/-10)
src/ui_fsmenu/about.cc (+0/-4)
src/ui_fsmenu/options.cc (+0/-4)
src/wui/building_statistics_menu.cc (+1/-1)
src/wui/buildingwindow.cc (+1/-1)
src/wui/economy_options_window.cc (+1/-1)
src/wui/encyclopedia_window.cc (+1/-1)
src/wui/fieldaction.cc (+1/-1)
src/wui/game_debug_ui.cc (+1/-1)
src/wui/stock_menu.cc (+1/-1)
src/wui/ware_statistics_menu.cc (+1/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1697242-fileview
Reviewer Review Type Date Requested Status
SirVer Approve
kaputtnik (community) testing Approve
GunChleoc Needs Resubmitting
Review via email: mp+325456@code.launchpad.net

Commit message

Fixed assert failure with sizes in FileViewPanel::layout() and TabPanel. Also, simplified the TabPanel constructor.

Description of the change

I can't reproduce the crash, but this branch will hopefully fix it.

To post a comment you must log in.
Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Code is OK, but I got this assertion:

Assertion failed: (nh >= 0), function set_size, file
.../bug-1697242-fileview/src/ui_basic/panel.cc, line 242.
Abort trap: 6

but was unable to reproduce this :\
I think it was opening the about window after switching to fullscreen?
ON OSX we have the extar quirks with the Retina displays and theire special
"resolutions".

Obne using a Debugger it did not happen again.

Revision history for this message
GunChleoc (gunchleoc) wrote :

The original Bug reporter has tested & confirmed that the fix is working now.

review: Needs Resubmitting
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 2285. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/242178054.
Appveyor build 2120. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1697242_fileview-2120.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 2310. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/243586383.
Appveyor build 2143. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1697242_fileview-2143.

Revision history for this message
kaputtnik (franku) wrote :

I got a crash when trying to open the buildings statistics menu:

widelands: /home/kaputtnik/widelands-repo/bug-1697242-fileview/src/ui_basic/tabpanel.cc:313: virtual void UI::TabPanel::draw(RenderTarget&): Assertion `x <= get_w()' failed.

Played in fullscreen with initial resolution 800x600. Tested this three times and on the second try the crash happens only after opening the buildings statistics menu the second time (press B (open), press B (close), press B -> crash). The first and third try the crash happens after the first try to open the buildings statistics menu.

To be sure i opened also a new game with this branch, and after hitting 'B' several times the crash happens.

review: Needs Fixing
Revision history for this message
GunChleoc (gunchleoc) wrote :

Thanks! Should be fixed now.

review: Needs Resubmitting
Revision history for this message
kaputtnik (franku) wrote :

The About page works not correct. When clicking one of the tabs, nothing is shown anymore and the tab bar is cut. Happens also if you click on "Readme".

Options window:
- The 'background panel(?) (the one where ui elements are placed on), is restricted to the height to fit all ui elements when widelands is started in widow mode. This has side effects for the dropdown menus (they appear to small in height so only one entry is shown)

Switching to fullscreen and back the height of the background panel is as usual. But the dropdown menus then begins to far down from the initial point.

Didn't inspect further.

review: Needs Fixing
Revision history for this message
kaputtnik (franku) wrote :

Ah, a debug build shows assertions:

Clicking on tab in About:
widelands: ../src/ui_basic/tabpanel.cc:313: virtual void UI::TabPanel::draw(RenderTarget&): Assertion `x <= get_w()' failed.

Opening Options:
widelands: ../src/ui_basic/panel.cc:242: void UI::Panel::set_size(int, int): Assertion `nh >= 0' failed.

Revision history for this message
GunChleoc (gunchleoc) wrote :

*headdesk* next try.

review: Needs Resubmitting
Revision history for this message
kaputtnik (franku) wrote :

Works :-) Opened all windows and found no problems.

Should Ken Cunnningham be asked if the initial bug is still fixed for him?

Looks like this is complicated stuff... maybe add some comments to the code?

review: Approve (testing)
Revision history for this message
SirVer (sirver) wrote :

lgtm. This seems like a net simplification to me, which is always great :)

@bunnybot merge

review: Approve
Revision history for this message
GunChleoc (gunchleoc) wrote :

Thanks for the review :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/editor/ui_menus/categorized_item_selection_menu.h'
2--- src/editor/ui_menus/categorized_item_selection_menu.h 2017-02-25 13:27:40 +0000
3+++ src/editor/ui_menus/categorized_item_selection_menu.h 2017-06-28 10:47:49 +0000
4@@ -83,7 +83,7 @@
5 descriptions_(descriptions),
6 select_correct_tool_(select_correct_tool),
7 protect_against_recursive_select_(false),
8- tab_panel_(this, 0, 0, g_gr->images().get("images/wui/window_background_dark.png")),
9+ tab_panel_(this, g_gr->images().get("images/wui/window_background_dark.png")),
10 current_selection_names_(this,
11 0,
12 0,
13
14=== modified file 'src/editor/ui_menus/main_menu_map_options.cc'
15--- src/editor/ui_menus/main_menu_map_options.cc 2017-05-14 04:38:39 +0000
16+++ src/editor/ui_menus/main_menu_map_options.cc 2017-06-28 10:47:49 +0000
17@@ -67,7 +67,7 @@
18 g_gr->images().get("images/ui_basic/but1.png"),
19 _("Cancel")),
20 tab_box_(this, padding_, padding_, UI::Box::Vertical, max_w_, get_inner_h(), 0),
21- tabs_(&tab_box_, 0, 0, nullptr),
22+ tabs_(&tab_box_, nullptr),
23
24 main_box_(&tabs_, padding_, padding_, UI::Box::Vertical, max_w_, get_inner_h(), 0),
25 tags_box_(&tabs_, padding_, padding_, UI::Box::Vertical, max_w_, get_inner_h(), 0),
26
27=== modified file 'src/ui_basic/fileview_panel.cc'
28--- src/ui_basic/fileview_panel.cc 2017-03-02 08:06:00 +0000
29+++ src/ui_basic/fileview_panel.cc 2017-06-28 10:47:49 +0000
30@@ -29,13 +29,9 @@
31 namespace UI {
32
33 FileViewPanel::FileViewPanel(Panel* parent,
34- int32_t x,
35- int32_t y,
36- int32_t w,
37- int32_t h,
38 const Image* background,
39 TabPanel::Type border_type)
40- : TabPanel(parent, x, y, w, h, background, border_type), padding_(5) {
41+ : TabPanel(parent, background, border_type), padding_(5), contents_width_(0), contents_height_(0) {
42 layout();
43 }
44
45@@ -64,23 +60,24 @@
46 }
47
48 void FileViewPanel::update_tab_size(size_t index) {
49+ assert(get_inner_w() >= 0 && get_inner_h() >= 0);
50+ assert(contents_width_ >= 0 && contents_height_ >= 0);
51+
52 boxes_.at(index)->set_size(get_inner_w(), get_inner_h());
53 textviews_.at(index)->set_size(contents_width_, contents_height_);
54 }
55
56 void FileViewPanel::layout() {
57 assert(boxes_.size() == textviews_.size());
58- if (get_inner_w() == 0 && get_inner_h() == 0) {
59- return;
60- }
61+ assert(get_inner_w() >= 0 && get_inner_h() >= 0);
62
63 // If there is a border, we have less space for the contents
64 contents_width_ =
65- border_type_ == TabPanel::Type::kNoBorder ? get_w() - padding_ : get_w() - 2 * padding_;
66+ std::max(0, border_type_ == TabPanel::Type::kNoBorder ? get_w() - padding_ : get_w() - 2 * padding_);
67
68- contents_height_ = border_type_ == TabPanel::Type::kNoBorder ?
69+ contents_height_ = std::max(0, border_type_ == TabPanel::Type::kNoBorder ?
70 get_inner_h() - 2 * padding_ - UI::kTabPanelButtonHeight :
71- get_inner_h() - 3 * padding_ - UI::kTabPanelButtonHeight;
72+ get_inner_h() - 3 * padding_ - UI::kTabPanelButtonHeight);
73
74 for (size_t i = 0; i < boxes_.size(); ++i) {
75 update_tab_size(i);
76
77=== modified file 'src/ui_basic/fileview_panel.h'
78--- src/ui_basic/fileview_panel.h 2017-01-25 18:55:59 +0000
79+++ src/ui_basic/fileview_panel.h 2017-06-28 10:47:49 +0000
80@@ -36,10 +36,6 @@
81 class FileViewPanel : public TabPanel {
82 public:
83 FileViewPanel(Panel* parent,
84- int32_t x,
85- int32_t y,
86- int32_t w,
87- int32_t h,
88 const Image* background,
89 TabPanel::Type border_type = TabPanel::Type::kNoBorder);
90
91@@ -50,7 +46,7 @@
92 private:
93 void update_tab_size(size_t index);
94 void layout() override;
95- const uint32_t padding_;
96+ const int padding_;
97 int contents_width_;
98 int contents_height_;
99
100
101=== modified file 'src/ui_basic/tabpanel.cc'
102--- src/ui_basic/tabpanel.cc 2017-05-14 04:38:39 +0000
103+++ src/ui_basic/tabpanel.cc 2017-06-28 10:47:49 +0000
104@@ -87,27 +87,12 @@
105 * =================
106 */
107 /**
108- * Initialize an empty TabPanel
109+ * Initialize an empty TabPanel. We use width == 0 as an indicator that the size hasn't been set yet.
110 */
111 TabPanel::TabPanel(Panel* const parent,
112- int32_t const x,
113- int32_t const y,
114- const Image* background,
115- TabPanel::Type border_type)
116- : Panel(parent, x, y, 0, 0),
117- border_type_(border_type),
118- active_(0),
119- highlight_(kNotFound),
120- pic_background_(background) {
121-}
122-TabPanel::TabPanel(Panel* const parent,
123- int32_t const x,
124- int32_t const y,
125- int32_t const w,
126- int32_t const h,
127- const Image* background,
128- TabPanel::Type border_type)
129- : Panel(parent, x, y, w, h),
130+ const Image* background,
131+ TabPanel::Type border_type)
132+ : Panel(parent, 0, 0, 0, 0),
133 border_type_(border_type),
134 active_(0),
135 highlight_(kNotFound),
136@@ -118,6 +103,10 @@
137 * Resize the visible tab based on our actual size.
138 */
139 void TabPanel::layout() {
140+ if (get_w() == 0) {
141+ // The size hasn't been set yet
142+ return;
143+ }
144 if (active_ < tabs_.size()) {
145 Panel* const panel = tabs_[active_]->panel;
146 uint32_t h = get_h();
147@@ -126,7 +115,7 @@
148 h = std::min(h, h - (kTabPanelButtonHeight + kTabPanelSeparatorHeight));
149 // If we have a border, we will also want some margin to the bottom
150 if (border_type_ == TabPanel::Type::kBorder) {
151- h = h - kTabPanelSeparatorHeight;
152+ h -= kTabPanelSeparatorHeight;
153 }
154 panel->set_size(get_w(), h);
155 }
156@@ -148,9 +137,7 @@
157 panel->get_desired_size(&panelw, &panelh);
158 // TODO(unknown): the panel might be bigger -> add a scrollbar in that case
159
160- if (panelw > w) {
161- w = panelw;
162- }
163+ w = std::max(w, panelw);
164 h += panelh;
165 }
166
167@@ -254,6 +241,11 @@
168 * Draw the buttons and the tab
169 */
170 void TabPanel::draw(RenderTarget& dst) {
171+ if (get_w() == 0) {
172+ // The size hasn't been set yet
173+ return;
174+ }
175+
176 // draw the background
177 static_assert(2 < kTabPanelButtonHeight, "assert(2 < kTabPanelButtonSize) failed.");
178 static_assert(4 < kTabPanelButtonHeight, "assert(4 < kTabPanelButtonSize) failed.");
179@@ -273,7 +265,7 @@
180 RGBColor black(0, 0, 0);
181
182 // draw the buttons
183- float x = 0;
184+ int x = 0;
185 int tab_width = 0;
186 for (size_t idx = 0; idx < tabs_.size(); ++idx) {
187 x = tabs_[idx]->get_x();
188
189=== modified file 'src/ui_basic/tabpanel.h'
190--- src/ui_basic/tabpanel.h 2017-05-21 18:15:17 +0000
191+++ src/ui_basic/tabpanel.h 2017-06-28 10:47:49 +0000
192@@ -88,16 +88,6 @@
193 friend struct Tab;
194
195 TabPanel(Panel* parent,
196- int32_t x,
197- int32_t y,
198- const Image* background,
199- TabPanel::Type border_type = TabPanel::Type::kNoBorder);
200- // For Fullscreen menus
201- TabPanel(Panel* parent,
202- int32_t x,
203- int32_t y,
204- int32_t w,
205- int32_t h,
206 const Image* background,
207 TabPanel::Type border_type = TabPanel::Type::kNoBorder);
208
209
210=== modified file 'src/ui_fsmenu/about.cc'
211--- src/ui_fsmenu/about.cc 2017-03-02 08:06:00 +0000
212+++ src/ui_fsmenu/about.cc 2017-06-28 10:47:49 +0000
213@@ -29,10 +29,6 @@
214 title_(this, 0, 0, _("About Widelands"), UI::Align::kCenter),
215 close_(this, "close", 0, 0, 0, 0, g_gr->images().get("images/ui_basic/but2.png"), _("Close")),
216 tabs_(this,
217- 0,
218- 0,
219- 0,
220- 0,
221 g_gr->images().get("images/ui_basic/but1.png"),
222 UI::TabPanel::Type::kBorder) {
223 title_.set_fontsize(UI_FONT_SIZE_BIG);
224
225=== modified file 'src/ui_fsmenu/options.cc'
226--- src/ui_fsmenu/options.cc 2017-06-23 16:16:28 +0000
227+++ src/ui_fsmenu/options.cc 2017-06-28 10:47:49 +0000
228@@ -119,10 +119,6 @@
229
230 // Tabs
231 tabs_(this,
232- 0,
233- 0,
234- 100, // 100 is arbitrary, will be resized in layout().
235- 100,
236 g_gr->images().get("images/ui_basic/but1.png"),
237 UI::TabPanel::Type::kBorder),
238
239
240=== modified file 'src/wui/building_statistics_menu.cc'
241--- src/wui/building_statistics_menu.cc 2017-06-24 20:22:19 +0000
242+++ src/wui/building_statistics_menu.cc 2017-06-28 10:47:49 +0000
243@@ -65,7 +65,7 @@
244 kWindowWidth,
245 kWindowHeight,
246 _("Building Statistics")),
247- tab_panel_(this, 0, 0, g_gr->images().get("images/ui_basic/but1.png")),
248+ tab_panel_(this, g_gr->images().get("images/ui_basic/but1.png")),
249 navigation_panel_(this, 0, 0, kWindowWidth, 4 * kButtonRowHeight),
250 building_name_(
251 &navigation_panel_, get_inner_w() / 2, 0, 0, kButtonHeight, "", UI::Align::kCenter),
252
253=== modified file 'src/wui/buildingwindow.cc'
254--- src/wui/buildingwindow.cc 2017-06-23 16:16:28 +0000
255+++ src/wui/buildingwindow.cc 2017-06-28 10:47:49 +0000
256@@ -98,7 +98,7 @@
257
258 vbox_.reset(new UI::Box(this, 0, 0, UI::Box::Vertical));
259
260- tabs_ = new UI::TabPanel(vbox_.get(), 0, 0, nullptr);
261+ tabs_ = new UI::TabPanel(vbox_.get(), nullptr);
262 vbox_->add(tabs_, UI::Box::Resizing::kFullSize);
263
264 capsbuttons_ = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
265
266=== modified file 'src/wui/economy_options_window.cc'
267--- src/wui/economy_options_window.cc 2017-02-25 13:27:40 +0000
268+++ src/wui/economy_options_window.cc 2017-06-28 10:47:49 +0000
269@@ -38,7 +38,7 @@
270 : UI::Window(parent, "economy_options", 0, 0, 0, 0, _("Economy options")),
271 economy_number_(economy->owner().get_economy_number(economy)),
272 owner_(economy->owner()),
273- tabpanel_(this, 0, 0, g_gr->images().get("images/ui_basic/but1.png")),
274+ tabpanel_(this, g_gr->images().get("images/ui_basic/but1.png")),
275 ware_panel_(
276 new EconomyOptionsPanel(&tabpanel_, can_act, Widelands::wwWARE, economy_number_, owner_)),
277 worker_panel_(new EconomyOptionsPanel(
278
279=== modified file 'src/wui/encyclopedia_window.cc'
280--- src/wui/encyclopedia_window.cc 2017-02-26 12:34:43 +0000
281+++ src/wui/encyclopedia_window.cc 2017-06-28 10:47:49 +0000
282@@ -57,7 +57,7 @@
283 LuaInterface* const lua)
284 : UI::UniqueWindow(&parent, "encyclopedia", &registry, WINDOW_WIDTH, WINDOW_HEIGHT, ""),
285 lua_(lua),
286- tabs_(this, 0, 0, nullptr) {
287+ tabs_(this, nullptr) {
288 }
289
290 void EncyclopediaWindow::init(InteractiveBase& parent, std::unique_ptr<LuaTable> table) {
291
292=== modified file 'src/wui/fieldaction.cc'
293--- src/wui/fieldaction.cc 2017-05-21 11:16:46 +0000
294+++ src/wui/fieldaction.cc 2017-06-28 10:47:49 +0000
295@@ -249,7 +249,7 @@
296 map_(&ib->egbase().map()),
297 field_overlay_manager_(*ib->mutable_field_overlay_manager()),
298 node_(ib->get_sel_pos().node, &(*map_)[ib->get_sel_pos().node]),
299- tabpanel_(this, 0, 0, g_gr->images().get("images/ui_basic/but1.png")),
300+ tabpanel_(this, g_gr->images().get("images/ui_basic/but1.png")),
301 fastclick_(true),
302 best_tab_(0),
303 workarea_preview_overlay_id_(0),
304
305=== modified file 'src/wui/game_debug_ui.cc'
306--- src/wui/game_debug_ui.cc 2017-06-24 11:18:12 +0000
307+++ src/wui/game_debug_ui.cc 2017-06-28 10:47:49 +0000
308@@ -139,7 +139,7 @@
309 : UI::Window(&parent, "map_object_debug", 0, 0, 100, 100, ""),
310 log_general_info_(true),
311 object_(&obj),
312- tabs_(this, 0, 0, g_gr->images().get("images/ui_basic/but4.png")) {
313+ tabs_(this, g_gr->images().get("images/ui_basic/but4.png")) {
314 serial_ = obj.serial();
315 set_title(std::to_string(serial_));
316
317
318=== modified file 'src/wui/stock_menu.cc'
319--- src/wui/stock_menu.cc 2017-01-25 18:55:59 +0000
320+++ src/wui/stock_menu.cc 2017-06-28 10:47:49 +0000
321@@ -35,7 +35,7 @@
322 StockMenu::StockMenu(InteractivePlayer& plr, UI::UniqueWindow::Registry& registry)
323 : UI::UniqueWindow(&plr, "stock_menu", &registry, 480, 640, _("Stock")), player_(plr) {
324 UI::TabPanel* tabs =
325- new UI::TabPanel(this, 0, 0, g_gr->images().get("images/ui_basic/but1.png"));
326+ new UI::TabPanel(this, g_gr->images().get("images/ui_basic/but1.png"));
327 set_center_panel(tabs);
328
329 all_wares_ = new WaresDisplay(tabs, 0, 0, plr.player().tribe(), Widelands::wwWARE, false);
330
331=== modified file 'src/wui/ware_statistics_menu.cc'
332--- src/wui/ware_statistics_menu.cc 2017-05-02 12:09:44 +0000
333+++ src/wui/ware_statistics_menu.cc 2017-06-28 10:47:49 +0000
334@@ -121,7 +121,7 @@
335 // Setup plot widgets
336 // Create a tabbed environment for the different plots
337 UI::TabPanel* tabs =
338- new UI::TabPanel(box, kSpacing, 0, g_gr->images().get("images/ui_basic/but1.png"));
339+ new UI::TabPanel(box, g_gr->images().get("images/ui_basic/but1.png"));
340
341 plot_production_ = new WuiPlotArea(tabs, 0, 0, kPlotWidth, kPlotHeight + kSpacing,
342 kStatisticsSampleTime, WuiPlotArea::Plotmode::kRelative);

Subscribers

People subscribed via source and target branches

to status/vote changes: