{"thread":{"id":"29310","subject":"[PATCH] git-gui: fix selection regression introduced in a8ca786991","startedAt":"2012-01-07T19:43:47Z","lastAt":"2012-01-14T11:58:47Z","messageCount":3,"participants":["Bert Wesarg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"182076","messageId":"14628854a651ab0202e3f82be9b245331cf9029a.1325965254.git.bert.wesarg@googlemail.com","threadId":"29310","inReplyTo":null,"subject":"[PATCH] git-gui: fix selection regression introduced in a8ca786991","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2012-01-07T19:43:47Z","receivedAt":"2012-01-07T19:43:47Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"While fixing the problem from a8ca786991, it introduces a regression\nregarding what happen after the multi selected file operation (ie.\none of Ctrl-{T,U,J}) because the next selected file could not be handled\nby such a subsequent file operation.\n\nThe right way is to move the fix from this commit down into the show_diff\nfunction. So that all code path add the current diff path to the list of\nselections.\n\nThis also simplifies helper functions for these operatione which needed\nto handle the case whether there is only the current diff path or also\na selction.\n\nSigned-off-by: Bert Wesarg <bert.wesarg@googlemail.com>\n---\n\nI propbose this to be inlcuded in the next git-1.7.9 release.\n\n git-gui.sh    |    1 -\n lib/diff.tcl  |    3 ++-\n lib/index.tcl |   48 +++++++++++++++---------------------------------\n 3 files changed, 17 insertions(+), 35 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex ba4e5c1..13b22dd 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -2520,7 +2520,6 @@ proc toggle_or_diff {w x y} {\n \t\t\t\t[concat $after [list ui_ready]]\n \t\t}\n \t} else {\n-\t\tset selected_paths($path) 1\n \t\tshow_diff $path $w $lno\n \t}\n }\ndiff --git a/lib/diff.tcl b/lib/diff.tcl\nindex ec44055..775649c 100644\n--- a/lib/diff.tcl\n+++ b/lib/diff.tcl\n@@ -75,7 +75,7 @@ A rescan will be automatically started to find other files which may have the sa\n }\n \n proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {\n-\tglobal file_states file_lists\n+\tglobal file_states file_lists selected_paths\n \tglobal is_3way_diff is_conflict_diff diff_active repo_config\n \tglobal ui_diff ui_index ui_workdir\n \tglobal current_diff_path current_diff_side current_diff_header\n@@ -91,6 +91,7 @@ proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {\n \t\t}\n \t}\n \tif {$lno >= 1} {\n+\t\tset selected_paths($path) 1\n \t\t$w tag add in_diff $lno.0 [expr {$lno + 1}].0\n \t\t$w see $lno.0\n \t}\ndiff --git a/lib/index.tcl b/lib/index.tcl\nindex 8efbbdd..2223a21 100644\n--- a/lib/index.tcl\n+++ b/lib/index.tcl\n@@ -287,17 +287,11 @@ proc unstage_helper {txt paths} {\n }\n \n proc do_unstage_selection {} {\n-\tglobal current_diff_path selected_paths\n-\n-\tif {[array size selected_paths] > 0} {\n-\t\tunstage_helper \\\n-\t\t\t{Unstaging selected files from commit} \\\n-\t\t\t[array names selected_paths]\n-\t} elseif {$current_diff_path ne {}} {\n-\t\tunstage_helper \\\n-\t\t\t[mc \"Unstaging %s from commit\" [short_path $current_diff_path]] \\\n-\t\t\t[list $current_diff_path]\n-\t}\n+\tglobal selected_paths\n+\n+\tunstage_helper \\\n+\t\t{Unstaging selected files from commit} \\\n+\t\t[array names selected_paths]\n }\n \n proc add_helper {txt paths} {\n@@ -339,17 +333,11 @@ proc add_helper {txt paths} {\n }\n \n proc do_add_selection {} {\n-\tglobal current_diff_path selected_paths\n-\n-\tif {[array size selected_paths] > 0} {\n-\t\tadd_helper \\\n-\t\t\t{Adding selected files} \\\n-\t\t\t[array names selected_paths]\n-\t} elseif {$current_diff_path ne {}} {\n-\t\tadd_helper \\\n-\t\t\t[mc \"Adding %s\" [short_path $current_diff_path]] \\\n-\t\t\t[list $current_diff_path]\n-\t}\n+\tglobal selected_paths\n+\n+\tadd_helper \\\n+\t\t{Adding selected files} \\\n+\t\t[array names selected_paths]\n }\n \n proc do_add_all {} {\n@@ -452,17 +440,11 @@ proc revert_helper {txt paths} {\n }\n \n proc do_revert_selection {} {\n-\tglobal current_diff_path selected_paths\n-\n-\tif {[array size selected_paths] > 0} {\n-\t\trevert_helper \\\n-\t\t\t[mc \"Reverting selected files\"] \\\n-\t\t\t[array names selected_paths]\n-\t} elseif {$current_diff_path ne {}} {\n-\t\trevert_helper \\\n-\t\t\t[mc \"Reverting %s\" [short_path $current_diff_path]] \\\n-\t\t\t[list $current_diff_path]\n-\t}\n+\tglobal selected_paths\n+\n+\trevert_helper \\\n+\t\t[mc \"Reverting selected files\"] \\\n+\t\t[array names selected_paths]\n }\n \n proc do_select_commit_type {} {\n-- \n1.7.8.1.873.gfea665\n"},{"id":"182156","messageId":"CAKPyHN0xRD7qYyLYaCB9u7mhCZYFObuTdJGHq-rRST-cEhTtXA@mail.gmail.com","threadId":"29310","inReplyTo":"14628854a651ab0202e3f82be9b245331cf9029a.1325965254.git.bert.wesarg@googlemail.com","subject":"Re: [PATCH] git-gui: fix selection regression introduced in a8ca786991","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2012-01-09T09:28:19Z","receivedAt":"2012-01-09T09:28:19Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"Hi,\n\nOn Sat, Jan 7, 2012 at 20:43, Bert Wesarg <bert.wesarg@googlemail.com> wrote:\n> While fixing the problem from a8ca786991, it introduces a regression\n> regarding what happen after the multi selected file operation (ie.\n> one of Ctrl-{T,U,J}) because the next selected file could not be handled\n> by such a subsequent file operation.\n>\n> The right way is to move the fix from this commit down into the show_diff\n> function. So that all code path add the current diff path to the list of\n> selections.\n>\n> This also simplifies helper functions for these operatione which needed\n> to handle the case whether there is only the current diff path or also\n> a selction.\n\nI think we need to think this more through, especially with input from\nShawn, please.\n\nI have now find out, that git-gui has two selections in the file\nlists. The first is that for the current path for what we show the\ndiff (the tag for this is called 'in_diff') and the the second is that\nfor the current list of paths which are selected ('in_sel'). The file\nlist operations 'staging', 'reverting', 'unstaging', work either on\n'in_sel'; if that is not empty, or on 'in_diff'. The problem I've now\nrealized is, that these two selections share the same visual hints,\nie. a lightgray background.\n\nThe problem I tried to solve in a8ca786991 was, that adding paths to\nthe selection with Ctrl-Button-1 or Shift-cutton-1, didn't included\nthe current diff path in the subsequent file list operation. But I\nwould have expected it, because it was visual in the 'selection'.\n\nMy current 'workaround' is to make the two selections visually\ndistinguishable (and reverting a8ca786991), by using a different\nbackground color for the 'in_sel' tag and also the italic font, so\nthat it is still possible to see whether the current diff path is in\nthe selection or not:\n\n@@ -717,11 +717,11 @@ proc tk_optionMenu {w varName args} {\n proc rmsel_tag {text} {\n \t$text tag conf sel \\\n \t\t-background [$text cget -background] \\\n \t\t-foreground [$text cget -foreground] \\\n \t\t-borderwidth 0\n-\t$text tag conf in_sel -background lightgray\n+\t$text tag conf in_sel -background SlateGray1 -font font_diffitalic\n \tbind $text <Motion> break\n \treturn $text\n }\n\n wm withdraw .\n@@ -3557,11 +3557,11 @@ if {$use_ttk} {\n \t.vpane.files add .vpane.files.index -sticky news\n }\n\n foreach i [list $ui_index $ui_workdir] {\n \trmsel_tag $i\n-\t$i tag conf in_diff -background [$i tag cget in_sel -background]\n+\t$i tag conf in_diff -background lightgray\n }\n unset i\n\n set files_ctxm .vpane.files.ctxm\n menu $files_ctxm -tearoff 0\n\nI'm not very pleased with this, but at least it is now possible to\nvisual recognize what files will be handled by a subsequent file list\noperation.\n\nAny input is more than welcome.\n\nRegards,\nBert\n"},{"id":"182545","messageId":"CAKPyHN31MV7PcSNP+ovdXx2H7Lbu=hxMN_pPOfi1TZW1qnYRvQ@mail.gmail.com","threadId":"29310","inReplyTo":"CAKPyHN0xRD7qYyLYaCB9u7mhCZYFObuTdJGHq-rRST-cEhTtXA@mail.gmail.com","subject":"Re: [PATCH] git-gui: fix selection regression introduced in a8ca786991","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2012-01-14T11:58:47Z","receivedAt":"2012-01-14T11:58:47Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"Hi Pat,\n\nOn Mon, Jan 9, 2012 at 10:28, Bert Wesarg <bert.wesarg@googlemail.com> wrote:\n> Hi,\n>\n> On Sat, Jan 7, 2012 at 20:43, Bert Wesarg <bert.wesarg@googlemail.com> wrote:\n>> While fixing the problem from a8ca786991, it introduces a regression\n>> regarding what happen after the multi selected file operation (ie.\n>> one of Ctrl-{T,U,J}) because the next selected file could not be handled\n>> by such a subsequent file operation.\n>>\n>> The right way is to move the fix from this commit down into the show_diff\n>> function. So that all code path add the current diff path to the list of\n>> selections.\n>>\n>> This also simplifies helper functions for these operatione which needed\n>> to handle the case whether there is only the current diff path or also\n>> a selction.\n>\n> I think we need to think this more through, especially with input from\n> Shawn, please.\n>\n> I have now find out, that git-gui has two selections in the file\n> lists. The first is that for the current path for what we show the\n> diff (the tag for this is called 'in_diff') and the the second is that\n> for the current list of paths which are selected ('in_sel'). The file\n> list operations 'staging', 'reverting', 'unstaging', work either on\n> 'in_sel'; if that is not empty, or on 'in_diff'. The problem I've now\n> realized is, that these two selections share the same visual hints,\n> ie. a lightgray background.\n>\n> The problem I tried to solve in a8ca786991 was, that adding paths to\n> the selection with Ctrl-Button-1 or Shift-cutton-1, didn't included\n> the current diff path in the subsequent file list operation. But I\n> would have expected it, because it was visual in the 'selection'.\n>\n> My current 'workaround' is to make the two selections visually\n> distinguishable (and reverting a8ca786991), by using a different\n> background color for the 'in_sel' tag and also the italic font, so\n> that it is still possible to see whether the current diff path is in\n> the selection or not:\n>\n> @@ -717,11 +717,11 @@ proc tk_optionMenu {w varName args} {\n>  proc rmsel_tag {text} {\n>        $text tag conf sel \\\n>                -background [$text cget -background] \\\n>                -foreground [$text cget -foreground] \\\n>                -borderwidth 0\n> -       $text tag conf in_sel -background lightgray\n> +       $text tag conf in_sel -background SlateGray1 -font font_diffitalic\n>        bind $text <Motion> break\n>        return $text\n>  }\n>\n>  wm withdraw .\n> @@ -3557,11 +3557,11 @@ if {$use_ttk} {\n>        .vpane.files add .vpane.files.index -sticky news\n>  }\n>\n>  foreach i [list $ui_index $ui_workdir] {\n>        rmsel_tag $i\n> -       $i tag conf in_diff -background [$i tag cget in_sel -background]\n> +       $i tag conf in_diff -background lightgray\n>  }\n>  unset i\n>\n>  set files_ctxm .vpane.files.ctxm\n>  menu $files_ctxm -tearoff 0\n>\n> I'm not very pleased with this, but at least it is now possible to\n> visual recognize what files will be handled by a subsequent file list\n> operation.\n>\n> Any input is more than welcome.\n>\n\nI think we don't resolve this now, because a8ca786991 introduced a\nregression introduced in 0.16, I propose to revert it for upcoming\n1.7.9 release.\n\nThanks.\n\nBert\n\n> Regards,\n> Bert\n"}]}