git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v6 2/3] git-gui: update status bar to track operations

From
Pratyush Yadav <me@yadavpratyush.com>
Date
Nov 30, 2019, 23:05 UTC
Message-ID
<20191130230543.p5xtapnx5a56arng@yadavpratyush.com>
In-Reply-To
<ab3d8e54c3d3d5174fe222ee77101ab3b8e9cab8.1574929833.git.gitgitgadget@gmail.com>
Hi Jonathan,
Thanks for the re-roll.
On 28/11/19 08:30AM, Jonathan Gilbert via GitGitGadget wrote:
Show 30 quoted lines
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
> 
> Update the status bar to track updates as individual "operations" that
> can overlap. Update all call sites to interact with the new status bar
> mechanism. Update initialization to explicitly clear status text,
> since otherwise it may persist across future operations.
> 
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
>  git-gui.sh                |  31 +++---
>  lib/blame.tcl             |  24 ++--
>  lib/checkout_op.tcl       |  15 +--
>  lib/choose_repository.tcl | 120 +++++++++++++-------
>  lib/index.tcl             |  31 ++++--
>  lib/merge.tcl             |  14 ++-
>  lib/status_bar.tcl        | 229 +++++++++++++++++++++++++++++++++-----
>  7 files changed, 354 insertions(+), 110 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 0d21f5688b..6dcf6551b6 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}]
>  ##
>  ## Tcl/Tk sanity check
>  
> -if {[catch {package require Tcl 8.4} err]
> - || [catch {package require Tk  8.4} err]
> +if {[catch {package require Tcl 8.6} err]
> + || [catch {package require Tk  8.6} err]
Nitpick: Since TclOO is introduced in patch 3 (and the commit message of 
patch 3 mentions it), this hunk should be in that patch instead.
Show 56 quoted lines
>  } {
>  	catch {wm withdraw .}
>  	tk_messageBox \
> @@ -1797,10 +1797,10 @@ proc ui_status {msg} {
>  	}
>  }
>  
> -proc ui_ready {{test {}}} {
> +proc ui_ready {} {
>  	global main_status
>  	if {[info exists main_status]} {
> -		$main_status show [mc "Ready."] $test
> +		$main_status show [mc "Ready."]
>  	}
>  }
>  
> @@ -2150,8 +2150,6 @@ proc incr_font_size {font {amt 1}} {
>  ##
>  ## ui commands
>  
> -set starting_gitk_msg [mc "Starting gitk... please wait..."]
> -
>  proc do_gitk {revs {is_submodule false}} {
>  	global current_diff_path file_states current_diff_side ui_index
>  	global _gitdir _gitworktree
> @@ -2206,10 +2204,11 @@ proc do_gitk {revs {is_submodule false}} {
>  		set env(GIT_WORK_TREE) $_gitworktree
>  		cd $pwd
>  
> -		ui_status $::starting_gitk_msg
> -		after 10000 {
> -			ui_ready $starting_gitk_msg
> -		}
> +		set status_operation [$::main_status \
> +			start \
> +			[mc "Starting %s... please wait..." "gitk"]]
> +
> +		after 3500 [list $status_operation stop]
>  	}
>  }
>  
> @@ -2240,10 +2239,11 @@ proc do_git_gui {} {
>  		set env(GIT_WORK_TREE) $_gitworktree
>  		cd $pwd
>  
> -		ui_status $::starting_gitk_msg
> -		after 10000 {
> -			ui_ready $starting_gitk_msg
> -		}
> +		set status_operation [$::main_status \
> +			start \
> +			[mc "Starting %s... please wait..." "git-gui"]]
> +
> +		after 3500 [list $status_operation stop]
>  	}
>  }
Looks good. Thanks for the cleanup.
Show 45 quoted lines
>  
> @@ -4159,6 +4159,9 @@ if {$picked && [is_config_true gui.autoexplore]} {
>  	do_explore
>  }
>  
> +# Clear "Initializing..." status
> +after 500 {$main_status show ""}
> +
>  # Local variables:
>  # mode: tcl
>  # indent-tabs-mode: t
> diff --git a/lib/blame.tcl b/lib/blame.tcl
> index a1aeb8b96e..bfcacd5584 100644
> --- a/lib/blame.tcl
> +++ b/lib/blame.tcl
> @@ -24,6 +24,7 @@ field w_cviewer  ; # pane showing commit message
>  field finder     ; # find mini-dialog frame
>  field gotoline   ; # line goto mini-dialog frame
>  field status     ; # status mega-widget instance
> +field status_operation ; # operation displayed by status mega-widget
>  field old_height ; # last known height of $w.file_pane
>  
>  
> @@ -274,6 +275,7 @@ constructor new {i_commit i_path i_jump} {
>  	pack $w_cviewer -expand 1 -fill both
>  
>  	set status [::status_bar::new $w.status]
> +	set status_operation {}
>  
>  	menu $w.ctxm -tearoff 0
>  	$w.ctxm add command \
> @@ -602,16 +604,23 @@ method _exec_blame {cur_w cur_d options cur_s} {
>  	} else {
>  		lappend options $commit
>  	}
> +
> +	# We may recurse in from another call to _exec_blame and already have
> +	# a status operation.
> +	if {$status_operation == {}} {
> +		set status_operation [$status start \
> +			$cur_s \
> +			[mc "lines annotated"]]
> +	} else {
> +		$status_operation show $cur_s
> +	}

IIUC, in the previous version, a 'start' would reset the progress/"meter". But this change only resets the label, not the actual progress, which I think is what the caller wanted. So I think this should be a full re-start instead.

Show 32 quoted lines
> +
>  	lappend options -- $path
>  	set fd [eval git_read --nice blame $options]
>  	fconfigure $fd -blocking 0 -translation lf -encoding utf-8
>  	fileevent $fd readable [cb _read_blame $fd $cur_w $cur_d]
>  	set current_fd $fd
>  	set blame_lines 0
> -
> -	$status start \
> -		$cur_s \
> -		[mc "lines annotated"]
>  }
>  
>  method _read_blame {fd cur_w cur_d} {
> diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
> index 80f5a59bbb..1ea0c9f7b8 100644
> --- a/lib/choose_repository.tcl
> +++ b/lib/choose_repository.tcl
> @@ -9,6 +9,18 @@ field w_body      ; # Widget holding the center content
>  field w_next      ; # Next button
>  field w_quit      ; # Quit button
>  field o_cons      ; # Console object (if active)
> +
> +# Status mega-widget instance during _do_clone2 (used by _copy_files and
> +# _link_files). Widget is destroyed before _do_clone2 calls
> +# _do_clone_checkout
> +field o_status
> +
> +# Operation displayed by status mega-widget during _do_clone_checkout =>
> +# _readtree_wait => _postcheckout_wait => _do_clone_submodules =>
> +# _do_validate_submodule_cloning. The status mega-widget is a difference
> +# instance than that stored in $o_status in earlier operations.

The last sentence doesn't make a lot of sense to me. What is "earlier operations"? If this refers to previous versions of this file, then I don't think such a comment belongs here. It should be in the commit message instead.

Show 66 quoted lines
> +field o_status_op
> +
>  field w_types     ; # List of type buttons in clone
>  field w_recentlist ; # Listbox containing recent repositories
>  field w_localpath  ; # Entry widget bound to local_path
> @@ -659,12 +671,12 @@ method _do_clone2 {} {
>  
>  	switch -exact -- $clone_type {
>  	hardlink {
> -		set o_cons [status_bar::two_line $w_body]
> +		set o_status [status_bar::two_line $w_body]
>  		pack $w_body -fill x -padx 10 -pady 10
>  
> -		$o_cons start \
> +		set status_op [$o_status start \
>  			[mc "Counting objects"] \
> -			[mc "buckets"]
> +			[mc "buckets"]]
>  		update
>  
>  		if {[file exists [file join $objdir info alternates]]} {
> @@ -689,6 +701,7 @@ method _do_clone2 {} {
>  			} err]} {
>  				catch {cd $pwd}
>  				_clone_failed $this [mc "Unable to copy objects/info/alternates: %s" $err]
> +				$status_op stop
>  				return
>  			}
>  		}
> @@ -700,7 +713,7 @@ method _do_clone2 {} {
>  			-directory [file join $objdir] ??]
>  		set bcnt [expr {[llength $buckets] + 2}]
>  		set bcur 1
> -		$o_cons update $bcur $bcnt
> +		$status_op update $bcur $bcnt
>  		update
>  
>  		file mkdir [file join .git objects pack]
> @@ -708,7 +721,7 @@ method _do_clone2 {} {
>  			-directory [file join $objdir pack] *] {
>  			lappend tolink [file join pack $i]
>  		}
> -		$o_cons update [incr bcur] $bcnt
> +		$status_op update [incr bcur] $bcnt
>  		update
>  
>  		foreach i $buckets {
> @@ -717,10 +730,10 @@ method _do_clone2 {} {
>  				-directory [file join $objdir $i] *] {
>  				lappend tolink [file join $i $j]
>  			}
> -			$o_cons update [incr bcur] $bcnt
> +			$status_op update [incr bcur] $bcnt
>  			update
>  		}
> -		$o_cons stop
> +		$status_op stop
>  
>  		if {$tolink eq {}} {
>  			info_popup [strcat \
> @@ -747,6 +760,8 @@ method _do_clone2 {} {
>  		if {!$i} return
>  
>  		destroy $w_body
> +
> +		set o_status {}

Should we be calling a destructor for this here? There is the '_delete' method in status_bar.tcl, but I don't see any usages of it so I'm not sure what exactly it is supposed to do.

That said, the previous version of this file doesn't call any sort of destructor either, so maybe we should just leave it like it is for now. I dunno.

Show 31 quoted lines
>  	}
>  	full {
>  		set o_cons [console::embed \
> @@ -976,33 +1010,9 @@ method _do_clone_checkout {HEAD} {
>  	fileevent $fd readable [cb _readtree_wait $fd]
>  }
>  
> -method _do_validate_submodule_cloning {ok} {
> -	if {$ok} {
> -		$o_cons done $ok
> -		set done 1
> -	} else {
> -		_clone_failed $this [mc "Cannot clone submodules."]
> -	}
> -}
> -
> -method _do_clone_submodules {} {
> -	if {$recursive eq {true}} {
> -		destroy $w_body
> -		set o_cons [console::embed \
> -			$w_body \
> -			[mc "Cloning submodules"]]
> -		pack $w_body -fill both -expand 1 -padx 10
> -		$o_cons exec \
> -			[list git submodule update --init --recursive] \
> -			[cb _do_validate_submodule_cloning]
> -	} else {
> -		set done 1
> -	}
> -}
> -

Is there a reason for moving these two methods around? Not that its a bad thing, I'm just curious.

Show 7 quoted lines
>  method _readtree_wait {fd} {
>  	set buf [read $fd]
> -	$o_cons update_meter $buf
> +	$o_status_op update_meter $buf
>  	append readtree_err $buf
>  
>  	fconfigure $fd -blocking 1

Everything other than a couple of minor comments above looks good. Thanks for the quality contribution. Looking forward to finally merging the next and final version of the series :)

-- 
Regards,
Pratyush Yadav
Previous: Jonathan Gilbert via GitGitGadgetNext: Jonathan Gilbert
Message 46 of 57 in “git-gui: revert untracked files by deleting them”
  1. 0/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Oct 30, 2019
  2. 1/2 git-gui: consolidate naming conventionsJonathan Gilbert via GitGitGadget, Oct 30, 2019
  3. Pratyush YadavNov 3, 2019
  4. 2/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Oct 30, 2019
  5. Pratyush YadavNov 3, 2019
  6. Jonathan GilbertNov 4, 2019
  7. Jonathan GilbertNov 4, 2019
  8. Bert WesargOct 30, 2019
  9. Jonathan GilbertOct 30, 2019
  10. Pratyush YadavNov 3, 2019
  11. Jonathan GilbertNov 3, 2019
  12. Pratyush YadavNov 3, 2019
  13. 0/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 7, 2019
  14. 1/2 git-gui: consolidate naming conventionsJonathan Gilbert via GitGitGadget, Nov 7, 2019
  15. 2/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 7, 2019
  16. Pratyush YadavNov 11, 2019
  17. Jonathan GilbertNov 11, 2019
  18. Philip OakleyNov 11, 2019
  19. Jonathan GilbertNov 12, 2019
  20. Philip OakleyNov 12, 2019
  21. Jonathan GilbertNov 12, 2019
  22. Philip OakleyNov 26, 2019
  23. Pratyush YadavNov 12, 2019
  24. Pratyush YadavNov 11, 2019
  25. 0/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 13, 2019
  26. 2/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 13, 2019
  27. Pratyush YadavNov 16, 2019
  28. Jonathan GilbertNov 16, 2019
  29. 1/2 git-gui: consolidate naming conventionsJonathan Gilbert via GitGitGadget, Nov 13, 2019
  30. 0/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 17, 2019
  31. 1/2 git-gui: consolidate naming conventionsJonathan Gilbert via GitGitGadget, Nov 17, 2019
  32. 2/2 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 17, 2019
  33. Pratyush YadavNov 24, 2019
  34. Pratyush YadavNov 19, 2019
  35. Jonathan GilbertNov 19, 2019
  36. 0/3 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 24, 2019
  37. 1/3 git-gui: consolidate naming conventionsJonathan Gilbert via GitGitGadget, Nov 24, 2019
  38. 2/3 git-gui: update status bar to track operationsJonathan Gilbert via GitGitGadget, Nov 24, 2019
  39. Pratyush YadavNov 27, 2019
  40. Jonathan GilbertNov 28, 2019
  41. 3/3 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 24, 2019
  42. Pratyush YadavNov 27, 2019
  43. 0/3 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 28, 2019
  44. 1/3 git-gui: consolidate naming conventionsJonathan Gilbert via GitGitGadget, Nov 28, 2019
  45. 2/3 git-gui: update status bar to track operationsJonathan Gilbert via GitGitGadget, Nov 28, 2019
  46. Pratyush YadavNov 30, 2019
  47. Jonathan GilbertDec 1, 2019
  48. Philip OakleyDec 1, 2019
  49. Jonathan GilbertDec 1, 2019
  50. 3/3 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Nov 28, 2019
  51. 0/3 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Dec 1, 2019
  52. 1/3 git-gui: consolidate naming conventionsJonathan Gilbert via GitGitGadget, Dec 1, 2019
  53. 3/3 git-gui: revert untracked files by deleting themJonathan Gilbert via GitGitGadget, Dec 1, 2019
  54. 2/3 git-gui: update status bar to track operationsJonathan Gilbert via GitGitGadget, Dec 1, 2019
  55. Benjamin PoirierFeb 26, 2020
  56. Pratyush YadavMar 2, 2020
  57. Pratyush YadavDec 5, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.