widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

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.