public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH] tui: focus next button by default
@ 2023-06-21 14:48 Dominik Csapak
  2023-06-22 12:54 ` [pve-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 2+ messages in thread
From: Dominik Csapak @ 2023-06-21 14:48 UTC (permalink / raw)
  To: pve-devel

except the password dialog, since the user must provide input

to do that, we have to set the focus index on all relevant views

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---

not sure if this is the correct approach, also the extra parameter feels
slightly wrong, but didn't found a nicer way to do this

any errors from focusing will be ignrored, but that shouldn't happen
anyway until we add/remove buttons and the index changes

alternatively we could create a second 'new_with_focus_next' (or
'without') that gets called respectively, but also seems a bit weird for
that

 proxmox-tui-installer/src/main.rs | 101 +++++++++++++++---------------
 1 file changed, 52 insertions(+), 49 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 20cb31b..5eaba7f 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -63,21 +63,21 @@ impl InstallerView {
         state: &InstallerState,
         view: T,
         next_cb: Box<dyn Fn(&mut Cursive)>,
+        focus_next: bool,
     ) -> Self {
-        let inner = LinearLayout::vertical()
+        let mut bbar = LinearLayout::horizontal()
+            .child(abort_install_button())
+            .child(DummyView.full_width())
+            .child(Button::new("Previous", switch_to_prev_screen))
+            .child(DummyView)
+            .child(Button::new("Next", next_cb));
+        let _ = bbar.set_focus_index(4); // ignore errors
+        let mut inner = LinearLayout::vertical()
             .child(PaddedView::lrtb(0, 0, 1, 1, view))
-            .child(PaddedView::lrtb(
-                1,
-                1,
-                0,
-                0,
-                LinearLayout::horizontal()
-                    .child(abort_install_button())
-                    .child(DummyView.full_width())
-                    .child(Button::new("Previous", switch_to_prev_screen))
-                    .child(DummyView)
-                    .child(Button::new("Next", next_cb)),
-            ));
+            .child(PaddedView::lrtb(1, 1, 0, 0, bbar));
+        if focus_next {
+            let _ = inner.set_focus_index(1); // ignore errors
+        }
 
         Self::with_raw(state, inner)
     }
@@ -378,7 +378,15 @@ fn get_eula() -> String {
 fn license_dialog(siv: &mut Cursive) -> InstallerView {
     let state = siv.user_data::<InstallerState>().unwrap();
 
-    let inner = LinearLayout::vertical()
+    let mut bbar = LinearLayout::horizontal()
+        .child(abort_install_button())
+        .child(DummyView.full_width())
+        .child(Button::new("I agree", |siv| {
+            switch_to_next_screen(siv, InstallerStep::Bootdisk, &bootdisk_dialog)
+        }));
+    let _ = bbar.set_focus_index(2); // ignore errors
+
+    let mut inner = LinearLayout::vertical()
         .child(PaddedView::lrtb(
             0,
             0,
@@ -389,18 +397,9 @@ fn license_dialog(siv: &mut Cursive) -> InstallerView {
         .child(Panel::new(ScrollView::new(
             TextView::new(get_eula()).center(),
         )))
-        .child(PaddedView::lrtb(
-            1,
-            1,
-            1,
-            0,
-            LinearLayout::horizontal()
-                .child(abort_install_button())
-                .child(DummyView.full_width())
-                .child(Button::new("I agree", |siv| {
-                    switch_to_next_screen(siv, InstallerStep::Bootdisk, &bootdisk_dialog)
-                })),
-        ));
+        .child(PaddedView::lrtb(1, 1, 1, 0, bbar));
+
+    let _ = inner.set_focus_index(2); // ignore errors
 
     InstallerView::with_raw(state, inner)
 }
@@ -428,6 +427,7 @@ fn bootdisk_dialog(siv: &mut Cursive) -> InstallerView {
                 _ => siv.add_layer(Dialog::info("Invalid values")),
             }
         }),
+        true,
     )
 }
 
@@ -453,6 +453,7 @@ fn timezone_dialog(siv: &mut Cursive) -> InstallerView {
                 _ => siv.add_layer(Dialog::info("Invalid values")),
             }
         }),
+        true,
     )
 }
 
@@ -518,6 +519,7 @@ fn password_dialog(siv: &mut Cursive) -> InstallerView {
                 _ => siv.add_layer(Dialog::info("Invalid values")),
             }
         }),
+        false,
     )
 }
 
@@ -613,6 +615,7 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView {
                 _ => siv.add_layer(Dialog::info("Invalid values")),
             }
         }),
+        true,
     )
 }
 
@@ -643,7 +646,27 @@ impl TableViewItem for SummaryOption {
 fn summary_dialog(siv: &mut Cursive) -> InstallerView {
     let state = siv.user_data::<InstallerState>().unwrap();
 
-    let inner = LinearLayout::vertical()
+    let mut bbar = LinearLayout::horizontal()
+        .child(abort_install_button())
+        .child(DummyView.full_width())
+        .child(Button::new("Previous", switch_to_prev_screen))
+        .child(DummyView)
+        .child(Button::new("Install", |siv| {
+            let autoreboot = siv
+                .find_name("reboot-after-install")
+                .map(|v: ViewRef<Checkbox>| v.is_checked())
+                .unwrap_or_default();
+
+            siv.with_user_data(|state: &mut InstallerState| {
+                state.options.autoreboot = autoreboot;
+            });
+
+            switch_to_next_screen(siv, InstallerStep::Install, &install_progress_dialog);
+        }));
+
+    let _ = bbar.set_focus_index(4); // ignore errors
+
+    let mut inner = LinearLayout::vertical()
         .child(PaddedView::lrtb(
             0,
             0,
@@ -665,29 +688,9 @@ fn summary_dialog(siv: &mut Cursive) -> InstallerView {
                 )
                 .child(DummyView.full_width()),
         )
-        .child(PaddedView::lrtb(
-            1,
-            1,
-            1,
-            0,
-            LinearLayout::horizontal()
-                .child(abort_install_button())
-                .child(DummyView.full_width())
-                .child(Button::new("Previous", switch_to_prev_screen))
-                .child(DummyView)
-                .child(Button::new("Install", |siv| {
-                    let autoreboot = siv
-                        .find_name("reboot-after-install")
-                        .map(|v: ViewRef<Checkbox>| v.is_checked())
-                        .unwrap_or_default();
+        .child(PaddedView::lrtb(1, 1, 1, 0, bbar));
 
-                    siv.with_user_data(|state: &mut InstallerState| {
-                        state.options.autoreboot = autoreboot;
-                    });
-
-                    switch_to_next_screen(siv, InstallerStep::Install, &install_progress_dialog);
-                })),
-        ));
+    let _ = inner.set_focus_index(2); // ignore errors
 
     InstallerView::with_raw(state, inner)
 }
-- 
2.30.2





^ permalink raw reply	[flat|nested] 2+ messages in thread

* [pve-devel] applied:  [PATCH] tui: focus next button by default
  2023-06-21 14:48 [pve-devel] [PATCH] tui: focus next button by default Dominik Csapak
@ 2023-06-22 12:54 ` Thomas Lamprecht
  0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2023-06-22 12:54 UTC (permalink / raw)
  To: Proxmox VE development discussion, Dominik Csapak

Am 21/06/2023 um 16:48 schrieb Dominik Csapak:
> except the password dialog, since the user must provide input
> 
> to do that, we have to set the focus index on all relevant views
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> 
> not sure if this is the correct approach, also the extra parameter feels
> slightly wrong, but didn't found a nicer way to do this
> 
> any errors from focusing will be ignrored, but that shouldn't happen
> anyway until we add/remove buttons and the index changes
> 
> alternatively we could create a second 'new_with_focus_next' (or
> 'without') that gets called respectively, but also seems a bit weird for
> that
> 
>  proxmox-tui-installer/src/main.rs | 101 +++++++++++++++---------------
>  1 file changed, 52 insertions(+), 49 deletions(-)
> 
>

seemingly forgot to write: I applied this one, but avoided setting the focus
on the install button for the summary step, as that can be a bit unexpected,
or even dangerous, thanks!




^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2023-06-22 12:54 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-21 14:48 [pve-devel] [PATCH] tui: focus next button by default Dominik Csapak
2023-06-22 12:54 ` [pve-devel] applied: " Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal