| Message ID | 20191007171145.1259-1-birger.sp@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] git-gui: implement proc select_path_in_widget | expand |
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.
Hi Pratyush, Thanks for reviewing. How does this work, do I send a re-roll of the patch(es)? Birger
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.
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
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
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
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
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]
Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com> --- git-gui.sh | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)