I have numbered points to discuss for easier reference.


+	if (result == HEBR_CANCEL)
+		this->UnfocusFocusedWidget();

1. Coding style.
   Never indent another statement below an if or else in this way. Either put
   it all at one line, or use curly braces.


 /**
+ * Focus the next widget that accepts keyboard input.
+ */
+void Window::FocusNextEditWidget()
+{
+	uint start_find_at = 0;
+	uint num_loops = this->nested_array_size;
+
+	if (this->nested_focus != NULL) {
+		start_find_at = this->nested_focus->index + 1 % this->nested_array_size;
+		num_loops = this->nested_array_size - 1;
+	}

6. No, not a mistake in numbering, consider this after fixing 4 and 5 at least.
   The '%' seems a bit expensive for an increment-ish operation.

   (really, stop reading now, and do 4 and 5 first, the text below will not make sense otherwise).

   Now if you did 4 and 5 in the way I imagine, the end of the loop does pretty
   much the same operation you are doing above (namely incrementing the widget
   index, and decrementing the loop count).

   In the ideal case, you would swap the edit-box test and the
   increment/decrement operation in the loop, and you're done. Unfortunately,
   that does not work here due to the fact that the above 'if' may not hold.

   Fixing the initialization code above the 'if' so you can first do an increment/decrement
   in the loop before doing the first test also seems a bit problematic.

   The third option (and probably the best one here) seems to make the end of
   the 'if' above look as much like the end of the 'for' loop as you can. It
   looks like you are duplicating code, but in fact, any decent compiler will
   recognize it, and generate a jump into the middle of the loop instead.

+
+	uint widget_index = start_find_at;
+	for (uint i = 0; i < num_loops; i++) {
+		const NWidgetBase *widget = this->GetWidget<NWidgetBase>(widget_index);
+
+		/* Focus next edit box */
+		if (widget != NULL && widget->type == WWT_EDITBOX) {
+			this->SetFocusedWidget(widget_index);
+			break;
+		}


+		/* Next widget index */
+		if (++widget_index == this->nested_array_size)
+			widget_index = 0;

2. Another coding style for the 'if' statement.

3. Having side effects such as pre-increment in a statement such as the 'if' above makes it harder to understand.
   Reduction of number of source code lines in exchange for increased complexity is almost never a good idea.
   Alternatively, you can use a '? :' statement instead, which does the test before the increment operation.

4. Look at how you use 'start_find_at' and 'widget_index'. The former is just
   the initial value of the latter. No need to have two variables for that.

5. Look at how you use 'num_loops' and 'i'. Also note that 'i' is not used
   anywhere else in the loop. If you decrement i instead of increment, you have
   the same case as 3.


+	/* Focus next edit widget in the focused widget on ctrl + tab */
+	if (keycode == (WKC_CTRL | WKC_TAB) && _focused_window != NULL) {
+		_focused_window->FocusNextEditWidget();
+		return;
+	}
+

7. Much better, isn't it?
   Now if I understand it correctly, ctl+tab does nothing when a window has
   only a single edit box. Is that intended behaviour?



From the 'Details' of the issue:

8. These two things are two code-wise unrelated features, but since I think
   they relate to each other in the way we want focusing to behave they are in the
   same patch file

   I'd suggest to split the patch in two pieces, especially since the "Pressing
   Ctrl + Tab key cycles the focus between the edit boxes in the focused
   window" functionality gives no benefits at all at this time, and thus that
   part will not go into trunk. (Less code in trunk is better for several
   reasons, so code must have a real purpose before adding it.)

8b. "I don't think the patch is useless before there is more edit boxes on the
   same window. It is true that it has been written to be future safe for when
   that is the case. ..."

   Being future-safe is not enough reason to include code. Nor is code that
   does things in a more complicated way than strictly necessary today. The
   future tends to change in unexpected ways, so 'future-safe' is less safe
   then you may expect.
   So instead of coding future-safe, we add/modify code when we need it and not
   earlier.

8c. "... Right now it can be used to focus the only edit box in a
   window when it is not focused. ..."

   We already have that code, don't we?

8d. "... While many windows has code in their constructors to give focus to the
   edit box, that is not mandatory. I think there shouldn't be too many windows
   that or we will get problems with the hotkeys. The idea with this patch is
   to provide a standardized way to give focus to the edit boxes so that each
   window do not has to implement their own hot keys for that or give focus to
   the edit box by default."

   Making it mandatory seems like an alternative solution that works well
   today. You'll have to show the actual benefits of doing it your way for todays
   code.

   Note that the aim of the code is to have a nice game, not to write a generic
   windowing system. Custom solutions that cover todays needs are good enough,
   even if they violate the idea of a generic windowing system. (Until the
   generic solution is much better than the custom solution of course.)


From the 'Details' of the issue:

9. "The patch does not remove the code of the fund town window that unfocus the
   edit widget when you hit escape. This has to be done before the patch is done."

   This change seems to be still missing.