Steffen Prohaska <prohaska@xxxxxx> wrote:
> If git-gui is started outside a work tree the repository
> chooser will offer a list of recently opend repositories.
> Clicking on an entry directly opens the repository.
>
> The list of recently opened repositories is stored in the
> config as gui.recentrepos. If the list grows beyond 10
> entries it will be truncated.
>
> Note, only repositories that are opened through the
> repository chooser will get added to the recent list.
> Repositories opened from the shell will not yet be added.
I think that all makes a lot of sense. Three comments below about
this patch in particular.
> I'd suggest to reduce the number of clicks needed to open or
> clone an existing directory that is not in the list of
> recent repositories. First choosing from the radiobuttons
> and then clicking next is one click to much. There are no
> options to combine. Choosing from the list should
> immediately trigger the action.
>
> We could either put 'Create/Clone/Open New Repository' into
> the Repository menu and only present the recent repository
> list. Or we could offer push buttons for the other actions.
I agree entirely. That "Next" button is stupid stupid stupid.
What was I smoking that day? :-)
I'm concerned about putting them into the Repository menu only
as then the main window is competely void and users are sort of
wondering what they should do next. I think we should actually
do both. Put them into the menu and as push buttons on the window.
> + label $w_body.space
> + label $w_body.recentlabel \
> + -anchor w \
> + -text "Select Recent Repository:"
This string needs to be i18n'd with [mc ...].
> + listbox $w_body.recentlist \
Please make a field in this class called say "w_recentlist"
so you can use that field name instead of $w_body.recentlist.
This simplifies the code if we ever have to change the actual path
that the widget resides at, such as to alter the layout.
> +proc _append_recentrepos {path} {
> + set recent [get_config gui.recentrepos]
> + if {[lsearch $recent $path] < 0} {
> + lappend recent $path
> + }
> + if {[llength $recent] > 10} {
> + set recent [lrange $recent 1 end]
> + }
> + regsub -all "\[{}\]" $recent {"} recent
> + git config --global gui.recentrepos $recent
> +}
Why treat this as a Tcl list in a single value? Why not make it
a true multi-value configuration entry in ~/.gitconfig, like how
remote.$name.fetch is a multi-value entry? Does Windows allow
you to put " in a path name? Because the above regex will make
a list of paths that contains " in one of the entries invalid.
I think you also want to have this function return back immediately
if [lsearch $recent $path] >= 0 as then you don't invoke git-config
to perform a no-op change in the configuration file. As you well
know forking on Windows is a major cost. We shouldn't run git-config
just because the user opened a recent repository.
--
Shawn.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
