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
> +0000
> +++ src/editor/ui_menus/main_menu_load_or_save_map.cc 2017-02-21 15:47:40
> +0000
> @@ -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
> +0000
> +++ src/editor/ui_menus/main_menu_random_map.cc 2017-02-21 15:47:40
> +0000
> @@ -182,46 +181,39 @@
> mountains_box_(&box_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> mountains_label_(&mountains_box_, 0, 0, _("Mountains:")),
> 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_(&box_, Vector2i(0, 0), _("Island mode")),
> // Geeky stuff
> map_number_box_(&box_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> map_number_label_(&map_number_box_, 0, 0, _("Random Number:")),
> map_number_edit_(&map_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_(&box_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> map_id_label_(&map_id_box_, 0, 0, _("Map ID:")),
> map_id_edit_(&map_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_(&box_, 0, 0, UI::Box::Horizontal, 0, 0, margin_),
> ok_button_(&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_(&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 +0000
> +++ src/editor/ui_menus/tool_noise_height_options_menu.cc 2017-02-21
> 15:47:40 +0000
> @@ -95,15 +95,15 @@
> UI::Textarea* label =
> new UI::Textarea(&box_, 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(&upper_, UI::Align::kLeft);
> - box_.add(&lower_, UI::Align::kLeft);
> + box_.add(label, UI::HAlign::kLeft);
> + box_.add(&upper_, UI::HAlign::kLeft);
> + box_.add(&lower_, UI::HAlign::kLeft);
>
> box_.add_space(2 * vspacing());
> label = new UI::Textarea(&box_, 0, 0, 0, 0, _("Fixed Height"),
> UI::Align::kCenter);
UI::HAlign::kCenter
> label->set_fixed_width(get_inner_w() - 2 * hmargin());
> - box_.add(label, UI::Align::kLeft);
> - box_.add(&set_to_, UI::Align::kLeft);
> + box_.add(label, UI::HAlign::kLeft);
> + box_.add(&set_to_, UI::HAlign::kLeft);
>
> box_.set_size(get_inner_w() - 2 * hmargin(), upper_.get_h() +
> lower_.get_h() + set_to_.get_h() +
> 2 * label->get_h() + 7
> * vspacing());
>
> === modified file 'src/graphic/wordwrap.h'
> --- src/graphic/wordwrap.h 2017-01-25 18:55:59 +0000
> +++ src/graphic/wordwrap.h 2017-02-21 15:47:40 +0000
> @@ -51,7 +51,7 @@
>
> void draw(RenderTarget& dst,
> Vector2i where,
> - Align align = UI::Align::kLeft,
> + Align align = UI::Align::kTopLeft,
UI::HAlign::kLeft
> uint32_t caret = std::numeric_limits<uint32_t>::max());
>
> void calc_wrapped_pos(uint32_t caret, uint32_t& line, uint32_t& pos)
> const;
>
> === modified file 'src/logic/map_objects/map_object.cc'
> --- src/logic/map_objects/map_object.cc 2017-01-25 18:55:59 +0000
> +++ src/logic/map_objects/map_object.cc 2017-02-21 15:47:40 +0000
> @@ -482,7 +482,7 @@
> round(census_pos + Vector2f(0,
> rendered_census_info->height() / 2.f + 10 * scale))
> .cast<float>();
> dst->blit(statistics_pos,
> - UI::g_fh1->render(as_condensed(statictics,
> UI::Align::kLeft, font_size)),
> + UI::g_fh1->render(as_condensed(statictics,
> UI::HAlign::kLeft, font_size)),
> BlendMode::UseAlpha, UI::Align::kCenter);
UI::HAlign::kCenter
> }
> }
>
> === modified file 'src/ui_basic/box.cc'
> --- src/ui_basic/box.cc 2017-02-14 18:11:53 +0000
> +++ src/ui_basic/box.cc 2017-02-21 15:47:40 +0000
> @@ -376,15 +376,15 @@
> maxbreadth = get_inner_w();
> }
> switch (it.u.panel.align) {
> - case UI::Align::kHCenter:
> + case UI::HAlign::kHCenter:
> breadth = (maxbreadth - breadth) / 2;
> break;
>
> - case UI::Align::kRight:
> + case UI::HAlign::kRight:
> breadth = maxbreadth - breadth;
> break;
> - case UI::Align::kLeft:
> - default:
> + case UI::HAlign::kLeft:
> + case UI::HAlign::kHorizontal: // should not be used but makes
> the enum complete
Get rid of this in the enum.
> breadth = 0;
> }
>
>
> === modified file 'src/ui_basic/checkbox.cc'
> --- src/ui_basic/checkbox.cc 2017-02-12 09:10:57 +0000
> +++ src/ui_basic/checkbox.cc 2017-02-21 15:47:40 +0000
> @@ -153,7 +153,7 @@
> image_anchor.x = rendered_text_->width() +
> kPadding;
> image_anchor.y = (get_h() - kStateboxSize) / 2;
> }
> - dst.blit(text_anchor, rendered_text_,
> BlendMode::UseAlpha, UI::Align::kLeft);
> + dst.blit(text_anchor, rendered_text_,
> BlendMode::UseAlpha, UI::Align::kTopLeft);
UI::HAlign::kLeft
> }
>
> dst.blitrect(
>
> === modified file 'src/ui_basic/fullscreen_window.cc'
> --- src/ui_basic/fullscreen_window.cc 2017-01-25 18:55:59 +0000
> +++ src/ui_basic/fullscreen_window.cc 2017-02-21 15:47:40 +0000
> @@ -93,14 +93,14 @@
> }
>
> // Frame edges
> - blit_image(dst,
> get_frame_image(FullscreenWindow::Frames::kEdgeLeftTile), UI::Align::kTopLeft,
> - UI::Align::kVertical);
> - blit_image(dst,
> get_frame_image(FullscreenWindow::Frames::kEdgeRightTile),
> UI::Align::kTopRight,
> - UI::Align::kVertical);
> - blit_image(dst,
> get_frame_image(FullscreenWindow::Frames::kEdgeTopTile), UI::Align::kTopLeft,
> - UI::Align::kHorizontal);
> + blit_image(dst,
> get_frame_image(FullscreenWindow::Frames::kEdgeLeftTile),
> + UI::Align::kTopLeft, UI::VAlign::kVertical);
I'll need to take a closer look at these - seems like horizonal/vertical might
be needed here after all. We might split this into 2 parameters for HAlign,
VAlign instead though.
> + blit_image(dst,
> get_frame_image(FullscreenWindow::Frames::kEdgeRightTile),
> + UI::Align::kTopRight, UI::VAlign::kVertical);
> + blit_image(dst, get_frame_image(FullscreenWindow::Frames::kEdgeTopTile),
> + UI::Align::kTopLeft, UI::HAlign::kHorizontal);
> blit_image(dst,
> get_frame_image(FullscreenWindow::Frames::kEdgeBottomTile),
> - UI::Align::kBottomLeft, UI::Align::kHorizontal);
> + UI::Align::kBottomLeft, UI::HAlign::kHorizontal);
>
> // Frame corners
> blit_image(dst,
> get_frame_image(FullscreenWindow::Frames::kCornerTopLeft),
> UI::Align::kTopLeft);
>
> === modified file 'src/ui_basic/multilineeditbox.cc'
> --- src/ui_basic/multilineeditbox.cc 2017-01-25 18:55:59 +0000
> +++ src/ui_basic/multilineeditbox.cc 2017-02-21 15:47:40 +0000
> @@ -450,7 +450,7 @@
>
> d_->ww.set_draw_caret(has_focus());
>
> - d_->ww.draw(dst, Vector2i(0, -int32_t(d_->scrollbar.get_scrollpos())),
> UI::Align::kLeft,
> + d_->ww.draw(dst, Vector2i(0, -int32_t(d_->scrollbar.get_scrollpos())),
> UI::Align::kTopLeft,
UI::HAlign::kLeft
> has_focus() ? d_->cursor_pos :
> std::numeric_limits<uint32_t>::max());
> }
>
>
> === modified file 'src/ui_basic/slider.cc'
> --- src/ui_basic/slider.cc 2017-02-18 14:03:02 +0000
> +++ src/ui_basic/slider.cc 2017-02-21 15:47:40 +0000
> @@ -540,8 +539,8 @@
> for (uint32_t i = 0; i < labels.size(); i++) {
> dst.blit(Vector2f(gap_1 + i * gap_n, get_h()),
> UI::g_fh1->render(
> - as_condensed(labels[i], UI::Align::kBottomCenter,
> UI_FONT_SIZE_SMALL - 2)),
> - BlendMode::UseAlpha, UI::Align::kBottomCenter);
> + as_condensed(labels[i], UI::HAlign::kHCenter,
> UI_FONT_SIZE_SMALL - 2)),
> + BlendMode::UseAlpha, UI::Align::kCenter);
UI::HAlign::kCenter
> }
> }
>
>
> === modified file 'src/ui_basic/spinbox.cc'
> --- src/ui_basic/spinbox.cc 2017-01-25 18:55:59 +0000
> +++ src/ui_basic/spinbox.cc 2017-02-21 15:47:40 +0000
> @@ -110,9 +110,9 @@
> box_ = new UI::Box(this, 0, 0, UI::Box::Horizontal, 0, 0, padding_);
>
> sbi_->label =
> - new UI::MultilineTextarea(box_, 0, 0, 0, 0, label_text,
> UI::Align::kLeft, button_background,
> + new UI::MultilineTextarea(box_, 0, 0, 0, 0, label_text,
> UI::HAlign::kLeft, button_background,
>
> UI::MultilineTextarea::ScrollMode::kNoScrolling);
> - box_->add(sbi_->label, UI::Align::kHCenter);
> + box_->add(sbi_->label, UI::HAlign::kHCenter);
>
> sbi_->text = new UI::Textarea(box_, "", UI::Align::kCenter);
UI::HAlign::kCenter
>
>
> === modified file 'src/ui_basic/tabpanel.cc'
> --- src/ui_basic/tabpanel.cc 2017-02-12 09:10:57 +0000
> +++ src/ui_basic/tabpanel.cc 2017-02-21 15:47:40 +0000
> @@ -302,7 +302,7 @@
> } else {
> dst.blit(Vector2f(x + kTabPanelTextMargin,
> (kTabPanelButtonHeight -
> tabs_[idx]->pic->height()) / 2),
> - tabs_[idx]->pic, BlendMode::UseAlpha,
> UI::Align::kLeft);
> + tabs_[idx]->pic, BlendMode::UseAlpha,
> UI::Align::kTopLeft);
UI::HAlign::kLeft
> }
>
> // Draw top part of border
>
> === modified file 'src/ui_fsmenu/campaign_select.cc'
> --- src/ui_fsmenu/campaign_select.cc 2017-01-25 18:55:59 +0000
> +++ src/ui_fsmenu/campaign_select.cc 2017-02-21 15:47:40 +0000
> @@ -45,10 +45,10 @@
> table_(this, tablex_, tabley_, tablew_, tableh_),
>
> // Main Title
> - title_(this, get_w() / 2, tabley_ / 3, _("Choose a campaign"),
> UI::Align::kHCenter),
> + title_(this, get_w() / 2, tabley_ / 3, _("Choose a campaign"),
> UI::Align::kTopCenter),
UI::HAlign::kCenter
>
> // Campaign description
> - label_campname_(this, right_column_x_, tabley_, "", UI::Align::kLeft),
> + label_campname_(this, right_column_x_, tabley_, "",
> UI::Align::kTopLeft),
UI::HAlign::kLeft
> ta_campname_(this,
> right_column_x_ + indent_,
> get_y_from_preceding(label_campname_) + padding_,
--
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
