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

[Pascal Terjan]

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)