widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Yes please. It makes for a confusing commit history (see my comments). I also like to "smuggle" in some small improvements here and there in a branch, but it must at least be documented and explained in the commit message. If the commit should break something, it will make it harder to find later on if it isn't commented.

Diff comments:

> 
> === modified file 'src/graphic/align.h'
> --- src/graphic/align.h	2017-01-25 18:55:59 +0000
> +++ src/graphic/align.h	2017-02-14 18:23:39 +0000
> @@ -24,29 +24,40 @@
>  
>  namespace UI {
>  
> +/**
> + * This Enum is a binary mix of one-dimensional and two-dimensional alignments.
> + *
> + * bits 0,1 values 0,1,2,3  are horizontal
> + * bits 2,3 values 0,4,8,12 are vertical
> + *
> + * mixed aligenments are results of a binary | operation.
> + */
> +
> + // TODO(klaus.halfmann): as this is not a real enum all compiler warnings about
> + // incomplete usage are useless.
> +
>  enum class Align {
> -	kLeft = 0,
> -	kHCenter = 1,
> -	kRight = 2,
> -	kHorizontal = 3,
> -
> -	kTop = 0,
> -	kVCenter = 4,
> -	kBottom = 8,
> -	kVertical = 12,
> -
> -	kTopLeft = 0,
> -	kCenterLeft = Align::kVCenter,
> -	kBottomLeft = Align::kBottom,
> -
> -	kTopCenter = Align::kHCenter,
> -	kCenter = Align::kHCenter | Align::kVCenter,
> -	kBottomCenter = Align::kHCenter | Align::kBottom,
> -
> -	kTopRight = Align::kRight,
> -	kCenterRight = Align::kRight | Align::kVCenter,
> -
> -	kBottomRight = Align::kRight | Align::kBottom,
> +	kLeft       = 0x00,
> +	kHCenter    = 0x01,
> +	kRight      = 0x02,
> +	kHorizontal = 0x03,
> +
> +	kTop        = 0x00,
> +	kVCenter    = 0x04,
> +	kBottom     = 0x08,
> +	kVertical   = 0x0C,
> +
> +	kTopLeft        = kLeft | kTop,
> +	kCenterLeft     = kLeft | kVCenter,
> +	kBottomLeft     = kLeft | kBottom,
> +
> +    kTopCenter      = kHCenter | kTop,
> +	kCenter         = kHCenter | kVCenter,
> +	kBottomCenter   = kHCenter | kBottom,
> +
> +	kTopRight       = kRight | kTop,
> +	kCenterRight    = kRight | kVCenter,
> +	kBottomRight    = kRight | kBottom,
>  };

Please leave the comments in, but what does the conversion to hex gain us? It makes the code harder to read, so there must be a good reason to include this.

>  
>  inline Align operator&(Align a, Align b) {
> 
> === modified file 'src/wui/building_ui.cc'
> --- src/wui/building_ui.cc	2017-01-25 18:55:59 +0000
> +++ src/wui/building_ui.cc	2017-02-14 18:23:39 +0000
> @@ -68,7 +68,6 @@
>  	if (optionswindow_) {
>  		Vector2i window_position = optionswindow_->get_pos();
>  		hide_options();
> -		show_options(igb, true);
> -		optionswindow_->set_pos(window_position);
> +		show_options(igb, true, window_position);

There should at least be a mention in the commit message that this was changed and why.

This change will become obsolete with https://code.launchpad.net/~widelands-dev/widelands/notifications_buildingwindows/+merge/317264 anyway, so less work for you if you just remove it.

>  	}
>  }


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1658489-epedition-tab/+merge/317047
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1658489-epedition-tab.