Moved tab_index to Style (instead of Field), it makes more sense there, since it pertains to the layout of stuff on the card.

Also don't precalculate list of fields sorted by tab order, just find the next/previous ones on the fly.

doc/type/field.txt

@@ -41,7 +41,6 @@ Fields are part of the [[file:style triangle]]:
 | @card list name@	[[type:string]]		field name	Alternate name to use for the card list, for example an abbreviation.
 | @card list alignment@	[[type:alignment]]	@left@		Alignment of the card list column.
 | @sort script@		[[type:script]]		 		Alternate way to sort the card list when using this column to sort the list.
-| @tab index@		[[type:int]]		@0@		Index for moving through the fields with the tab key. The default is from left to right and then top to bottom.
 
 The @type@ determines what values of this field contain:
 ! Type			Values contain								Displayed as

doc/type/style.txt

@@ -68,6 +68,7 @@ Here are some examples:
 --Properties--
 ! Property		Type					Default		Description
 | @z index@		[[type:int]]				@0@		Stacking of this box, fields with a higher @z index@ are placed on top of those with a lower index.
+| @tab index@		[[type:int]]				@0@		Index for moving through the fields with the tab key, fields with a lower tab index come first. Otherwise the order is from top to bottom and then left to right.
 | @left@		[[type:scriptable]] [[type:double]]	''Required''	Distance between left edge of the box and the left of the card in pixels.
 | @width@		[[type:scriptable]] [[type:double]]	''Required''	Width of the box in pixels.
 | @right@		[[type:scriptable]] [[type:double]]	''Required''	Distance between right edge of the box and the ''left'' of the card in pixels.

src/data/field.cpp

@@ -33,7 +33,6 @@ Field::Field()
   , card_list_visible(false)
   , card_list_allow  (true)
   , card_list_align  (ALIGN_LEFT)
-  , tab_index        (0)
 {}
 
 Field::~Field() {}
@@ -64,7 +63,6 @@ IMPLEMENT_REFLECTION(Field) {
   REFLECT(card_list_name);
   REFLECT(sort_script);
   REFLECT_N("card_list_alignment", card_list_align);
-  REFLECT(tab_index);
   REFLECT_IF_READING if(caption.empty()) caption = name_to_caption(name);
   REFLECT_IF_READING if(card_list_name.empty()) card_list_name = capitalize(caption);
 }
@@ -97,6 +95,7 @@ intrusive_ptr<Field> read_new<Field>(Reader& reader) {
 Style::Style(const FieldP& field)
   : fieldP(field)
   , z_index(0)
+  , tab_index(0)
   , left (1000000), top   (1000000)
   , width(0),       height(0)
   , right(1000000), bottom(1000000)
@@ -110,6 +109,7 @@ Style::~Style() {}
 
 IMPLEMENT_REFLECTION(Style) {
   REFLECT(z_index);
+  REFLECT(tab_index);
   REFLECT(left);
   REFLECT(width);
   REFLECT(right);

src/data/field.hpp

@@ -58,7 +58,6 @@ class Field : public IntrusivePtrVirtualBase {
   String    card_list_name;   ///< Alternate name to use in card list.
   Alignment card_list_align;  ///< Alignment of the card list colummn.
   OptionalScript sort_script; ///< The script to use when sorting this, if not the value.
-  int       tab_index;        ///< Tab index in editor
   Dependencies dependent_scripts; ///< Scripts that depend on values of this field
   
   /// Creates a new Value corresponding to this Field
@@ -98,6 +97,7 @@ class Style : public IntrusivePtrVirtualBase {
   const FieldP       fieldP;          ///< Field this style is for, should have the right type!
   
   int                z_index;         ///< Stacking of values of this field, higher = on top
+  int                tab_index;       ///< Tab index in editor
   Scriptable<double> left,  top;      ///< Position of this field
   Scriptable<double> width, height;   ///< Position of this field
   Scriptable<double> right, bottom;   ///< Position of this field

src/gui/control/card_editor.cpp

@@ -16,6 +16,7 @@
 #include <util/find_replace.hpp>
 #include <util/window_id.hpp>
 #include <wx/caret.h>
+#include <boost/iterator/filter_iterator.hpp>
 
 // ----------------------------------------------------------------------------- : DataEditor
 
@@ -55,21 +56,57 @@ bool DataEditor::viewerIsCurrent(const ValueViewer* viewer) const {
   return viewer == current_viewer && FindFocus() == this;
 }
 
-
 void DataEditor::addAction(unique_ptr<Action> action) {
   set->actions.addAction(move(action));
 }
 
+// ----------------------------------------------------------------------------- : Algorithms
+
+/// Swap the order of comparison, i.e. greater-than instead of less-than
+template <typename Comp>
+struct SwapCompare {
+  Comp comp;
+  SwapCompare(Comp comp) : comp(comp) {}
+  template <typename T, typename U> inline bool operator () (T x, U y) {
+    return comp(y,x);
+  }
+};
+
+// Return the next element in a collection after x, when using comp as ordering
+template <typename It, typename V, typename Comp>
+It next_element(It first, It last, V const& x, Comp comp) {
+  It best = last;
+  for (It it = first ; it != last ; ++it) {
+    if (comp(x, *it)) {
+      // this is a candidate
+      if (best == last || comp(*it, *best)) {
+        best = it;
+      }
+    }
+  }
+  return best;
+}
+
+template <typename It, typename V, typename Comp>
+It prev_element(It first, It last, V const& x, Comp comp) {
+  return next_element(first, last, x, SwapCompare<Comp>(comp));
+}
+
 // ----------------------------------------------------------------------------- : Selection
 
 bool DataEditor::AcceptsFocus() const {
   return wxWindow::AcceptsFocus();
 }
 
-void DataEditor::select(ValueViewer* v) {
+void DataEditor::select(ValueViewer* new_viewer) {
   ValueEditor* old_editor = current_editor;
-  current_viewer = v;
-  current_editor = v->getEditor();
+  if (new_viewer) {
+    current_viewer = new_viewer;
+    current_editor = new_viewer->getEditor();
+  } else {
+    current_viewer = nullptr;
+    current_editor = nullptr;
+  }
   if (current_editor != old_editor) {
     // selection has changed
     if (old_editor)     old_editor->onLoseFocus();
@@ -78,54 +115,19 @@ void DataEditor::select(ValueViewer* v) {
   }
 }
 
-void DataEditor::selectFirst() {
-  selectByTabPos(0, true);
-}
-void DataEditor::selectLast() {
-  selectByTabPos((int)by_tab_index.size() - 1, false);
-}
-bool DataEditor::selectNext() {
-  return selectByTabPos(currentTabPos() + 1, true);
-}
-bool DataEditor::selectPrevious() {
-  return selectByTabPos(currentTabPos() - 1, false);
-}
-
-bool DataEditor::selectByTabPos(int tab_pos, bool forward) {
-  while (tab_pos >= 0 && (size_t)tab_pos < by_tab_index.size()) {
-    ValueViewer* v = by_tab_index[tab_pos];
-    if (v->getField()->editable && v->getStyle()->isVisible()) {
-      select(v);
-      return true;
-    }
-    // not enabled, maybe the next one?
-    tab_pos += forward ? 1 : -1;
-  }
-  // deselect
-  if (current_editor) {
-    current_editor->onLoseFocus();
-    onChange();
-  }
-  current_viewer = nullptr;
-  current_editor = nullptr;
-  return false;
-}
-int DataEditor::currentTabPos() const {
-  int i = 0;
-  FOR_EACH_CONST(v, by_tab_index) {
-    if (v == current_viewer) return i;
-    ++i;
-  }
-  return -1;
-}
-
-struct CompareTabIndex {
+struct CompareTabOrder {
   bool operator() (ValueViewer* a, ValueViewer* b) {
+    assert(a && b);
     Style& as = *a->getStyle(), &bs = *b->getStyle();
-    Field& af = *as.fieldP,     &bf = *bs.fieldP;
-    if (af.tab_index < bf.tab_index) return true;
-    if (af.tab_index > bf.tab_index) return false;
-    if (fabs(as.top - bs.top) < 15) {
+    // if tab_index differs, use that
+    if (as.tab_index < as.tab_index) return true;
+    if (as.tab_index > as.tab_index) return false;
+    // otherwise look at the positions
+    // TODO: does this actually give a total order?
+    double vertical_overlap   = min(bs.bottom - as.top, as.bottom - bs.top);
+    double horizontal_overlap = min(bs.right - as.left, as.right - bs.left);
+    if (vertical_overlap > 0 && vertical_overlap > horizontal_overlap) {
+      // fields overlap (mostly) vertically
       // the fields are almost on the same 'row'
       // compare horizontally first
       if (as.left < bs.left) return true; // horizontal sorting
@@ -137,21 +139,65 @@ struct CompareTabIndex {
       if (as.top  > bs.top)  return false;
       if (as.left < bs.left) return true; // horizontal sorting
     }
-    return false;
+    // arbitrary order otherwise
+    return a < b;
+  }
+  bool operator() (ValueViewerP const& a, ValueViewer* b) {
+    return operator () (a.get(), b);
+  }
+  bool operator() (ValueViewer* a, ValueViewerP const& b) {
+    return operator () (a, b.get());
+  }
+  bool operator() (ValueViewerP const& a, ValueViewerP const& b) {
+    return operator () (a.get(), b.get());
   }
 };
-void DataEditor::createTabIndex() {
-  by_tab_index.clear();
-  FOR_EACH(v, viewers) {
-    ValueEditor* e = v->getEditor();
-    if (e) {
-      by_tab_index.push_back(v.get());
-    }
-  }
-  stable_sort(by_tab_index.begin(), by_tab_index.end(), CompareTabIndex());
+
+bool is_enabled(ValueViewerP const& v) {
+  return v->getField()->editable && v->getStyle()->isVisible();
 }
+
+bool DataEditor::selectWithTab(vector<ValueViewerP>::iterator const& it) {
+  if (it != viewers.end()) {
+    select(it->get());
+    return true;
+  } else {
+    select(nullptr);
+    return false;
+  }
+}
+
+bool DataEditor::selectFirst() {
+  // This would be nicer with boost::range, but filtered adaptor was only introduced in boost 1.42(?)
+  return selectWithTab(std::min_element(
+    boost::make_filter_iterator(is_enabled, viewers.begin(), viewers.end()),
+    boost::make_filter_iterator(is_enabled, viewers.end(), viewers.end()),
+    CompareTabOrder()).base());
+}
+bool DataEditor::selectLast() {
+  return selectWithTab(std::max_element(
+    boost::make_filter_iterator(is_enabled, viewers.begin(), viewers.end()),
+    boost::make_filter_iterator(is_enabled, viewers.end(), viewers.end()),
+    CompareTabOrder()).base());
+}
+bool DataEditor::selectNext() {
+  if (!current_viewer) return selectFirst();
+  return selectWithTab(next_element(
+    boost::make_filter_iterator(is_enabled, viewers.begin(), viewers.end()),
+    boost::make_filter_iterator(is_enabled, viewers.end(), viewers.end()),
+    current_viewer,
+    CompareTabOrder()).base());
+}
+bool DataEditor::selectPrevious() {
+  if (!current_viewer) return selectLast();
+  return selectWithTab(prev_element(
+    boost::make_filter_iterator(is_enabled, viewers.begin(), viewers.end()),
+    boost::make_filter_iterator(is_enabled, viewers.end(), viewers.end()),
+    current_viewer,
+    CompareTabOrder()).base());
+}
+
 void DataEditor::onInit() {
-  createTabIndex();
   current_viewer = nullptr;
   current_editor = nullptr;
   hovered_viewer = nullptr;

src/gui/control/card_editor.hpp

@@ -34,9 +34,9 @@ class DataEditor : public CardViewer {
   /// Select the given viewer, sends focus events
   void select(ValueViewer* v);
   /// Select the first editable and visible editor (by tab index)
-  void selectFirst();
+  bool selectFirst();
   /// Select the last editable and visible editor (by tab index)
-  void selectLast();
+  bool selectLast();
   /// Select the next editable editor, returns false if the current editor is the last one
   bool selectNext();
   /// Select the previous editable editor, returns false if the current editor is the first one
@@ -127,13 +127,8 @@ class DataEditor : public CardViewer {
   /// Convert mouse coordinates to internal coordinates
   RealPoint mousePoint(const wxMouseEvent&, const ValueViewer& viewer);
   
-  // Create tab index ordering of the (editable) viewers
-  void createTabIndex();
-  /// Select the field with the given position in the by_tab_index list
-  /** Returns success */
-  bool selectByTabPos(int tab_pos, bool forward = true);
-  /// Find the tab pos of the current viewer, returns -1 if not found
-  int currentTabPos() const;
+  /// Select a field found by tab order, can be viewers.end()
+  bool selectWithTab(vector<ValueViewerP>::iterator const&);
 };
 
 /// By default a DataEditor edits cards