git-gui: allow opening currently selected file #499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @zoliszabo, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you want to see what email(s) would be sent for a submit request, add a PR comment If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtNeed help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
Please edit the commit message so that the first line is shorter than 76 columns, does not end in a |
|
/allow |
|
User zoliszabo is now allowed to use GitGitGadget. WARNING: zoliszabo has no public email address set on GitHub |
zoliszabo
force-pushed
the
git-gui/open-current-file
branch
from
dd07357
to
fce80f1
Compare
zoliszabo
changed the title
|
/submit |
|
Submitted as pull.499.git.1577386915.gitgitgadget@gmail.com WARNING: zoliszabo has no public email address set on GitHub |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -2248,8 +2248,8 @@ proc do_git_gui {} { | |||
| } | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Zoli,
Thanks for the patch.
On 26/12/19 07:01PM, Zoli Szab� via GitGitGadget wrote:
> From: =?UTF-8?q?Zoli=20Szab=C3=B3?= <zoli.szabo@gmail.com>
>
> ...in the default associated app (e.g. in a text editor / IDE).
>
> Many times there's the need to quickly open a source file (the one you're
> looking at in Git GUI) in the predefined text editor / IDE. Of course,
> the file can be searched for in your preferred file manager or directly
> in the text editor, but having the option to directly open the current
> file from Git GUI would be just faster. This change enables just that by:
> - Diff header path context menu -> Open;
Would it be a better idea to have this option in the diff body context
menu (.vpane.lower.diff.body.ctxm) instead? The problem I see with the
way its currently done is visibility/discovery. It is not very likely
for a user to try and click the file name which doesn't give any
indication that it is clickable. So how will someone who hasn't read
this commit message know that they can use this neat feature. The diff
body context menu is much more "visible" IMO.
> - or double-clicking the diff header path.
An alternative to the above suggestion would be to make this path
underlined and blue in color (like a hyperlink in a web browser). This
will give the indication that this is not just plain text.
I like the latter idea more, but I don't mind either.
> One "downside" of the approach is that executable files will be run
> and not opened for editing.
FWIW, I do not see it as a downside at all. The menu option is called
"open" not "edit". So if you click it, you should expect the file to
open. In case its a binary file, executing it is the correct outcome. In
case its a text file, opening it in the editor is the correct outcome.
> Signed-off-by: Zoli Szab� <zoli.szabo@gmail.com>
> ---
> git-gui.sh | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index c1be733e3e..705b97f7ab 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2248,8 +2248,8 @@ proc do_git_gui {} {
> }
> }
>
> -proc do_explore {} {
> - global _gitworktree
> +# Get the system-specific explorer app/command.
> +proc get_explorer {} {
> set explorer {}
Not exactly related to this patch, but this line is redundant. No point
in clearing 'explorer' only to set it in the very next if-else chain.
> if {[is_Cygwin] || [is_Windows]} {
> set explorer "explorer.exe"
> @@ -2259,9 +2259,25 @@ proc do_explore {} {
> # freedesktop.org-conforming system is our best shot
> set explorer "xdg-open"
> }
> + return $explorer
> +}
> +
> +proc do_explore {} {
> + global _gitworktree
> + set explorer [get_explorer]
> eval exec $explorer [list [file nativename $_gitworktree]] &
> }
>
> +# Trigger opening a file (relative to the working tree) by the default
> +# associated app of the OS (e.g. a text editor or IDE).
Nitpick: This comment doesn't really add a whole lot of information. The
procedure name already make it pretty clear that it will open a file.
And it is pretty easy to understand from reading the body of the
procedure that it will be relative to the working tree. So, I think it
can be removed altogether.
But even if you think it should stay, please at least make it more
concise. Maybe something like:
Open file relative to the working tree by the default associated app.
> +# FIXME: What about executables (will be run, not opened for editing)?
Again, I think running the executables is the correct behaviour. So I
don't think this needs any fixing.
> +proc do_file_open {file} {
> + global _gitworktree
> + set explorer [get_explorer]
> + set full_file_path [file join $_gitworktree $file]
> + eval exec $explorer [list [file nativename $full_file_path]] &
This executes $explorer, which is 'explorer.exe' on Windows. I'm not a
heavy Windows user but AFAIK it is a file manager. This makes it quite
different from 'xdg-open' which is used to open _any_ file/URL in the
user's default application. So it also happens to open _directories_ in
the default file explorer which was the original intention of this
procedure.
Have you tested it on Windows? Does 'explorer.exe' do the correct thing?
Looking at MacOS's 'open' man page, I think it should also work like
xdg-open and shouldn't be a problem.
> +}
> +
> set is_quitting 0
> set ret_code 1
>
> @@ -3530,8 +3546,14 @@ $ctxm add command \
> -type STRING \
> -- $current_diff_path
> }
> +$ctxm add command \
> + -label [mc Open] \
> + -command {
> + do_file_open $current_diff_path
> + }
Nitpick: no need to make the command multi-line. So, change it to
something like:
-command {do_file_open $current_diff_path}
> lappend diff_actions [list $ctxm entryconf [$ctxm index last] -state]
> bind_button3 .vpane.lower.diff.header.path "tk_popup $ctxm %X %Y"
> +bind .vpane.lower.diff.header.path <Double-1> {do_file_open $current_diff_path}
>
> # -- Diff Body
> #
Tested on Linux. Works fine. Looking forward to the re-roll.
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Pratyush Yadav <me@yadavpratyush.com> writes:
> Have you tested it on Windows? Does 'explorer.exe' do the correct thing?
>
> Looking at MacOS's 'open' man page, I think it should also work like
> xdg-open and shouldn't be a problem.
> ...
> Tested on Linux. Works fine. Looking forward to the re-roll.
> Subject: Re: [PATCH 1/1] git-gui: add possibility to open currently selected file
The phrasing on the title is a bit awkward. "add possibility to do
X" is better spelled "allow doing X", no?
There was a problem hiding this comment.
On the Git mailing list, Zoli Szabó wrote (reply to this):
Hi Pratyush,
Thanks for your thorough review.
On 2019.12.27 21:34, Pratyush Yadav wrote:
>> ...This change enables just that by:
>> - Diff header path context menu -> Open;
>
> Would it be a better idea to have this option in the diff body context
> menu (.vpane.lower.diff.body.ctxm) instead? The problem I see with the
> way its currently done is visibility/discovery. It is not very likely
> for a user to try and click the file name which doesn't give any
> indication that it is clickable. So how will someone who hasn't read
> this commit message know that they can use this neat feature. The diff
> body context menu is much more "visible" IMO.
>
>> - or double-clicking the diff header path.
>
> An alternative to the above suggestion would be to make this path
> underlined and blue in color (like a hyperlink in a web browser). This
> will give the indication that this is not just plain text.
>
> I like the latter idea more, but I don't mind either.
For me, the body context menu holds the diff-related options, I am not
sure the "Open" fits in there. But I do like your suggestion of making
the filename web browser-link-like. I'll try to implement that.
>> One "downside" of the approach is that executable files will be run
>> and not opened for editing.
>
> FWIW, I do not see it as a downside at all. The menu option is called
> "open" not "edit". So if you click it, you should expect the file to
> open. In case its a binary file, executing it is the correct outcome. In
> case its a text file, opening it in the editor is the correct outcome.
Alright then.
>> +proc do_file_open {file} {
>> + global _gitworktree
>> + set explorer [get_explorer]
>> + set full_file_path [file join $_gitworktree $file]
>> + eval exec $explorer [list [file nativename $full_file_path]] &
>
> This executes $explorer, which is 'explorer.exe' on Windows. I'm not a
> heavy Windows user but AFAIK it is a file manager. This makes it quite
> different from 'xdg-open' which is used to open _any_ file/URL in the
> user's default application. So it also happens to open _directories_ in
> the default file explorer which was the original intention of this
> procedure.
>
> Have you tested it on Windows? Does 'explorer.exe' do the correct thing?
>
> Looking at MacOS's 'open' man page, I think it should also work like
> xdg-open and shouldn't be a problem.
I am in fact working on this patch on Windows. explorer.exe works also
for files and even brings up the "Open with..." dialog for
not-recognized file types.
> Tested on Linux. Works fine. Looking forward to the re-roll.
Thanks for testing it on Linux. Since I am working on Windows, I just
read about `xdg-open` and `open` and assumed everything should work OK.
All other points I agree with you and will push a new version of the patch.
Thanks,
Zoli
There was a problem hiding this comment.
On the Git mailing list, Zoli Szabó wrote (reply to this):
On 2019.12.28 00:32, Junio C Hamano wrote:
> The phrasing on the title is a bit awkward. "add possibility to do
> X" is better spelled "allow doing X", no?
Thank you, Junio, for the hint. Updated the commit message accordingly
(PATCH v2).
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Zoli Szabó <zoli.szabo@gmail.com> writes:
> On 2019.12.28 00:32, Junio C Hamano wrote:
>
>> The phrasing on the title is a bit awkward. "add possibility to do
>> X" is better spelled "allow doing X", no?
>
> Thank you, Junio, for the hint. Updated the commit message accordingly
> (PATCH v2).
Also, do not start the body of the proposed log message with half
sentence. The title is supposed to be a full sentence.
There was a problem hiding this comment.
On the Git mailing list, Zoli Szabó wrote (reply to this):
Adapted the commit message once again (PATCH v3).
On 2019.12.30 01:14, Junio C Hamano wrote:
> Zoli Szabó <zoli.szabo@gmail.com> writes:
>
>> On 2019.12.28 00:32, Junio C Hamano wrote:
>>
>>> The phrasing on the title is a bit awkward. "add possibility to do
>>> X" is better spelled "allow doing X", no?
>>
>> Thank you, Junio, for the hint. Updated the commit message accordingly
>> (PATCH v2).
>
> Also, do not start the body of the proposed log message with half
> sentence. The title is supposed to be a full sentence.
>
>
>
zoliszabo
force-pushed
the
git-gui/open-current-file
branch
from
fce80f1
to
a6fde25
Compare
zoliszabo
changed the title
|
/submit |
|
Submitted as pull.499.v2.git.1577647930.gitgitgadget@gmail.com WARNING: zoliszabo has no public email address set on GitHub |
zoliszabo
force-pushed
the
git-gui/open-current-file
branch
from
a6fde25
to
1b2363b
Compare
|
/submit |
|
Submitted as pull.499.v3.git.1577721419.gitgitgadget@gmail.com WARNING: zoliszabo has no public email address set on GitHub |
| @@ -2248,9 +2248,8 @@ proc do_git_gui {} { | |||
| } | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Zoli,
On 30/12/19 03:56PM, Zoli Szab� via GitGitGadget wrote:
> From: =?UTF-8?q?Zoli=20Szab=C3=B3?= <zoli.szabo@gmail.com>
>
> Many times there's the need to quickly open a source file (the one you're
> looking at in Git GUI) in the predefined text editor / IDE. Of course,
> the file can be searched for in your preferred file manager or directly
> in the text editor, but having the option to directly open the current
> file from Git GUI would be just faster. This change enables just that by:
> - clicking the diff header path (which is now highlighted as a hyperlink)
> - or diff header path context menu -> Open;
Semi-colon left in by mistake?
>
> Note: executable files will be run and not opened for editing.
>
> Signed-off-by: Zoli Szab� <zoli.szabo@gmail.com>
> ---
> git-gui.sh | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index c1be733e3e..8920e4ddb0 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2259,9 +2258,23 @@ proc do_explore {} {
> +# Open file relative to the working tree by the default associated
> app.
> +proc do_file_open {file} {
> + global _gitworktree
> + set explorer [get_explorer]
> + set full_file_path [file join $_gitworktree $file]
> + eval exec $explorer [list [file nativename $full_file_path]] &
IIUC, this line can be simplified to:
exec $explorer [file nativename $full_file_path] &
It works fine for me including on files with spaces in their names, but
a test on Windows would be appreciated just to rule out any hidden
surprises.
No need to send a re-roll just for these two small things. I have
updated the commit locally before pushing the new version out [0]. The
rest of the patch looks good. Will merge. Thanks.
> +}
> +
> set is_quitting 0
> set ret_code 1
>
[0] https://github.com/prati0100/git-gui/tree/zs/open-current-file
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Zoli Szabó wrote (reply to this):
On 2019.12.30 21:41, Pratyush Yadav wrote:
> Hi Zoli,
>
> On 30/12/19 03:56PM, Zoli Szabó via GitGitGadget wrote:
>> From: =?UTF-8?q?Zoli=20Szab=C3=B3?= <zoli.szabo@gmail.com>
>>
>> Many times there's the need to quickly open a source file (the one you're
>> looking at in Git GUI) in the predefined text editor / IDE. Of course,
>> the file can be searched for in your preferred file manager or directly
>> in the text editor, but having the option to directly open the current
>> file from Git GUI would be just faster. This change enables just that by:
>> - clicking the diff header path (which is now highlighted as a hyperlink)
>> - or diff header path context menu -> Open;
>
> Semi-colon left in by mistake?
>
Yes, that should have been a simple period (.), signaling the end of the
sentence.
>> + eval exec $explorer [list [file nativename $full_file_path]] &
>
> IIUC, this line can be simplified to:
>
> exec $explorer [file nativename $full_file_path] &
>
> It works fine for me including on files with spaces in their names, but
> a test on Windows would be appreciated just to rule out any hidden
> surprises.
You're right. It can be simplified like you have proposed. Tested it and
works correctly also on Windows. (Since I have copied that line from the
existing `do_explore` proc, the same simplification can be done there
too. (I did not realize this myself, since I am just learning some TCL
in order to have this feature in Git GUI...))
>
> No need to send a re-roll just for these two small things. I have
> updated the commit locally before pushing the new version out [0]. The
> rest of the patch looks good. Will merge. Thanks.
>
> [0] https://github.com/prati0100/git-gui/tree/zs/open-current-file
>
Thank you very much for your support!
All the best,
Zoli
Many times there's the need to quickly open a source file (the one you're looking at in Git GUI) in the predefined text editor / IDE. Of course, the file can be searched for in your preferred file manager or directly in the text editor, but having the option to directly open the current file from Git GUI would be just faster. This change enables just that by: - clicking the diff header path (which is now highlighted as a hyperlink) - or diff header path context menu -> Open. Note: executable files will be run and not opened for editing. Signed-off-by: Zoli Szabó <zoli.szabo@gmail.com>
zoliszabo
force-pushed
the
git-gui/open-current-file
branch
from
1b2363b
to
5e6be9f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
...in the default associated app (e.g. in a text editor / IDE).
Many times there's the need to quickly open a source file (the one you're
looking at in Git GUI) in the predefined text editor / IDE. Of course,
the file can be searched for in your preferred file manager or directly
in the text editor, but having the option to directly open the current
file from Git GUI would be just faster. This change enables just that by:
Note: executable files will be run and not opened for editing.
Signed-off-by: Zoli Szabó zoli.szabo@gmail.com
Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.
Please read the "guidelines for contributing" linked above!