[pve-devel] [PATCH installer 1/2] implement previous button
Thomas Lamprecht
t.lamprecht at proxmox.com
Mon Dec 10 13:27:49 CET 2018
On 12/10/18 12:36 PM, Oguz Bektas wrote:
> 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
>
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
> 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)
> +
> +my @function_list = (
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 $mailto = 'mail at example.invalid';
> 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
> +
> + my ($text, $fctn) = @_;
> +
> + $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 {
> my ($text, $fctn) = @_;
>
> $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('mail at example.invalid');
> + $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); };
> my $err = $@;
>
> - $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);
> }
>
>
More information about the pve-devel
mailing list