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
