[Mageia-dev] [soft-commits] [4252] fix removing several notebook pages

Wed Apr 25 11:35:43 CEST 2012

On Wed, Apr 25, 2012 at 09:15, Guillaume Rousse <guillomovitch at gmail.com> wrote:
+> Le 25/04/2012 09:56, Thierry Vignaud a écrit :
+>
+>> On 24 April 2012 23:17,<root at mageia.org>  wrote:
+>>>
+>>> fix removing several notebook pages
+>>
+>>
+>> (...)
+>>
+>>> --- drakx/trunk/perl-install/diskdrake/hd_gtk.pm        2012-04-24
+>>> 20:10:47 UTC
+>>> (rev 4251)
+>>> +++ drakx/trunk/perl-install/diskdrake/hd_gtk.pm        2012-04-24
+>>> 21:17:30 UTC
+>>> (rev 4252)
+>>> @@ -285,9 +285,15 @@
+>>>      $may_add->(raid2kind()) if @{$all_hds->{raids}};
+>>>      $may_add->(loopback2kind()) if @{$all_hds->{loopbacks}};
+>>>
+>>> -    @notebook = grep_index {
+>>> -       my $b = $_->{marked} or $notebook_widget->remove_page($::i);
+>>> -       $b;
+>>> +    my $i = 0;
+>>> +    @notebook = grep {
+>>> +       if ($_->{marked}) {
+>>> +           $i++;
+>>> +           1;
+>>> +       } else {
+>>> +           $notebook_widget->remove_page($i);
+>>> +           0;
+>>> +       }
+>>
+>>
+>> Wouldn't have been simpler to decrease $::i instead?
+
+Yes but I really don't like grep_index using a global variable (so not
+being reentrant) and I think it would make it even less obvious to
+read.
+I doubt anyone will add a callback on remove_page using grep_index or
+$::i but still I prefer to be safe.
+
+> I'm more concerned about the ugly mix between variable affectation (
+> @notebook = grep {} @list) and additional concerns hidden inside
+> ($notebook_widget->remove_page($i) ). And explicit loop would be easier to
+> understand, and less error-prone:
+>
+> my @notebook;
+> my $i = 0;
+> foreach my $notebook (@list) {
+>    if ($notebook->{marked}) {
+>        push @notebook, $notebook;
+>        $i++;
+>    } else {
+>        $notebook_widget->remove_page($i);
+>    }
+> }
+>
+
+In this case, @list is @notebook so you would need another list (we
+are filtering @notebook but removing matching element from
+$notebook_widget when removing one from @notebook)
+