From: Bert Wesarg <bert.wesarg@googlemail.com>
Date: Sat, 14 Jan 2012 11:58:47 GMT
Subject: Re: [PATCH] git-gui: fix selection regression introduced in a8ca786991
Message-ID: <CAKPyHN31MV7PcSNP+ovdXx2H7Lbu=hxMN_pPOfi1TZW1qnYRvQ@mail.gmail.com>
In-Reply-To: <CAKPyHN0xRD7qYyLYaCB9u7mhCZYFObuTdJGHq-rRST-cEhTtXA@mail.gmail.com>

Hi Pat,

On Mon, Jan 9, 2012 at 10:28, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> Hi,
>
> On Sat, Jan 7, 2012 at 20:43, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> While fixing the problem from a8ca786991, it introduces a regression
>> regarding what happen after the multi selected file operation (ie.
>> one of Ctrl-{T,U,J}) because the next selected file could not be handled
>> by such a subsequent file operation.
>>
>> The right way is to move the fix from this commit down into the show_diff
>> function. So that all code path add the current diff path to the list of
>> selections.
>>
>> This also simplifies helper functions for these operatione which needed
>> to handle the case whether there is only the current diff path or also
>> a selction.
>
> I think we need to think this more through, especially with input from
> Shawn, please.
>
> I have now find out, that git-gui has two selections in the file
> lists. The first is that for the current path for what we show the
> diff (the tag for this is called 'in_diff') and the the second is that
> for the current list of paths which are selected ('in_sel'). The file
> list operations 'staging', 'reverting', 'unstaging', work either on
> 'in_sel'; if that is not empty, or on 'in_diff'. The problem I've now
> realized is, that these two selections share the same visual hints,
> ie. a lightgray background.
>
> The problem I tried to solve in a8ca786991 was, that adding paths to
> the selection with Ctrl-Button-1 or Shift-cutton-1, didn't included
> the current diff path in the subsequent file list operation. But I
> would have expected it, because it was visual in the 'selection'.
>
> My current 'workaround' is to make the two selections visually
> distinguishable (and reverting a8ca786991), by using a different
> background color for the 'in_sel' tag and also the italic font, so
> that it is still possible to see whether the current diff path is in
> the selection or not:
>
> @@ -717,11 +717,11 @@ proc tk_optionMenu {w varName args} {
>  proc rmsel_tag {text} {
>        $text tag conf sel \
>                -background [$text cget -background] \
>                -foreground [$text cget -foreground] \
>                -borderwidth 0
> -       $text tag conf in_sel -background lightgray
> +       $text tag conf in_sel -background SlateGray1 -font font_diffitalic
>        bind $text <Motion> break
>        return $text
>  }
>
>  wm withdraw .
> @@ -3557,11 +3557,11 @@ if {$use_ttk} {
>        .vpane.files add .vpane.files.index -sticky news
>  }
>
>  foreach i [list $ui_index $ui_workdir] {
>        rmsel_tag $i
> -       $i tag conf in_diff -background [$i tag cget in_sel -background]
> +       $i tag conf in_diff -background lightgray
>  }
>  unset i
>
>  set files_ctxm .vpane.files.ctxm
>  menu $files_ctxm -tearoff 0
>
> I'm not very pleased with this, but at least it is now possible to
> visual recognize what files will be handled by a subsequent file list
> operation.
>
> Any input is more than welcome.
>

I think we don't resolve this now, because a8ca786991 introduced a
regression introduced in 0.16, I propose to revert it for upcoming
1.7.9 release.

Thanks.

Bert

> Regards,
> Bert