Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/align-align into lp:widelands
FullscreenWindow will need Tiling { kNone, kHorizontal, kVertical }
and FullscreenWindow actually needs the all the Align enums.
I will try what I can make of it.
--
https://code.launchpad.net/~widelands-dev/widelands/align-align/+merge/317871
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/align-align 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1663490-ship-windows into lp:widelands
Currently, create() is only called by the ship windows and by popup messages (e.g. the current status with the collectors win condition but not messages like "no rocks"). When the notifications_buildingwindows branch is merged, buildings will use it, too. I just tried (and pushed) a modification of create() where an existing window is focused when "created" a second time. In the case of ships this means that clicking on a ship will show the minimized window again, which is a feature I personally would like to have. With the collector-messages the change means that the message window will be focused again when previously minimized (but not closed). Whether we want this is up to discussion. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1663490-ship-windows/+merge/317051 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1663490-ship-windows. ___ 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/align-align into lp:widelands
I have looked at the codebase now - the only places where we use the
horizontal/vertical distinction is in wordwrap and in
FullscreenWindow::blit_image. We do not need vertical alignment at all.
The one in Wordwrap can go - we only use horizontal alignment there.
FullscreenWindow::blit_image can be refactored by supplying an extra enum class
locally there:
enum class Tiling { kHorizontal, kVertical }
Then we will be left with:
enum clas UI::Align { kLeft, kCenter, kRight }
--
https://code.launchpad.net/~widelands-dev/widelands/align-align/+merge/317871
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/align-align 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/hasimusic into lp:widelands
Continuous integration builds have changed state: Travis build 1971. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/203921992. Appveyor build 1806. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_hasimusic-1806. -- https://code.launchpad.net/~widelands-dev/widelands/hasimusic/+merge/317892 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/hasimusic. ___ 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/cleanup-networking-messages into lp:widelands
Review: Approve diff only I am a bit scared to say something here again, but: The diff looks good to me. ;-) I have neither tested nor compiled the changes, though. But grep couldn't find any of the to-be-removed strings in trunk or the metaserver source. So, uh... I guess it can be merged? I wanted to suggest waiting for travis, but somehow it seems to have lost interest on this branch. Maybe when I set the state to "approved"? -- https://code.launchpad.net/~widelands-dev/widelands/cleanup-networking-messages/+merge/316974 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/cleanup-networking-messages. ___ 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-1663490-ship-windows into lp:widelands
Or have a look at where create is used - maybe we do want to focus and maximize at all times anyway. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1663490-ship-windows/+merge/317051 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1663490-ship-windows. ___ 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-1663490-ship-windows into lp:widelands
How about adding a new action "Selected" to the ship notifications? This could then focus and maximize the window. create() is only used when the window has been closed. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1663490-ship-windows/+merge/317051 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1663490-ship-windows. ___ 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-1663490-ship-windows into lp:widelands
Review: Resubmit Okay, good news first: I am using the UniqueWindow now and the code looks much better. Thanks for the hint. Bad news: In my previous commit, clicking on a ship with the window in the background or minimized focused and showed the window. This is no longer the case with the UniqueWindow. The same problem (?) happens in the notifications branch for buildings, but works in trunk. I would suggest changing the UniqueWindow class to support this feature (by adapting UniqueWindow::Registry::create() to show the window when already existing). -- https://code.launchpad.net/~widelands-dev/widelands/bug-1663490-ship-windows/+merge/317051 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1663490-ship-windows. ___ 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/align-align into lp:widelands
Continuous integration builds have changed state: Travis build 1970. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/203874154. Appveyor build 1805. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_align_align-1805. -- https://code.launchpad.net/~widelands-dev/widelands/align-align/+merge/317871 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/align-align 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/align-align into lp:widelands
Hello Gun, thnaks for the first review, I was not sure where to use one dimension HAlign and where two Align, I will follow your suggestions and change the sigantures of the funktions involved. About the Bitmask: I will extract an anonymous enum and put them there, as they are needed for bitwiese fucntions (slice Align into HAlign / VALign). About using Hex: As these numbers should be seen as bits I like to use hex to emphasise this. Will start the next plow through the code tomorrow ... -- https://code.launchpad.net/~widelands-dev/widelands/align-align/+merge/317871 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/align-align 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/align-align into lp:widelands
First impression:
1. Why does HAlign have a Bitmask for horizontal, and VALign a bitmask for
Vertical? They are by definition horizontal/vertical. We should get rid of
these bitmasks.
2. I made Align an enum class on purpose for type safety - at some parts in the
code, people had hacked hard-coded ints in the past, which made the code both
fragile and harder to read. Having an enum class prevents people from doing
that. I have annotated the first 2000 lines - most of these would have been
flagged up by the compiler had you been using enum class. So, please use enum
class first and clean up the compiler errors, then I will have another look.
Let me know if you need any help with the trickier parts like the tiling in
fs_menu.
3. UI::HAlign::kHCenter can now be renamed to UI::HAlign::kCenter - easier to
read.
4. Is there a special reason for the hex notation?
5. Finally, we should analyze the remaining uses of Align to see if we still
need them.
Diff comments:
>
> === modified file 'src/editor/ui_menus/main_menu_load_or_save_map.cc'
> --- src/editor/ui_menus/main_menu_load_or_save_map.cc 2017-01-25 18:55:59
> +
> +++ src/editor/ui_menus/main_menu_load_or_save_map.cc 2017-02-21 15:47:40
> +
> @@ -57,7 +57,7 @@
>tableh_,
>MapDetails::Style::kWui),
> directory_info_(
> -this, padding_, get_inner_h() - 2 * buth_ - 4 * padding_, "",
> UI::Align::kLeft),
> +this, padding_, get_inner_h() - 2 * buth_ - 4 * padding_, "",
> UI::Align::kTopLeft),
UI::HAlign::kLeft
> ok_(this,
> "ok",
> UI::g_fh1->fontset()->is_rtl() ? get_inner_w() / 2 - butw_ -
> padding_ :
>
> === modified file 'src/editor/ui_menus/main_menu_random_map.cc'
> --- src/editor/ui_menus/main_menu_random_map.cc 2017-01-25 18:55:59
> +
> +++ src/editor/ui_menus/main_menu_random_map.cc 2017-02-21 15:47:40
> +
> @@ -182,46 +181,39 @@
> mountains_box_(_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> mountains_label_(_box_, 0, 0, _("Mountains:")),
> mountains_(_box_,
> -0,
> -0,
> +0, 0,
> box_width_ / 3,
> resources_label_.get_h(),
> (boost::format(_("%i %%")) % mountainsval_).str(),
> -UI::Align::kHCenter),
> +UI::Align::kTopCenter),
UI::HAlign::kCenter
> island_mode_(_, Vector2i(0, 0), _("Island mode")),
> // Geeky stuff
> map_number_box_(_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> map_number_label_(_number_box_, 0, 0, _("Random Number:")),
> map_number_edit_(_number_box_,
> - 0,
> - 0,
> + 0, 0,
>box_width_ - 2 * margin_ - map_number_label_.get_w(),
> - 0,
> - 2,
> + 0, 2,
>g_gr->images().get("images/ui_basic/but1.png")),
> map_id_box_(_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> map_id_label_(_id_box_, 0, 0, _("Map ID:")),
> map_id_edit_(_id_box_,
> - 0,
> - 0,
> + 0, 0,
>box_width_ - 2 * margin_ - map_id_label_.get_w(),
> - 0,
> - 2,
> + 0, 2,
>g_gr->images().get("images/ui_basic/but1.png")),
> // Buttons
> button_box_(_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> ok_button_(_box_,
> "generate_map",
> -0,
> -0,
> +0, 0,
> box_width_ / 2 - margin_,
> 0,
> g_gr->images().get("images/ui_basic/but5.png"),
> _("Generate Map")),
> cancel_button_(_box_,
> "generate_map",
> -0,
> -0,
> +0, 0,
> box_width_ / 2 - margin_,
> 0,
> g_gr->images().get("images/ui_basic/but1.png"),
>
> === modified file 'src/editor/ui_menus/tool_noise_height_options_menu.cc'
> --- src/editor/ui_menus/tool_noise_height_options_menu.cc 2017-01-25
> 18:55:59 +
> +++ src/editor/ui_menus/tool_noise_height_options_menu.cc 2017-02-21
> 15:47:40 +
> @@ -95,15 +95,15 @@
> UI::Textarea* label =
> new UI::Textarea(_, 0, 0, 0, 0, _("Random Height"),
> UI::Align::kCenter);
> label->set_fixed_width(get_inner_w() - 2 * hmargin());
> - box_.add(label, UI::Align::kLeft);
> - box_.add(_, UI::Align::kLeft);
> - box_.add(_, UI::Align::kLeft);
> + box_.add(label, UI::HAlign::kLeft);
> + box_.add(_, UI::HAlign::kLeft);
> + box_.add(_, UI::HAlign::kLeft);
>
> box_.add_space(2 * vspacing());
> label = new UI::Textarea(_, 0, 0, 0, 0, _("Fixed Height"),
> UI::Align::kCenter);
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/hasimusic into lp:widelands
Klaus Halfmann has proposed merging lp:~widelands-dev/widelands/hasimusic into lp:widelands. Requested reviews: toptopple (7010622-q) Related bugs: Bug #1549963 in widelands: "Proposal for a new Game music" https://bugs.launchpad.net/widelands/+bug/1549963 Bug #1556620 in widelands: "Silkweavers Song" https://bugs.launchpad.net/widelands/+bug/1556620 Bug #1568929 in widelands: "Widelands March" https://bugs.launchpad.net/widelands/+bug/1568929 For more details, see: https://code.launchpad.net/~widelands-dev/widelands/hasimusic/+merge/317892 After my Music was lingering around for some time now, I now added it to the Game. As no real player plays with music on this will not make widelands worse. It will however change the first impression, as I used my Widelands-Song as intro. -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/hasimusic. === renamed file 'data/music/intro_00.ogg' => 'data/music/ingame_23.ogg' === added file 'data/music/ingame_24.ogg' Binary files data/music/ingame_24.ogg 1970-01-01 00:00:00 + and data/music/ingame_24.ogg 2017-02-21 18:15:17 + differ === added file 'data/music/ingame_25.ogg' Binary files data/music/ingame_25.ogg 1970-01-01 00:00:00 + and data/music/ingame_25.ogg 2017-02-21 18:15:17 + differ === added file 'data/music/intro_00.ogg' Binary files data/music/intro_00.ogg 1970-01-01 00:00:00 + and data/music/intro_00.ogg 2017-02-21 18:15:17 + differ ___ 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
