git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jonathan Gilbert <logic@deltaq.org>
To: "Pratyush Yadav me-at-yadavpratyush.com |GitHub Public/Example
	Allow|"  <172q77k4bxwj0zt@sneakemail.com>
Cc: Jonathan Gilbert via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Jonathan Gilbert <rcq8n2xf3v@liamekaens.com>,
	Jonathan Gilbert <JonathanG@iqmetrix.com>
Subject: Re: [PATCH v5 2/3] git-gui: update status bar to track operations
Date: Thu, 28 Nov 2019 01:34:21 -0600
Message-ID: <CAPSOpYvv+QZORJoGSNPisE=S_bAtS5tFtodnK9sHZVuTqVVxFg@mail.gmail.com> (raw)
In-Reply-To: <20191127215503.x2lt2b3nce7q4yj2@yadavpratyush.com>

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

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-30  6:48 [PATCH 0/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-10-30  6:48 ` [PATCH 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-03  0:27   ` Pratyush Yadav
2019-10-30  6:48 ` [PATCH 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-03  7:44   ` Pratyush Yadav
2019-11-04 16:04     ` Jonathan Gilbert
2019-11-04 17:36     ` Jonathan Gilbert
2019-10-30  9:06 ` [PATCH 0/2] " Bert Wesarg
2019-10-30 17:16   ` Jonathan Gilbert
2019-11-03  1:12     ` Pratyush Yadav
2019-11-03  4:41       ` Jonathan Gilbert
2019-11-03  7:54         ` Pratyush Yadav
2019-11-07  7:05 ` [PATCH v2 " Jonathan Gilbert via GitGitGadget
2019-11-07  7:05   ` [PATCH v2 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-07  7:05   ` [PATCH v2 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-11 19:25     ` Pratyush Yadav
2019-11-11 21:55       ` Jonathan Gilbert
2019-11-11 22:59         ` Philip Oakley
2019-11-12  4:49           ` Jonathan Gilbert
2019-11-12 10:45             ` Philip Oakley
2019-11-12 16:29               ` Jonathan Gilbert
2019-11-26 11:22                 ` Philip Oakley
2019-11-12 19:35         ` Pratyush Yadav
2019-11-11 19:35   ` [PATCH v2 0/2] " Pratyush Yadav
2019-11-13  9:56   ` [PATCH v3 " Jonathan Gilbert via GitGitGadget
2019-11-13  9:56     ` [PATCH v3 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-13  9:56     ` [PATCH v3 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-16 15:11       ` Pratyush Yadav
2019-11-16 21:42         ` Jonathan Gilbert
2019-11-17  6:56     ` [PATCH v4 0/2] " Jonathan Gilbert via GitGitGadget
2019-11-17  6:56       ` [PATCH v4 1/2] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-17  6:56       ` [PATCH v4 2/2] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-24 13:09         ` Pratyush Yadav
2019-11-19 15:21       ` [PATCH v4 0/2] " Pratyush Yadav
2019-11-19 16:56         ` Jonathan Gilbert
2019-11-24 20:37       ` [PATCH v5 0/3] " Jonathan Gilbert via GitGitGadget
2019-11-24 20:37         ` [PATCH v5 1/3] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-24 20:37         ` [PATCH v5 2/3] git-gui: update status bar to track operations Jonathan Gilbert via GitGitGadget
2019-11-27 21:55           ` Pratyush Yadav
2019-11-28  7:34             ` Jonathan Gilbert [this message]
2019-11-24 20:37         ` [PATCH v5 3/3] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-11-27 22:03           ` Pratyush Yadav
2019-11-28  8:30         ` [PATCH v6 0/3] " Jonathan Gilbert via GitGitGadget
2019-11-28  8:30           ` [PATCH v6 1/3] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-11-28  8:30           ` [PATCH v6 2/3] git-gui: update status bar to track operations Jonathan Gilbert via GitGitGadget
2019-11-30 23:05             ` Pratyush Yadav
2019-12-01  2:12               ` Jonathan Gilbert
2019-12-01 11:43               ` Philip Oakley
2019-12-01 20:09                 ` Jonathan Gilbert
2019-11-28  8:30           ` [PATCH v6 3/3] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-12-01  2:28           ` [PATCH v7 0/3] " Jonathan Gilbert via GitGitGadget
2019-12-01  2:28             ` [PATCH v7 1/3] git-gui: consolidate naming conventions Jonathan Gilbert via GitGitGadget
2019-12-01  2:28             ` [PATCH v7 2/3] git-gui: update status bar to track operations Jonathan Gilbert via GitGitGadget
2020-02-26  8:24               ` Benjamin Poirier
2020-03-02 18:14                 ` Pratyush Yadav
2019-12-01  2:28             ` [PATCH v7 3/3] git-gui: revert untracked files by deleting them Jonathan Gilbert via GitGitGadget
2019-12-05 18:54             ` [PATCH v7 0/3] " Pratyush Yadav

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPSOpYvv+QZORJoGSNPisE=S_bAtS5tFtodnK9sHZVuTqVVxFg@mail.gmail.com' \
    --to=logic@deltaq.org \
    --cc=172q77k4bxwj0zt@sneakemail.com \
    --cc=JonathanG@iqmetrix.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=rcq8n2xf3v@liamekaens.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git