git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* git-gui: automatically move focus to staged file before typing commit message?
@ 2019-09-14 12:24 Birger Skogeng Pedersen
  2019-09-14 21:15 ` Pratyush Yadav
  0 siblings, 1 reply; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-09-14 12:24 UTC (permalink / raw)
  To: Git List

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-14 12:24 git-gui: automatically move focus to staged file before typing commit message? Birger Skogeng Pedersen
@ 2019-09-14 21:15 ` Pratyush Yadav
  2019-09-14 21:26   ` Johannes Sixt
  2019-09-14 21:27   ` Pratyush Yadav
  0 siblings, 2 replies; 22+ messages in thread
From: Pratyush Yadav @ 2019-09-14 21:15 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: Git List

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-14 21:15 ` Pratyush Yadav
@ 2019-09-14 21:26   ` Johannes Sixt
  2019-09-14 21:27   ` Pratyush Yadav
  1 sibling, 0 replies; 22+ messages in thread
From: Johannes Sixt @ 2019-09-14 21:26 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Birger Skogeng Pedersen, Git List

Am 14.09.19 um 23:15 schrieb Pratyush Yadav:
> On 14/09/19 02:24PM, Birger Skogeng Pedersen wrote:
>> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-14 21:15 ` Pratyush Yadav
  2019-09-14 21:26   ` Johannes Sixt
@ 2019-09-14 21:27   ` Pratyush Yadav
  2019-09-15  7:55     ` Birger Skogeng Pedersen
  1 sibling, 1 reply; 22+ messages in thread
From: Pratyush Yadav @ 2019-09-14 21:27 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: Git List

On 15/09/19 02:45AM, Pratyush Yadav wrote:
> 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?

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.
>  
> > 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-14 21:27   ` Pratyush Yadav
@ 2019-09-15  7:55     ` Birger Skogeng Pedersen
  2019-09-16 18:01       ` Pratyush Yadav
  0 siblings, 1 reply; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-09-15  7:55 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Git List, Johannes Sixt

Hi Pratyush,

On Sat, Sep 14, 2019 at 11:15 PM Pratyush Yadav <me@yadavpratyush.com> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-15  7:55     ` Birger Skogeng Pedersen
@ 2019-09-16 18:01       ` Pratyush Yadav
  2019-09-26 18:33         ` Birger Skogeng Pedersen
  0 siblings, 1 reply; 22+ messages in thread
From: Pratyush Yadav @ 2019-09-16 18:01 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: Git List, Johannes Sixt

On 15/09/19 09:55AM, Birger Skogeng Pedersen wrote:
> Hi Pratyush,
> 
> On Sat, Sep 14, 2019 at 11:15 PM Pratyush Yadav <me@yadavpratyush.com> 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.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-16 18:01       ` Pratyush Yadav
@ 2019-09-26 18:33         ` Birger Skogeng Pedersen
  2019-09-26 19:30           ` Pratyush Yadav
  0 siblings, 1 reply; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-09-26 18:33 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Git List, Johannes Sixt

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-26 18:33         ` Birger Skogeng Pedersen
@ 2019-09-26 19:30           ` Pratyush Yadav
  2019-09-26 21:17             ` Birger Skogeng Pedersen
  0 siblings, 1 reply; 22+ messages in thread
From: Pratyush Yadav @ 2019-09-26 19:30 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: Git List, Johannes Sixt

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-26 19:30           ` Pratyush Yadav
@ 2019-09-26 21:17             ` Birger Skogeng Pedersen
  2019-10-07 16:52               ` Birger Skogeng Pedersen
  0 siblings, 1 reply; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-09-26 21:17 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Git List, Johannes Sixt

On Thu, Sep 26, 2019 at 9:30 PM Pratyush Yadav <me@yadavpratyush.com> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-09-26 21:17             ` Birger Skogeng Pedersen
@ 2019-10-07 16:52               ` Birger Skogeng Pedersen
  2019-10-07 17:11                 ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Birger Skogeng Pedersen
  2019-10-08 17:59                 ` git-gui: automatically move focus to staged file before typing commit message? Pratyush Yadav
  0 siblings, 2 replies; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-10-07 16:52 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Git List, Johannes Sixt

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-07 16:52               ` Birger Skogeng Pedersen
@ 2019-10-07 17:11                 ` Birger Skogeng Pedersen
  2019-10-07 17:11                   ` [PATCH 2/2] git-gui: select staged on ui_comm focus Birger Skogeng Pedersen
  2019-10-13 20:21                   ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Pratyush Yadav
  2019-10-08 17:59                 ` git-gui: automatically move focus to staged file before typing commit message? Pratyush Yadav
  1 sibling, 2 replies; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-10-07 17:11 UTC (permalink / raw)
  To: git; +Cc: Birger Skogeng Pedersen

Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/2] git-gui: select staged on ui_comm focus
  2019-10-07 17:11                 ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Birger Skogeng Pedersen
@ 2019-10-07 17:11                   ` Birger Skogeng Pedersen
  2019-10-16 19:25                     ` Pratyush Yadav
  2019-10-13 20:21                   ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Pratyush Yadav
  1 sibling, 1 reply; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-10-07 17:11 UTC (permalink / raw)
  To: git; +Cc: Birger Skogeng Pedersen

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 <birger.sp@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-10-07 16:52               ` Birger Skogeng Pedersen
  2019-10-07 17:11                 ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Birger Skogeng Pedersen
@ 2019-10-08 17:59                 ` Pratyush Yadav
  2019-10-08 19:46                   ` Birger Skogeng Pedersen
  1 sibling, 1 reply; 22+ messages in thread
From: Pratyush Yadav @ 2019-10-08 17:59 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: Git List, Johannes Sixt

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git-gui: automatically move focus to staged file before typing commit message?
  2019-10-08 17:59                 ` git-gui: automatically move focus to staged file before typing commit message? Pratyush Yadav
@ 2019-10-08 19:46                   ` Birger Skogeng Pedersen
  0 siblings, 0 replies; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-10-08 19:46 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Git List, Johannes Sixt

Hi Pratyush,

On Tue, Oct 8, 2019 at 7:59 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> 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?

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).

> > 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.

> > 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-07 17:11                 ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Birger Skogeng Pedersen
  2019-10-07 17:11                   ` [PATCH 2/2] git-gui: select staged on ui_comm focus Birger Skogeng Pedersen
@ 2019-10-13 20:21                   ` Pratyush Yadav
  2019-10-15 10:51                     ` Birger Skogeng Pedersen
  1 sibling, 1 reply; 22+ messages in thread
From: Pratyush Yadav @ 2019-10-13 20:21 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: git

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:
> Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> ---
>  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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-13 20:21                   ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Pratyush Yadav
@ 2019-10-15 10:51                     ` Birger Skogeng Pedersen
  2019-10-16 19:28                       ` Pratyush Yadav
  0 siblings, 1 reply; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-10-15 10:51 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Git List

Hi Pratyush,

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

Birger

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] git-gui: select staged on ui_comm focus
  2019-10-07 17:11                   ` [PATCH 2/2] git-gui: select staged on ui_comm focus Birger Skogeng Pedersen
@ 2019-10-16 19:25                     ` Pratyush Yadav
  0 siblings, 0 replies; 22+ messages in thread
From: Pratyush Yadav @ 2019-10-16 19:25 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: git

On 07/10/19 07:11PM, Birger Skogeng Pedersen wrote:
> 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 <birger.sp@gmail.com>
> ---
>  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} {

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

> +		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}
> +

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-15 10:51                     ` Birger Skogeng Pedersen
@ 2019-10-16 19:28                       ` Pratyush Yadav
  2019-10-17  5:08                         ` Birger Skogeng Pedersen
  0 siblings, 1 reply; 22+ messages in thread
From: Pratyush Yadav @ 2019-10-16 19:28 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: Git List

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-16 19:28                       ` Pratyush Yadav
@ 2019-10-17  5:08                         ` Birger Skogeng Pedersen
  2019-10-17  5:33                           ` Johannes Sixt
  2019-10-17 18:28                           ` Pratyush Yadav
  0 siblings, 2 replies; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-10-17  5:08 UTC (permalink / raw)
  To: Pratyush Yadav; +Cc: Git List

Hi Pratyush,

On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav <me@yadavpratyush.com> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-17  5:08                         ` Birger Skogeng Pedersen
@ 2019-10-17  5:33                           ` Johannes Sixt
  2019-10-17  6:54                             ` Birger Skogeng Pedersen
  2019-10-17 18:28                           ` Pratyush Yadav
  1 sibling, 1 reply; 22+ messages in thread
From: Johannes Sixt @ 2019-10-17  5:33 UTC (permalink / raw)
  To: Birger Skogeng Pedersen, Pratyush Yadav; +Cc: Git List

Am 17.10.19 um 07:08 schrieb Birger Skogeng Pedersen:
> Hi Pratyush,
> 
> On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav <me@yadavpratyush.com> 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 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-17  5:33                           ` Johannes Sixt
@ 2019-10-17  6:54                             ` Birger Skogeng Pedersen
  0 siblings, 0 replies; 22+ messages in thread
From: Birger Skogeng Pedersen @ 2019-10-17  6:54 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Pratyush Yadav, Git List

Hi Johannes,

On Thu, Oct 17, 2019 at 7:33 AM Johannes Sixt <j6t@kdbg.org> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget
  2019-10-17  5:08                         ` Birger Skogeng Pedersen
  2019-10-17  5:33                           ` Johannes Sixt
@ 2019-10-17 18:28                           ` Pratyush Yadav
  1 sibling, 0 replies; 22+ messages in thread
From: Pratyush Yadav @ 2019-10-17 18:28 UTC (permalink / raw)
  To: Birger Skogeng Pedersen; +Cc: Git List

On 17/10/19 07:08AM, Birger Skogeng Pedersen wrote:
> Hi Pratyush,
> 
> On Wed, Oct 16, 2019 at 9:28 PM Pratyush Yadav <me@yadavpratyush.com> 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?

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2019-10-17 18:28 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-14 12:24 git-gui: automatically move focus to staged file before typing commit message? Birger Skogeng Pedersen
2019-09-14 21:15 ` Pratyush Yadav
2019-09-14 21:26   ` Johannes Sixt
2019-09-14 21:27   ` Pratyush Yadav
2019-09-15  7:55     ` Birger Skogeng Pedersen
2019-09-16 18:01       ` Pratyush Yadav
2019-09-26 18:33         ` Birger Skogeng Pedersen
2019-09-26 19:30           ` Pratyush Yadav
2019-09-26 21:17             ` Birger Skogeng Pedersen
2019-10-07 16:52               ` Birger Skogeng Pedersen
2019-10-07 17:11                 ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Birger Skogeng Pedersen
2019-10-07 17:11                   ` [PATCH 2/2] git-gui: select staged on ui_comm focus Birger Skogeng Pedersen
2019-10-16 19:25                     ` Pratyush Yadav
2019-10-13 20:21                   ` [PATCH 1/2] git-gui: implement proc select_path_in_widget Pratyush Yadav
2019-10-15 10:51                     ` Birger Skogeng Pedersen
2019-10-16 19:28                       ` Pratyush Yadav
2019-10-17  5:08                         ` Birger Skogeng Pedersen
2019-10-17  5:33                           ` Johannes Sixt
2019-10-17  6:54                             ` Birger Skogeng Pedersen
2019-10-17 18:28                           ` Pratyush Yadav
2019-10-08 17:59                 ` git-gui: automatically move focus to staged file before typing commit message? Pratyush Yadav
2019-10-08 19:46                   ` Birger Skogeng Pedersen

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).