From 9cb6facf507aff7cddd124b8c29714d2b0e7bd13 Mon Sep 17 00:00:00 2001
From: Wim Looman <wim@nemo157.com>
Date: Tue, 25 Oct 2016 22:50:29 +0200
Subject: [PATCH] fix: Derive display order after propagation

Don't attempt to change the display order of flags/options until any app
settings have been propagated down from a parent App in case DeriveDisplayOrder
and/or UnifiedHelpMessage are propagated.

Fixes #706
---
 src/app/mod.rs                 |  1 +
 src/app/parser.rs              | 37 ++++++++++++++++++----------------
 src/args/arg_builder/flag.rs   |  4 ++++
 src/args/arg_builder/option.rs |  3 +++
 4 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/src/app/mod.rs b/src/app/mod.rs
index 02f12b766f0..32e66d0d7b4 100644
--- a/src/app/mod.rs
+++ b/src/app/mod.rs
@@ -1318,6 +1318,7 @@ impl<'a, 'b> App<'a, 'b> {
         // before parsing incase we run into a subcommand
         self.p.propogate_globals();
         self.p.propogate_settings();
+        self.p.derive_display_order();
 
         let mut matcher = ArgMatcher::new();
 
diff --git a/src/app/parser.rs b/src/app/parser.rs
index 6c327e24f67..3f155dd0164 100644
--- a/src/app/parser.rs
+++ b/src/app/parser.rs
@@ -179,23 +179,11 @@ impl<'a, 'b> Parser<'a, 'b>
             self.positionals.insert(i, pb);
         } else if a.is_set(ArgSettings::TakesValue) {
             let mut ob = OptBuilder::from_arg(a, &mut self.required);
-            if self.settings.is_set(AppSettings::DeriveDisplayOrder) && a.disp_ord == 999 {
-                ob.disp_ord = if self.settings.is_set(AppSettings::UnifiedHelpMessage) {
-                    self.flags.len() + self.opts.len()
-                } else {
-                    self.opts.len()
-                };
-            }
+            ob.unified_ord = self.flags.len() + self.opts.len();
             self.opts.push(ob);
         } else {
             let mut fb = FlagBuilder::from(a);
-            if self.settings.is_set(AppSettings::DeriveDisplayOrder) && a.disp_ord == 999 {
-                fb.disp_ord = if self.settings.is_set(AppSettings::UnifiedHelpMessage) {
-                    self.flags.len() + self.opts.len()
-                } else {
-                    self.flags.len()
-                };
-            }
+            fb.unified_ord = self.flags.len() + self.opts.len();
             self.flags.push(fb);
         }
         if a.is_set(ArgSettings::Global) {
@@ -242,9 +230,6 @@ impl<'a, 'b> Parser<'a, 'b>
             sdebugln!("No");
         }
 
-        if self.settings.is_set(AppSettings::DeriveDisplayOrder) {
-            subcmd.p.meta.disp_ord = self.subcommands.len();
-        }
         self.subcommands.push(subcmd);
     }
 
@@ -275,6 +260,24 @@ impl<'a, 'b> Parser<'a, 'b>
         }
     }
 
+    pub fn derive_display_order(&mut self) {
+        if self.settings.is_set(AppSettings::DeriveDisplayOrder) {
+            let unified = self.settings.is_set(AppSettings::UnifiedHelpMessage);
+            for (i, o) in self.opts.iter_mut().enumerate().filter(|&(_, ref o)| o.disp_ord == 999) {
+                o.disp_ord = if unified { o.unified_ord } else { i };
+            }
+            for (i, f) in self.flags.iter_mut().enumerate().filter(|&(_, ref f)| f.disp_ord == 999) {
+                f.disp_ord = if unified { f.unified_ord } else { i };
+            }
+            for (i, sc) in &mut self.subcommands.iter_mut().enumerate().filter(|&(_, ref sc)| sc.p.meta.disp_ord == 999) {
+                sc.p.meta.disp_ord = i;
+            }
+        }
+        for sc in &mut self.subcommands {
+            sc.p.derive_display_order();
+        }
+    }
+
     pub fn required(&self) -> Iter<&str> {
         self.required.iter()
     }
diff --git a/src/args/arg_builder/flag.rs b/src/args/arg_builder/flag.rs
index 78df5c835c2..2f5c648bcde 100644
--- a/src/args/arg_builder/flag.rs
+++ b/src/args/arg_builder/flag.rs
@@ -25,6 +25,7 @@ pub struct FlagBuilder<'n, 'e> {
     pub overrides: Option<Vec<&'e str>>,
     pub settings: ArgFlags,
     pub disp_ord: usize,
+    pub unified_ord: usize,
 }
 
 impl<'n, 'e> Default for FlagBuilder<'n, 'e> {
@@ -40,6 +41,7 @@ impl<'n, 'e> Default for FlagBuilder<'n, 'e> {
             overrides: None,
             settings: ArgFlags::new(),
             disp_ord: 999,
+            unified_ord: 999,
         }
     }
 }
@@ -77,6 +79,7 @@ impl<'a, 'b, 'z> From<&'z Arg<'a, 'b>> for FlagBuilder<'a, 'b> {
             requires: a.requires.clone(),
             settings: a.settings,
             disp_ord: a.disp_ord,
+            ..Default::default()
         }
     }
 }
@@ -106,6 +109,7 @@ impl<'n, 'e> Clone for FlagBuilder<'n, 'e> {
             requires: self.requires.clone(),
             settings: self.settings,
             disp_ord: self.disp_ord,
+            unified_ord: self.unified_ord,
         }
     }
 }
diff --git a/src/args/arg_builder/option.rs b/src/args/arg_builder/option.rs
index b918dccedfa..1452172fe43 100644
--- a/src/args/arg_builder/option.rs
+++ b/src/args/arg_builder/option.rs
@@ -31,6 +31,7 @@ pub struct OptBuilder<'n, 'e> {
     pub val_delim: Option<char>,
     pub default_val: Option<&'n str>,
     pub disp_ord: usize,
+    pub unified_ord: usize,
     pub r_unless: Option<Vec<&'e str>>,
 }
 
@@ -55,6 +56,7 @@ impl<'n, 'e> Default for OptBuilder<'n, 'e> {
             val_delim: Some(','),
             default_val: None,
             disp_ord: 999,
+            unified_ord: 999,
             r_unless: None,
         }
     }
@@ -177,6 +179,7 @@ impl<'n, 'e> Clone for OptBuilder<'n, 'e> {
             requires: self.requires.clone(),
             settings: self.settings,
             disp_ord: self.disp_ord,
+            unified_ord: self.unified_ord,
             num_vals: self.num_vals,
             min_vals: self.min_vals,
             max_vals: self.max_vals,