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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
