[pve-devel] [PATCH v2 installer] implemented acknowledgement screen

[Thomas Lamprecht]

not bad in general! a (short) commit message about what you do and, more important,
why you do it, would be always great here. doesn't has to be much.

And while some things like HD settings do get remembered when going back, most don't,
this needs fixing. Maybe with some global config hash to save the value and setting it
on view (re)create?

On 12/3/18 12:06 PM, Oguz Bektas wrote:> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
>  html-common/ipconf.htm |   4 +-
>  proxinstall            | 116 +++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 115 insertions(+), 5 deletions(-)
> 
> diff --git a/html-common/ipconf.htm b/html-common/ipconf.htm
> index ba699ed..2aefa90 100644
> --- a/html-common/ipconf.htm
> +++ b/html-common/ipconf.htm
> @@ -19,8 +19,8 @@
>  	the displayed network configuration.  You will need a valid network
>  	configuration to access the management interface after installation.
>  	<br><br>
> -	Afterwards press the <b>Install</b> button to start the installation. The
> -	installer will then partition your hard disk and start copying packages.
> +	Afterwards press the Next button. You will be displayed a list of the options
> +	that you chose during the previous steps.
>        </td></tr>
>      </tbody></table>
>    </p></td>
> diff --git a/proxinstall b/proxinstall
> index 9945dd1..0631490 100755
> --- a/proxinstall
> +++ b/proxinstall
> @@ -14,6 +14,7 @@ use IO::Select;
>  use Cwd 'abs_path';
>  use Gtk3 '-init';
>  use Gtk3::WebKit;
> +use Gtk3::SimpleList;
>  use Encode;
>  use String::ShellQuote;
>  use Data::Dumper;
> @@ -190,8 +191,23 @@ my $ipv4_reverse_mask = [
>      '255.255.255.255',
>  ];
>  
> +my $stack_number = 0; # init number for function stack
> +
> +my %function_stack = (
> +
> +    # Description: Hash for global function stack
> +
> +    1 => \&create_intro_view,
> +    2 => \&create_hdsel_view,
> +    3 => \&create_country_view,
> +    4 => \&create_password_view,
> +    5 => \&create_ipconf_view,
> +    6 => \&create_ack_view,
Hmm, you're effectively using the hash as an array, so either just use
an array or expand this into a real hash use, e.g., something like:

my $steps = {
    intro => {
        html => 'license.htm',
        button => 'I a_gree', # could default to 'N_ext'
        nextstep => 'hdsel',
        code => \coderef,
    },
    hdsel => {
        html => 'page1.htm',
        next => 'country',
        code => \coderef,
    },
    ...
};

Then one could to a a bit general step advancing which remembers the previous
view and does so common things like cleanup_view or display_html (if html
property is defined). Just as an idea.

> +);
> +
>  my ($window, $cmdbox, $inbox, $htmlview);
>  my ($next, $next_fctn, $target_hd);
> +my $prev;
>  my ($progress, $progress_status);
>  my ($ipversion, $ipaddress, $ipconf_entry_addr);
>  my ($netmask, $ipconf_entry_mask);
> @@ -1682,6 +1698,20 @@ sub display_html {
>      $last_display_change = time();
>  }
>  
> +sub prev_fctn {
> +
> +    my ($text, $fctn) = @_;
could we please rename fctn to something more sensible, function isn't much
longer... 'code' or 'screen' could be alternatives..

> +
> +    $stack_number--;
> +    $fctn = $stack_number if !$fctn;
> +    $text = "_Previous" if !$text;
> +    $prev->set_label ($text);
> +
> +    $function_stack{lc $stack_number}();
why lc (lower case) here?

> +
> +    $prev->grab_focus ();
> +}
> +
>  sub set_next {
>      my ($text, $fctn) = @_;
>  
> @@ -1721,6 +1751,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_fctn (); });
> +    $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);
> @@ -1899,6 +1936,8 @@ my $ipconf_first_view = 1;
>  
>  sub create_ipconf_view {
>  
> +    $stack_number = 5;
> +
hmm, do not like the hardcoded values to much here, does
$stack_number++ not works?

>      cleanup_view ();
>      display_html ("ipconf.htm");
>  
> @@ -1991,7 +2030,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,12 +2116,73 @@ sub create_ipconf_view {
>  
>  	#print "TEST $ipaddress $netmask $gateway $dnsserver\n";
>  
> -	create_extract_view ();
> +	create_ack_view ();
>      });
>  
>      $hostentry->grab_focus();
>  }
>  
> +sub create_ack_view {
> +
> +    # Description: Function for showing the user a list of
> +    # the previously chosen options during install
> +
> +    $stack_number = 6;
> +
> +    cleanup_view ();
> +
> +    display_info ();
> +
> +    #display_html ('install.htm'); # TODO: show fitting html file
> +
> +    my $grid =  Gtk3::Grid->new();
> +    $grid->set_visible(1);
> +    $grid->set_column_spacing(10);
> +    $grid->set_row_spacing(10);
> +    $grid->set_hexpand(1);
> +
> +    $grid->set_margin_start(7);
> +    $grid->set_margin_end(7);
> +    $grid->set_margin_top(7);
> +    $grid->set_margin_bottom(7);
> +
> +
> +    # acknowledge the following configs:
> +    #	a. target disk
> +    #	b. network configuration
> +    #	c. timezone/keyboard layout
> +    #	d. email
> +
> +    my $ack_list = Gtk3::SimpleList->new (
> +	'Option' => 'text',
> +	'Value' => 'text',
> +    );
Above would need to add libgtk3-simplelist-perl to the build dependency list in
debian/control

Maybe just use HTML with a template (libtemplate-perl) and show the values there
in a nice full width table with a 'Summary' heading?

> +
> +    @{$ack_list->{data}} = (
> +	[ 'disk', $target_hd ],
> +	[ 'country' ,$country ],
> +	[ 'timezone' ,$timezone ],
> +	[ 'keymap' ,$keymap ],
> +	[ 'mailto' ,$mailto ],
> +	[ 'interface', $ipconf->{ifaces}->{$ipconf->{selected}}->{name} ],
> +	[ 'hostname' ,$hostname ],
> +	[ 'ip' ,$ipaddress ],
> +	[ 'netmask' ,$netmask ],
> +	[ 'gateway' ,$gateway ],
> +	[ 'dnsserver' ,$dnsserver ],
> +    );
> +
> +    $grid->add($ack_list);
> +
> +    $inbox->pack_start($grid, 1, 0, 0);
> +    $inbox->show_all ();
> +
> +    set_next ('_Install', sub {
> +	create_extract_view ();
> +    });
> +
> +}
> +
>  sub get_device_desc {
>      my ($devname, $size, $model) = @_;
>  
> @@ -2165,6 +2265,8 @@ sub update_zonelist {
>  
>  sub create_password_view {
>  
> +    $stack_number = 4;
> +
>      cleanup_view ();
>  
>      my $vbox2 =  Gtk3::VBox->new (0, 0);
> @@ -2254,6 +2356,8 @@ sub create_password_view {
>  
>  sub create_country_view {
>  
> +    $stack_number = 3;
> +
>      cleanup_view ();
>  
>      my $countryhash = $cmap->{countryhash};
> @@ -2886,6 +2990,9 @@ sub get_btrfs_raid_setup {
>  
>  sub create_hdsel_view {
>  
> +    $stack_number = 2;
> +    $prev->set_sensitive (1); # enable previous button at this point
> +
>      cleanup_view ();
>  
>      my $vbox =  Gtk3::VBox->new (0, 0);
> @@ -2973,7 +3080,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;
> @@ -2997,6 +3104,9 @@ sub create_extract_view {
>  
>  sub create_intro_view {
>  
> +    $stack_number = 1;
> +    $prev->set_sensitive (0);
please no space between method name and parameters.

> +
>      cleanup_view ();
>  
>      if ($setup->{product} eq 'pve') {
>