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

[Jonathan Gilbert <logic@deltaq.org>]

On Wed, Nov 27, 2019 at 3:59 PM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> On 24/11/19 08:37PM, Jonathan Gilbert via GitGitGadget wrote:
> > -proc ui_ready {{test {}}} {
> > +proc ui_ready {} {
>
> This is not quite correct. There is one user of 'ui_ready' that uses
> 'test'. It is in git-gui.sh:2211. It is used when starting gitk. This
> change breaks that call. 10 seconds after opening gitk via the
> "Visualise master's history" option, I get the following error:
>
>   wrong # args: should be "ui_ready"
>       while executing
>   "ui_ready $starting_gitk_msg"
>       ("after" script)
[..]
> I'm not sure if this heuristic/hack is really needed, and that we need
> to keep the "Starting gitk..." message around for 10 seconds.
[..]
> So, I vote for just getting rid of this hack.

Oh geeze, I can't believe I missed this. This looks like it ought to
be relatively straightforward to port to the new operations, though,
which is a more isolated approach (keeping this change's fingers where
they belong), and then the operation provides segregation that means
it can just be ended after X seconds without caring what anything else
might be doing with the status bar. We can independently figure out if
we want to restructure that part. Given that computers are faster now
and that the status bar could end up doing something else in the
meantime (well let's be realistic, probably not, but who knows :-) ),
I'd vote for reducing the time the message is shown from 10 seconds
to, I dunno, 3 or 4 seconds.

One other thing I note is that both `do_gitk` and `do_git_gui` use
`$starting_gitk_msg`, which means that when `do_git_gui` is invoked to
launch a Git Gui child process for a submodule, it will be setting the
status bar text to say that it is launching Gitk.

Speaking of things that are out of scope for this PR, I did notice
this in the code:

> # -- Always start git gui through whatever we were loaded with.  This
> #    lets us bypass using shell process on Windows systems.
> #
> set exe [list [_which git]]

As far as I can tell, there's virtually no connection between the
comment and what the code is actually doing. I haven't yet figured out
what exactly it is or where it comes from, but on my Windows systems,
`git-gui` is actually an EXE file `git-gui.exe`, and I _think_ what it
is doing is running `wish.exe`, which I'm guessing has something to do
with hosting a Tcl interpreter with Win32 support for Tk GUI.

I'm not sure whether the code is doing the right thing here or not,
but I'm pretty sure what it's _not_ doing is figuring out how the
current `git-gui` process was started/is being hosted. :-P

> >  field finder     ; # find mini-dialog frame
> >  field gotoline   ; # line goto mini-dialog frame
> >  field status     ; # status mega-widget instance
> > +field status_operation ; # status operation
>
> Nitpick: The comment doesn't give any information the field name doesn't
> already give. Either remove it or replace it with something more
> descriptive.

Hmm, okay. I didn't want something that felt wildly imbalanced with
respect to the other lines, but you're right that this particular line
is literally just repeating the variable name. :-P

> > +     if {$status_operation == {}} {
> > +             set status_operation [$status start \
> > +                     $cur_s \
> > +                     [mc "lines annotated"]]
>
> The call to this method from '_read_blame' specifies a different $cur_s.
> So shouldn't we be destroying $status_operation (after stopping it), and
> creating a new one?

We can change the text by calling `$status_operation show`.

> >  method _read_blame {fd cur_w cur_d} {
>
> You did not update 'lib/choose_repository.tcl'. It still uses the old
> version of the status bar. Other than that, the rest of the patch looks
> good. Thanks.

Ugh, I can't believe I overlooked this. I was aware of the file using
the status bar, because it's the one place that uses the `two_line`
constructor, but then I forgot to actually make it create and use the
(single concurrent) operation that a `two_line`-er is allowed.

The code in there seems to overload the purpose of the `o_cons`
variable, so that sometimes it is pointing at a status bar and
sometimes it is pointing at whatever `console::embed` returns. I will
change this.

This code also depends heavily on `update` to keep the UI active,
which as I understand it is problematic because it could potentially
result in re-entrance since the user can interact with the UI in the
middle of the operation. I will not make any attempt to change this,
though. :-)

> [0]:
>   Curiously, if I do 'git log -L 2208,+5:git-gui.sh' to find the origins
>   of the line, it leads me to the commit 25476c6. And looking at the
>   commit, it does indeed appear to be the origin of the line since the
>   line is in the post-image, and not the pre-image. But I accidentally
>   noticed the line in a parent of that commit. Looking further, it turns
>   out the line originated in e210e67. Probably a bug in some really old
>   versions of git. Interesting nonetheless.

In e210e67, I see this:

set starting_gitk_msg {Please wait... Starting gitk...}
proc do_gitk {} {
        global tcl_platform ui_status_value starting_gitk_msg

        set ui_status_value $starting_gitk_msg
        after 5000 {
                if {$ui_status_value == $starting_gitk_msg} {
                        set ui_status_value {Ready.}
                }
        }
        ...

In 043f7011, all string comparisons were changed from ==/!= to eq/ne.
The commit message explains that when you use == and !=, Tcl will
attempt to convert either side to numeric if one of the two sides
looks like a numeric. Guess I should review my commit for this error
:-P

-                if {$ui_status_value == $starting_gitk_msg} {
+                if {$ui_status_value eq $starting_gitk_msg} {

In 699d5601 "Refactor our ui_status_value update technique", this became:

set starting_gitk_msg [mc "Starting gitk... please wait..."]
...
        global ... starting_gitk_msg
...
        ui_status $starting_gitk_msg
        after 10000 {
                ui_ready $starting_gitk_msg
        }

Finally it became this in 02efd48f, apparently an unrelated
refactoring removed the global variable declaration:

set starting_gitk_msg [mc "Starting gitk... please wait..."]
...
        ui_status $::starting_gitk_msg
        after 10000 {
                ui_ready $starting_gitk_msg
        }

I gathered this information using Git Gui's blame function, which I
guess is a good demonstration that my latest blame.tcl revision
corrects the problems in the earlier submission :-D

Next revision coming soon.

Jonathan Gilbert