widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Review: Needs Fixing

Not sure how to do a code review, but lets try.

1) I looked at r8139, so the two newest commits aren't tested by me. But I looked at the diffs, they seem to be okay.

2) I am generally not sure what I should do. This is refactoring and no bug fix / new feature so I guess I look at the code and press some buttons in-game? Note that I don't know the window system.

3) The diff looks good to me, with the changes it is tidier than before.

4) While pressing buttons in-game I got a failed assert:

widelands: ../src/ui_basic/unique_window.cc:71: void UI::UniqueWindow::Registry::assign_toggle_button(UI::Button*): Assertion `!on_create' failed.

This happens reproducible when pressing the statistics-menu button in-game often very fast. The other buttons seem to be fine. Addition: Seems like it always happens when the menu is opened a second time, independent of the clicking speed.

5) For the user, the statistics-menu (list of buttons for submenus) and the tool-menu (list of buttons for submenus) in the editor look (more or less) the same. In the code they are quite different, maybe update the tool-menu, too?

6) Connected with the above: Why is the new new add_button() method in the class GameStatisticsMenu? Maybe extract it to a UI::ButtonWindow class or so? It is good enough for now, but when refactoring the tool-menu it would create code duplication.

7) Not really related to this branch since it also happens in trunk: When the "General Statistics" and the "Statistics Menu" are open, clicking on the "general" button in the menu closes the window as expected. However, the button is sometimes flickering shortly (unpressed, pressed again, unpressed).

8) In trunk, the buttons in toolbar are grouped by spaces. The branch does not do this. Is this intentional?
-- 
https://code.launchpad.net/~widelands-dev/widelands/toolbar_cleanup/+merge/311115
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/toolbar_cleanup.