Discussion:
[pve-devel] [PATCH installer 2/2] implemented acknowledgement screen
Thomas Lamprecht
2018-12-10 13:04:24 UTC
Permalink
this implements an acknowledgement screen as the last
install step, which shows the user what they chose
during the installation.
the html files have been modified according to the
new installation steps as well.
looks OK in general, I'd propose the following to make the formatting a bit
nicer and fitting to the rest of the installer/Proxmox VE theme:

----8<----
Author: Thomas Lamprecht <***@proxmox.com>
Date: Mon Dec 10 12:50:06 2018 +0100

improve summary screen styling

use a more Proxmox VE like styling, differ background color of
alternating rows slightly, fix missing tbody opening tag and try to
match table column width a bit better.

Signed-off-by: Thomas Lamprecht <***@proxmox.com>

diff --git a/html-common/ack_template.htm b/html-common/ack_template.htm
index af7f20a..6236ed1 100644
--- a/html-common/ack_template.htm
+++ b/html-common/ack_template.htm
@@ -2,11 +2,25 @@
<html>
<head>
<link rel="stylesheet" type="text/css" href="pve-installer.css">
+<style type="text/css">
+ #summary td, tr {
+ padding: 3px;
+ }
+ #summary tr:nth-child(even){ background-color: #f2f2f2; }
+ #summary tr:hover { background-color: #ddd; }
+ #summary th {
+ padding-top: 5px;
+ padding-bottom: 5px;
+ padding-left: 5px;
+ text-align: left;
+ background-color: #e57000;
+ color: white;
+ }
+</style>
</head>
<body>
<center>
-<table border="0" width="800">
-<tr><td>&nbsp;</td></tr>
+<table border="0" width="800" style="margin-top: 10pt;">
<tr>
<td colspan="4" align="center" width="800">
<b class="htext">Summary</b>
@@ -26,10 +40,10 @@
</tbody></table>
</tr>

-<table border="0" width="800">
-
+<table id="summary" border="0" width="800">
+<tbody>
<tr>
- <th>Option</th>
+ <th width="25%">Option</th>
<th>Value</th>
</tr>

@@ -37,7 +51,6 @@
<td>Disk:</td>
<td>__target_hd__</td>
</tr>
-
<tr>
<td>Country:</td>
<td>__country__</td>
Thomas Lamprecht
2018-12-10 12:27:49 UTC
Permalink
this implements a previous button to the installer,
along with some structural changes to allow the
installer to be more modular in the future
(such as a global list for functions and a
hash to hold the given config options)
your textwidth seems really off... I've the following line in my .vimrc
to ensure I to not need to think about it :)
au FileType gitcommit setlocal tw=69
---
proxinstall | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 127 insertions(+), 14 deletions(-)
diff --git a/proxinstall b/proxinstall
index 159c727..893f292 100755
--- a/proxinstall
+++ b/proxinstall
@@ -190,9 +190,59 @@ my $ipv4_reverse_mask = [
'255.255.255.255',
];
+my $stack_number = 0; # Init number for global function list
I'd call it $step_number (see below)
+
can we please call it "step_order
+
+ # Description: Global array list for functions
those types of comments seems just like noise, i.e., they to not add
additional information. I see that this is an array and that it's named
function_list from reading the code already..

"Why" comments are generally better that "what" comments, one should be
able to determine the "what" from reading the code, else something is
probably off, at least normally and assuming the reader has some basic
coding/perl/pve understanding :)
+
+ 'intro',
+ 'hdsel',
+ 'country',
+ 'password',
+ 'ipconf',
+ 'ack',
added ack already here, while you add the ack view only in patch 2/2,
this introduces a temporary breakage as 'ack' has no entry in
$function_options below...
+ 'extract',
+);
+
+my $function_options = {
hmm, we could obsolete above function_list, maybe use an array here, and move
the step name inside the hash?

my $steps = [
{
step => 'intro',
html => 'license.htm',
button => 'I a_gree',
function => \&create_intro_view,
},
{
step => 'hdsel',
....
},
....
];

Then you would get the order and definition in one single array, not needing both
an array to ensure the order and this hash?
+
+ # Description: Custom options for functions
same as above, comment does not tells more than one can read from the code
already..
+
+ intro => {
+ html => 'license.htm',
+ button => 'I a_gree',
+ function => \&create_intro_view,
+ },
+ hdsel => {
+ html => 'page1.htm',
+ function => \&create_hdsel_view,
+ },
+ country => {
+ html => 'country.htm',
+ function => \&create_country_view,
+ },
+ password => {
+ html => 'passwd.htm',
+ function => \&create_password_view,
+ },
+ ipconf => {
+ next_button => '_Install',
+ html => 'ipconf.htm',
+ function => \&create_ipconf_view,
+ },
+ extract => {
+ next_button => '_Reboot',
+ function => \&create_extract_view,
+ },
+};
+
+# GUI global variables
my ($window, $cmdbox, $inbox, $htmlview);
+my $prev;
my ($next, $next_fctn, $target_hd);
my ($progress, $progress_status);
+
my ($ipversion, $ipaddress, $ipconf_entry_addr);
my ($netmask, $ipconf_entry_mask);
my ($gateway, $ipconf_entry_gw);
@@ -203,11 +253,37 @@ my $cmdline = file_read_firstline("/proc/cmdline");
my $ipconf;
my $country;
my $timezone = 'Europe/Vienna';
-my $password;
-my $mailto;
my $keymap = 'en-us';
+my $password;
my $cmap;
+my $global_configuration = {
+
+ # Description: Hash to hold global configuration
+ # options for future use
same as above, not needed, just more to read..
+
+ # Format: screen/function name => settings for that screen
this is actually OK, but I'd move it directly above the hash definition.
+
+ # TODO: add all the user-provided options during the install
+ # to be able to call them back if necessary
+
+ hdsel => {},
+ country => {
+ country => $country,
+ timezone => $timezone,
+ keymap => $keymap,
+ },
+ password => {
+ password => $password,
+ mailto => $mailto,
+ },
+ ipconf => {
+ hostname => $hostname,
+ domain => $domain,
+ },
+};
+
# parse command line args
my $config_options = {};
@@ -1682,11 +1758,28 @@ sub display_html {
$last_display_change = time();
}
+sub prev_function {
+
+ # Description: Calls the last function
+
+
+ $fctn = $stack_number if !$fctn;
+ $text = "_Previous" if !$text;
+ $prev->set_label ($text);
+
+ $stack_number--;
+ $function_options->{$function_list[$stack_number]}->{function}();
+
+ $prev->grab_focus ();
+}
+
sub set_next {
$next_fctn = $fctn;
- $text = "_Next" if !$text;
+ my $step = $function_list[$stack_number];
+ $text //= $function_options->{$step}->{next_button} // '_Next';
$next->set_label ($text);
$next->grab_focus ();
@@ -1721,6 +1814,13 @@ sub create_main_window {
$next = Gtk3::Button->new ('_Next');
$next->signal_connect (clicked => sub { $last_display_change = 0; &$next_fctn (); });
$cmdbox->pack_end ($next, 0, 0, 10);
+
+
+ $prev = Gtk3::Button->new ('_Previous');
+ $prev->signal_connect (clicked => sub { $last_display_change = 0; &prev_function (); });
+ $cmdbox->pack_end ($prev, 0, 0, 10);
+
+
my $abort = Gtk3::Button->new ('_Abort');
$abort->set_can_focus (0);
$cmdbox->pack_start ($abort, 0, 0, 10);
@@ -1900,7 +2000,7 @@ my $ipconf_first_view = 1;
sub create_ipconf_view {
cleanup_view ();
- display_html ("ipconf.htm");
+ display_html ($function_options->{ipconf}->{html});
my $vbox = Gtk3::VBox->new (0, 0);
$inbox->pack_start ($vbox, 1, 0, 0);
@@ -1991,7 +2091,7 @@ sub create_ipconf_view {
$vbox2->pack_start ($dnsbox, 0, 0, 0);
$inbox->show_all;
- set_next ('_Install', sub {
+ set_next (undef, sub {
# verify hostname
@@ -2077,7 +2177,8 @@ sub create_ipconf_view {
#print "TEST $ipaddress $netmask $gateway $dnsserver\n";
- create_extract_view ();
+ $stack_number++;
+ create_extract_view();
});
$hostentry->grab_focus();
@@ -2179,6 +2280,7 @@ sub create_password_view {
$hbox1->pack_start ($label, 0, 0, 10);
my $pwe1 = Gtk3::Entry->new ();
$pwe1->set_visibility (0);
+ $pwe1->set_text($password) if $password;
$pwe1->set_size_request (200, -1);
$hbox1->pack_start ($pwe1, 0, 0, 0);
@@ -2189,6 +2291,7 @@ sub create_password_view {
$hbox2->pack_start ($label, 0, 0, 10);
my $pwe2 = Gtk3::Entry->new ();
$pwe2->set_visibility (0);
+ $pwe2->set_text($password) if $password;
$pwe2->set_size_request (200, -1);
$hbox2->pack_start ($pwe2, 0, 0, 0);
@@ -2199,7 +2302,7 @@ sub create_password_view {
$hbox3->pack_start ($label, 0, 0, 10);
my $eme = Gtk3::Entry->new ();
$eme->set_size_request (200, -1);
+ $eme->set_text($mailto);
$hbox3->pack_start ($eme, 0, 0, 0);
@@ -2209,7 +2312,7 @@ sub create_password_view {
$inbox->show_all;
- display_html ("passwd.htm");
+ display_html ($function_options->{password}->{html});
set_next (undef, sub {
@@ -2245,6 +2348,7 @@ sub create_password_view {
$password = $t1;
$mailto = $t3;
+ $stack_number++;
create_ipconf_view();
});
@@ -2389,13 +2493,14 @@ sub create_country_view {
$inbox->show_all;
- display_html ("country.htm");
+ display_html ($function_options->{country}->{html});
set_next (undef, sub {
my $text = $w->get_text;
if (my $cc = $countryhash->{lc($text)}) {
$country = $cc;
+ $stack_number++;
create_password_view();
return;
} else {
@@ -2888,6 +2993,8 @@ sub get_btrfs_raid_setup {
sub create_hdsel_view {
+ $prev->set_sensitive(1); # enable previous button at this point
+
cleanup_view ();
my $vbox = Gtk3::VBox->new (0, 0);
@@ -2924,7 +3031,7 @@ sub create_hdsel_view {
$inbox->show_all;
- display_html ("page1.htm");
+ display_html($function_options->{hdsel}->{html});
set_next (undef, sub {
@@ -2934,6 +3041,7 @@ sub create_hdsel_view {
display_message ("Warning: $err\n" .
"Please fix ZFS setup first.");
} else {
+ $stack_number++;
create_country_view();
}
} elsif ($config_options->{filesys} =~ m/btrfs/) {
@@ -2942,9 +3050,11 @@ sub create_hdsel_view {
display_message ("Warning: $err\n" .
"Please fix BTRFS setup first.");
} else {
+ $stack_number++;
create_country_view();
}
} else {
+ $stack_number++;
create_country_view();
}
});
@@ -2956,7 +3066,7 @@ sub create_extract_view {
display_info();
- $next->set_sensitive (0);
+ $next->set_sensitive(0);
my $vbox = Gtk3::VBox->new (0, 0);
$inbox->pack_start ($vbox, 1, 0, 0);
@@ -2975,7 +3085,7 @@ sub create_extract_view {
$vbox2->pack_start ($progress, 0, 0, 0);
- $inbox->show_all;
+ $inbox->show_all();
my $tdir = $opt_testmode ? "target" : "/target";
mkdir $tdir;
@@ -2984,7 +3094,7 @@ sub create_extract_view {
eval { extract_data ($base, $tdir); };
- $next->set_sensitive (1);
+ $next->set_sensitive(1);
set_next ("_Reboot", sub { exit (0); } );
@@ -2999,6 +3109,8 @@ sub create_extract_view {
sub create_intro_view {
+ $prev->set_sensitive(0);
+
cleanup_view ();
if ($setup->{product} eq 'pve') {
@@ -3011,8 +3123,9 @@ sub create_intro_view {
};
}
- display_html ("license.htm");
+ display_html ($function_options->{intro}->{html});
+ $stack_number++;
set_next ("I a_gree", \&create_hdsel_view);
}
Loading...