git-gui: automatically move focus to staged file before typing commit message?

Hi everyone,


I personally prefer to have the changes I am about to commit visible
in the diff view, while I write my commit message. So usually I do
this:
1. Stage the file(s) I've been working on.
2. Select a file I just staged, so I can see the changes in the diff widget.
3. Jump to the "Commit Message" widget to type up my commit message.
Basically, I would like to be able to skip step 2. When the user
stages the last file in the "Unstaged Changes" widget, no file is
selected and the diff view becomes blank. When this is the case, I
would prefer that git-gui automatically selects one of the staged
files and shows it in the diff widget before I type up my commit
message. Naturally, this automatic selection should **only** happen
when the user chooses focus the "Commit Message" widget.

I propose:
(When the user focuses the "Commit Message" widget, if no file is
currently selected (i.e. diff widget shows no text))
automatically select the first file listed in the "Staged Changes"
widget so the changes of that file show up in the diff widget.

Thoughts?


Best regards,
Birger S Pedersen

On 14/09/19 02:24PM, Birger Skogeng Pedersen wrote:

Hi everyone,


I personally prefer to have the changes I am about to commit visible
in the diff view, while I write my commit message. So usually I do
this:
1. Stage the file(s) I've been working on.
2. Select a file I just staged, so I can see the changes in the diff widget.
3. Jump to the "Commit Message" widget to type up my commit message.
Basically, I would like to be able to skip step 2. When the user
stages the last file in the "Unstaged Changes" widget, no file is
selected and the diff view becomes blank. When this is the case, I
would prefer that git-gui automatically selects one of the staged
files and shows it in the diff widget before I type up my commit
message. Naturally, this automatic selection should **only** happen
when the user chooses focus the "Commit Message" widget.

Why should it only happen when the commit message widget is selected? 
What's wrong with directly switching focus when all the files are 
staged?

What I have in mind is once there are no more files to stage, the focus 
directly goes to the staged files section, and the first staged file 
gets selected. Then if you want you can type in the commit message. And 
conversely, when unstaging things, once all files are unstaged, the 
focus goes directly to the unstaged files section.
 

I propose:
(When the user focuses the "Commit Message" widget, if no file is
currently selected (i.e. diff widget shows no text))
automatically select the first file listed in the "Staged Changes"
widget so the changes of that file show up in the diff widget.

-- 
Regards,
Pratyush Yadav

Am 14.09.19 um 23:15 schrieb Pratyush Yadav:

On 14/09/19 02:24PM, Birger Skogeng Pedersen wrote:

quoted

When the user
stages the last file in the "Unstaged Changes" widget, no file is
selected and the diff view becomes blank. When this is the case, I
would prefer that git-gui automatically selects one of the staged
files and shows it in the diff widget before I type up my commit
message. Naturally, this automatic selection should **only** happen
when the user chooses focus the "Commit Message" widget.

Why should it only happen when the commit message widget is selected? 
What's wrong with directly switching focus when all the files are 
staged?

That was my reaction, too.

What I have in mind is once there are no more files to stage, the focus 
directly goes to the staged files section, and the first staged file 
gets selected.

... or the last one that was staged. Typically, it is a fixup found
during testing that is staged last. Then I like to have a look at the
complete staged changes of that file.

 Then if you want you can type in the commit message. And

conversely, when unstaging things, once all files are unstaged, the 
focus goes directly to the unstaged files section.

Same here.

-- Hannes

On 15/09/19 02:45AM, Pratyush Yadav wrote:

On 14/09/19 02:24PM, Birger Skogeng Pedersen wrote:

quoted

Hi everyone,


I personally prefer to have the changes I am about to commit visible
in the diff view, while I write my commit message. So usually I do
this:
1. Stage the file(s) I've been working on.
2. Select a file I just staged, so I can see the changes in the diff widget.
3. Jump to the "Commit Message" widget to type up my commit message.
Basically, I would like to be able to skip step 2. When the user
stages the last file in the "Unstaged Changes" widget, no file is
selected and the diff view becomes blank. When this is the case, I
would prefer that git-gui automatically selects one of the staged
files and shows it in the diff widget before I type up my commit
message. Naturally, this automatic selection should **only** happen
when the user chooses focus the "Commit Message" widget.

Why should it only happen when the commit message widget is selected? 
What's wrong with directly switching focus when all the files are 
staged?

Re-reading this email, this might have come across as a bit too harsh. 
That was not my intention. I just want to know why you think it should 
only happen when focussing on the commit message, and what in your mind 
is the problem with directly switching when no more files are left.

What I have in mind is once there are no more files to stage, the focus 
directly goes to the staged files section, and the first staged file 
gets selected. Then if you want you can type in the commit message. And 
conversely, when unstaging things, once all files are unstaged, the 
focus goes directly to the unstaged files section.
 

quoted

I propose:
(When the user focuses the "Commit Message" widget, if no file is
currently selected (i.e. diff widget shows no text))
automatically select the first file listed in the "Staged Changes"
widget so the changes of that file show up in the diff widget.

-- 
Regards,
Pratyush Yadav

-- 
Regards,
Pratyush Yadav

Hi Pratyush,

On Sat, Sep 14, 2019 at 11:15 PM Pratyush Yadav [off-list ref] wrote:

Why should it only happen when the commit message widget is selected?
What's wrong with directly switching focus when all the files are
staged?

What I have in mind is once there are no more files to stage, the focus
directly goes to the staged files section, and the first staged file
gets selected. Then if you want you can type in the commit message. And
conversely, when unstaging things, once all files are unstaged, the
focus goes directly to the unstaged files section.

Your questions are fair. My reasoning: I imagine it could be a bit
frustrating that the focus automatically goes away from the "Unstaged
Changes" widget, when the user actually isn't done doing changes.

For instance (as a user);
- Do some changes
- Stage the changes (no more unstaged changes in the repo)
- Realize that you forgot something, jump back to the IDE and make
some more changes
- Jump back again to git-gui, hit refresh
In this scenario, I imagine the user would want to have focus kept on
the "Unstaged Changes" widget. Even if it became empty with files
before.

When the user focus the "Commit Message" widget, the user is kinda
stating "I'm done staging stuff for now". And when that happens, it
really doesn't make sense to show a blank diff any more.

I hope that made sense.

Birger

On 15/09/19 09:55AM, Birger Skogeng Pedersen wrote:

Hi Pratyush,

On Sat, Sep 14, 2019 at 11:15 PM Pratyush Yadav [off-list ref] wrote:

quoted

Why should it only happen when the commit message widget is selected?
What's wrong with directly switching focus when all the files are
staged?

What I have in mind is once there are no more files to stage, the focus
directly goes to the staged files section, and the first staged file
gets selected. Then if you want you can type in the commit message. And
conversely, when unstaging things, once all files are unstaged, the
focus goes directly to the unstaged files section.

Your questions are fair. My reasoning: I imagine it could be a bit
frustrating that the focus automatically goes away from the "Unstaged
Changes" widget, when the user actually isn't done doing changes.

I suppose a similar argument can be made against your suggestion though. 
When a user clicks on the commit message buffer, they did one thing: 
click on the buffer. They did not click on any diff. So, wouldn't it be 
disorienting for them if their action of clicking the commit message 
buffer also switches the diff view?

I'm not arguing in favour or against your suggestion, I just want to 
consider all angles/viewpoints before going forward.
 

For instance (as a user);
- Do some changes
- Stage the changes (no more unstaged changes in the repo)
- Realize that you forgot something, jump back to the IDE and make
some more changes
- Jump back again to git-gui, hit refresh
In this scenario, I imagine the user would want to have focus kept on
the "Unstaged Changes" widget. Even if it became empty with files
before.

When the user focus the "Commit Message" widget, the user is kinda
stating "I'm done staging stuff for now". And when that happens, it
really doesn't make sense to show a blank diff any more.

Makes sense. But I'm not sure if this would be beneficial to other 
git-gui users. I'd like to hear about what other people think about this 
change.

-- 
Regards,
Pratyush Yadav

Honestly I'll need some help to get this one implemented. The only
implementation I've got working currently, is to change Alt+4 key bind
to do the following:
- Focus the "Staged Changes" widget (which will select a path in the
list, if it isn't focused already), then
- Focus the "Commit Message" widget

¯\_(ツ)_/¯
Birger

On 26/09/19 08:33PM, Birger Skogeng Pedersen wrote:

Honestly I'll need some help to get this one implemented. The only
implementation I've got working currently, is to change Alt+4 key bind
to do the following:
- Focus the "Staged Changes" widget (which will select a path in the
list, if it isn't focused already), then
- Focus the "Commit Message" widget

Why are you changing the Alt+4 binding? This means your feature won't 
work for people who use the mouse to move around in the UI (which I 
suppose would be a majority).

Did you try binding a script to the FocusIn event of the commit message 
buffer? You can do this like:
  
  bind $ui_comm <FocusIn> {your_script}

I'm not sure if $ui_comm is the correct widget, but you can experiment a 
bit by printing something in your_script to find out for sure.

-- 
Regards,
Pratyush Yadav

On Thu, Sep 26, 2019 at 9:30 PM Pratyush Yadav [off-list ref] wrote:

Why are you changing the Alt+4 binding?

I couldn't really find an easier way before.

This means your feature won't
work for people who use the mouse to move around in the UI (which I
suppose would be a majority).

True. I would much prefer that the staged file is selected on commit
widget focus, regardless of how the focus was changed (hotkey or
mouse).

Did you try binding a script to the FocusIn event of the commit message
buffer? You can do this like:

  bind $ui_comm <FocusIn> {your_script}

I'm not sure if $ui_comm is the correct widget, but you can experiment a
bit by printing something in your_script to find out for sure.

Ah, that's pretty neat! I'll play around with that. Thanks, Pratyush.

Birger

So I kinda got this working, but only when focusing the commit message widget.

I did not manage to get it working when invoking "do_add_all", (e.g.
when pressing CTRL/CMD+i). I added this:

bind $ui_comm <$M1B-Key-i> {do_add_all;select_staged_file;break}
bind $ui_comm <$M1B-Key-I> {do_add_all;select_staged_file;break}

But it seems that the "select_staged_file" procedure is invoked
_before_ "do_add_all" finishes. So that's not working. All changes
gets staged, but no staged change is selected.

And I'm quite stuck. Do I send the unfinished patch, so maybe I can
get some advice? Or is it better to just wait until I have the perfect
patch ready?

Birger

Signed-off-by: Birger Skogeng Pedersen <redacted>
---
 git-gui.sh | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index fd476b6..b7f4d1e 100755
--- a/git-gui.sh
+++ b/git-gui.sh

@@ -2669,25 +2669,31 @@ proc show_less_context {} {
 }
 
 proc focus_widget {widget} {
-	global file_lists last_clicked selected_paths
-	global file_lists_last_clicked
+	global file_lists
 
 	if {[llength $file_lists($widget)] > 0} {
-		set path $file_lists_last_clicked($widget)
-		set index [lsearch -sorted -exact $file_lists($widget) $path]
-		if {$index < 0} {
-			set index 0
-			set path [lindex $file_lists($widget) $index]
-		}
-
+		select_path_in_widget $widget
 		focus $widget
-		set last_clicked [list $widget [expr $index + 1]]
-		array unset selected_paths
-		set selected_paths($path) 1
-		show_diff $path $widget
 	}
 }
 
+proc select_path_in_widget {widget} {
+	global file_lists last_clicked selected_paths
+	global file_lists_last_clicked
+
+	set path $file_lists_last_clicked($widget)
+	set index [lsearch -sorted -exact $file_lists($widget) $path]
+	if {$index < 0} {
+		set index 0
+		set path [lindex $file_lists($widget) $index]
+	}
+
+	set last_clicked [list $widget [expr $index + 1]]
+	array unset selected_paths
+	set selected_paths($path) 1
+	show_diff $path $widget
+}
+
 proc toggle_commit_type {} {
 	global commit_type_is_amend
 	set commit_type_is_amend [expr !$commit_type_is_amend]

-- 
2.23.0.windows.1

When the user focuses the Commit Message widget (to write a message), the
diff view may be blank.

With this patch a staged file is automatically selected when the Commit
Message widget is focused, if no other file is selected (i.e. diff view
is blank).

Signed-off-by: Birger Skogeng Pedersen <redacted>
---
 git-gui.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/git-gui.sh b/git-gui.sh
index b7f4d1e..70b846a 100755
--- a/git-gui.sh
+++ b/git-gui.sh

@@ -2700,6 +2700,15 @@ proc toggle_commit_type {} {
 	do_select_commit_type
 }
 
+proc check_diff_selected {} {
+	global current_diff_path file_lists
+	# If no diff path selected, select a staged file
+	if {$current_diff_path eq {}
+		&& [llength $file_lists($::ui_index)] > 0} {
+		select_path_in_widget $::ui_index
+	}
+}
+
 ######################################################################
 ##
 ## ui construction

@@ -3437,6 +3446,8 @@ pack .vpane.lower.commarea.buffer.header -side top -fill x
 pack .vpane.lower.commarea.buffer.frame -side left -fill y
 pack .vpane.lower.commarea.buffer -side left -fill y
 
+bind $ui_comm <FocusIn> {check_diff_selected}
+
 # -- Commit Message Buffer Context Menu
 #
 set ctxm .vpane.lower.commarea.buffer.ctxm

-- 
2.23.0.windows.1

On 07/10/19 06:52PM, Birger Skogeng Pedersen wrote:

So I kinda got this working, but only when focusing the commit message widget.

Isn't this the point of your feature? You change the view when focusing 
the commit message widget. I remember you were explicitly against doing 
it as soon as all unstaged files were staged. Did you change your point 
of view on that?
 

I did not manage to get it working when invoking "do_add_all", (e.g.
when pressing CTRL/CMD+i). I added this:

bind $ui_comm <$M1B-Key-i> {do_add_all;select_staged_file;break}
bind $ui_comm <$M1B-Key-I> {do_add_all;select_staged_file;break}

But it seems that the "select_staged_file" procedure is invoked
_before_ "do_add_all" finishes. So that's not working. All changes
gets staged, but no staged change is selected.

Hmm, that shouldn't happen. select_staged_file should be executed 
_after_ do_add_all, not before. I haven't looked into your patches yet 
though.
 

And I'm quite stuck. Do I send the unfinished patch, so maybe I can
get some advice? Or is it better to just wait until I have the perfect
patch ready?

If you are stuck on something, and want to share the WIP feature to get 
help/comments, you should mark your patches as "RFC" (Request For 
Comments). This can be done by passing the option '-rfc' to 
`git-format-patch`. This will make your subject prefix to "RFC PATCH" 
instead of just "PATCH".

And yes, it is perfectly OK to send incomplete changes as long as you 
mark them as such, and specify what you need help with.

But I see that you have already sent those patches. I'm not sure when I 
can find time to tinker around with them, so it might take me a couple 
of days to get to them.

-- 
Regards,
Pratyush Yadav

Hi Pratyush,

On Tue, Oct 8, 2019 at 7:59 PM Pratyush Yadav [off-list ref] wrote:

On 07/10/19 06:52PM, Birger Skogeng Pedersen wrote:

quoted

So I kinda got this working, but only when focusing the commit message widget.

Isn't this the point of your feature? You change the view when focusing
the commit message widget. I remember you were explicitly against doing
it as soon as all unstaged files were staged. Did you change your point
of view on that?

Yes, kindof. Sorry for not being more clear about the intent.
I would prefer that the automatic selection of a staged file happens
also when adding files while the Commit Message widget is in focused.
For instance, if the user focuses the Commit Message widget and then
hits CTRL/CMD+i a staged file should be selected (if it isn't
already).

quoted

I did not manage to get it working when invoking "do_add_all", (e.g.
when pressing CTRL/CMD+i). I added this:

bind $ui_comm <$M1B-Key-i> {do_add_all;select_staged_file;break}
bind $ui_comm <$M1B-Key-I> {do_add_all;select_staged_file;break}

But it seems that the "select_staged_file" procedure is invoked
_before_ "do_add_all" finishes. So that's not working. All changes
gets staged, but no staged change is selected.

Hmm, that shouldn't happen. select_staged_file should be executed
_after_ do_add_all, not before. I haven't looked into your patches yet
though.

I don't understand why it doesn't work. I'll play around some more with it.

quoted

And I'm quite stuck. Do I send the unfinished patch, so maybe I can
get some advice? Or is it better to just wait until I have the perfect
patch ready?

If you are stuck on something, and want to share the WIP feature to get
help/comments, you should mark your patches as "RFC" (Request For
Comments). This can be done by passing the option '-rfc' to
`git-format-patch`. This will make your subject prefix to "RFC PATCH"
instead of just "PATCH".

Sorry for not doing that, I'll take not and do that next time.

Thanks!
Birger

Hi Birger,

Your subject is a bit redundant. A reader of this commit can easily see 
the diff and know that you implemented "proc select_path_in_widget". 
What's more important is why you implemented it. That is what should go 
in the commit message. So for example in this patch, you can say 
something like:

  git-gui: move last clicked path selection logic to a separate function

  This same logic will be used elsewhere in a follow-up commit, so make 
  it re-useable.

This is what I came up with at first thought. Maybe something even 
better and concise can say the same thing.

On 07/10/19 07:11PM, Birger Skogeng Pedersen wrote:

There is a change in the order of events here. Earlier, we first 
focussed the widget, and then ran `show_diff`. Now we first run 
`show_diff` (via `select_path_in_widget`), and then focus the widget. 
This won't cause any problems, right?

 	}
 }
 
+proc select_path_in_widget {widget} {
+	global file_lists last_clicked selected_paths
+	global file_lists_last_clicked
+
+	set path $file_lists_last_clicked($widget)
+	set index [lsearch -sorted -exact $file_lists($widget) $path]
+	if {$index < 0} {
+		set index 0
+		set path [lindex $file_lists($widget) $index]
+	}
+
+	set last_clicked [list $widget [expr $index + 1]]
+	array unset selected_paths
+	set selected_paths($path) 1
+	show_diff $path $widget
+}
+
 proc toggle_commit_type {} {
 	global commit_type_is_amend
 	set commit_type_is_amend [expr !$commit_type_is_amend]

Other than that, looks good. There isn't much changed here. Just some 
code moved around.

-- 
Regards,
Pratyush Yadav

Hi Pratyush,

Thanks for reviewing. How does this work, do I send a re-roll of the patch(es)?

Birger

On 07/10/19 07:11PM, Birger Skogeng Pedersen wrote:

Nitpick: Please declare ui_index to be global. That way we can just use 
$ui_index instead of the more tedious $::ui_index.

This would mean the diff shows _only_ when you switch focus to the 
commit message buffer. If the buffer is already in focus, and you stage 
all files via Ctrl-I, the staged diff would not show.

IIRC you were having some trouble with this. A quick suggestion without 
looking too much into the problem is to try putting the logic inside 
`do_add_all` instead of inside the bind event handler.

 # -- Commit Message Buffer Context Menu
 #
 set ctxm .vpane.lower.commarea.buffer.ctxm
-- 
2.23.0.windows.1

-- 
Regards,
Pratyush Yadav

On 15/10/19 12:51PM, Birger Skogeng Pedersen wrote:

Hi Pratyush,

Thanks for reviewing. How does this work, do I send a re-roll of the patch(es)?

Yes, please do.

I mentioned this earlier, and I'll mention this again: I'm not sure 
whether this feature would be a good thing for the larger population. So 
this _might_ not end up being accepted depending on how people react to 
the proposal. I thought I'd let you know to avoid any nasty surprises 
later.

-- 
Regards,
Pratyush Yadav

Hi Pratyush,

On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav [off-list ref] wrote:

I mentioned this earlier, and I'll mention this again: I'm not sure
whether this feature would be a good thing for the larger population. So
this _might_ not end up being accepted depending on how people react to
the proposal. I thought I'd let you know to avoid any nasty surprises
later.

Could you please elaborate on why you think the feature might be
undesired? Why would users not want a staged file to be selected
automatically?

FWIW I've also got 2 comments on this in GH[1].

[1] https://github.com/git-for-windows/git/issues/2341

Best regards,
Birger

Am 17.10.19 um 07:08 schrieb Birger Skogeng Pedersen:

Hi Pratyush,

On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav [off-list ref] wrote:

quoted

I mentioned this earlier, and I'll mention this again: I'm not sure
whether this feature would be a good thing for the larger population. So
this _might_ not end up being accepted depending on how people react to
the proposal. I thought I'd let you know to avoid any nasty surprises
later.

Could you please elaborate on why you think the feature might be
undesired? Why would users not want a staged file to be selected
automatically?

FWIW, I would prefer to experiment with the feature for a few weeks
before it (or a configuration that enables it by default) is baked in.

-- Hannes

Hi Johannes,

On Thu, Oct 17, 2019 at 7:33 AM Johannes Sixt [off-list ref] wrote:

FWIW, I would prefer to experiment with the feature for a few weeks
before it (or a configuration that enables it by default) is baked in.

Yes please do. Obviously I'm glad someone other than me will be
actually testing it.
(As I mentioned earlier) I'm all for it when it comes to using a
config setting, rather than having this as default behaviour. I
propose "gui.autoFocusStaged" as a variable name for the setting.
I'll be doing a re-roll once I get some more spare time.

Birger

On 17/10/19 07:08AM, Birger Skogeng Pedersen wrote:

Hi Pratyush,

On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav [off-list ref] wrote:

quoted

I mentioned this earlier, and I'll mention this again: I'm not sure
whether this feature would be a good thing for the larger population. So
this _might_ not end up being accepted depending on how people react to
the proposal. I thought I'd let you know to avoid any nasty surprises
later.

Could you please elaborate on why you think the feature might be
undesired? Why would users not want a staged file to be selected
automatically?

I have similar arguments to what Philip said in the GitHub link you 
sent. I think software should do what the user tells it to do, and 
should not try to "second guess" them. This second guessing can get 
annoying.

So if I select the commit message buffer, I told the software to select 
it by clicking it. I did not tell it to switch my diff view. This switch 
of view can prove to be disorienting/confusing to a user because they 
did not select any diff. They selected a text box.

At the very least, I think we should have a config option, and it should 
be turned off by default. That said, I'd certainly like to hear what 
other people think on this topic.
 

FWIW I've also got 2 comments on this in GH[1].

[1] https://github.com/git-for-windows/git/issues/2341

-- 
Regards,
Pratyush Yadav