Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VS Code, Sublime Text, Atom as text editors #200

Merged
merged 15 commits into from
Sep 29, 2018
Merged

Conversation

ungps
Copy link
Contributor

@ungps ungps commented Aug 16, 2018

In response to: git-for-windows/git#1795

P.S: "VS Code - Insiders" does not work for me (it is not even detected), but I hope I will be able to fix this too.

@ungps
Copy link
Contributor Author

ungps commented Aug 18, 2018

Ok, I thought I would only fix the problem I reported here, but since I was still here, I pushed some more commits to:

@ungps
Copy link
Contributor Author

ungps commented Aug 20, 2018

Indeed, I will resend a fix for this as soon as possible.

@@ -1133,7 +1133,11 @@ begin

EditorAvailable[GE_NotepadPlusPlus]:=RegQueryStringValue(HKEY_LOCAL_MACHINE,'SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\notepad++.exe','',NotepadPlusPlusPath);
EditorAvailable[GE_VisualStudioCode]:=RegQueryStringValue(HKEY_LOCAL_MACHINE,'SOFTWARE\Classes\Applications\Code.exe\shell\open\command','',VisualStudioCodePath);
if (not EditorAvailable[GE_VisualStudioCode]) then
EditorAvailable[GE_VisualStudioCode]:=RegQueryStringValue(HKEY_CURRENT_USER,'Software\Classes\Applications\Code.exe\shell\open\command','',VisualStudioCodePath);

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@dakotahawkins
Copy link
Contributor

@dscho Would it be possible to set core.editor --system to a "meta" value that triggers a runtime check followed by setting core.editor --global on first use?

@dscho
Copy link
Member

dscho commented Aug 27, 2018

Would it be possible to set core.editor --system to a "meta" value that triggers a runtime check followed by setting core.editor --global on first use?

That is always possible: you could provide a separate program that is then used for the core.editor setting, and which performs said runtime check.

It would require for that meta program to be implemented, though.

Selecting VS Code [Insiders] as default editor did not work
as intended. `Git-for-windows` would install successfully,
but `core.editor` option would not be set.

git-for-windows/git#1795

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
ungps added 3 commits August 31, 2018 17:09
Add `Sublime Text` and `Atom` in installer as default
editors.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Update Visual Studio Code [Insiders] to also look in
HKCU, not only in HKLM.

git-for-windows/git#1741 (comment)

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Allow user to select any editor by selecting the executable's
path.

git-for-windows/git#1741 (comment)

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
@ungps
Copy link
Contributor Author

ungps commented Aug 31, 2018

I force pushed some changes and I hope that the first three commits (all, but "installer: add option to allow unlisted editors") are now ok. I tested it by using the command ./installer/release.sh --debug-wizard-page="Editor". It takes some time to build the installer. I will come back later (after I actaully test the installer, not by just debugging the wizard page) with a reply to confirm if it actually works ok.

@ungps
Copy link
Contributor Author

ungps commented Aug 31, 2018

Ok, I am back. The results

  • Sublime worked fine.
  • Atom worked fine.
  • VSCode 1.25 - works fine (though, it looks into HKLM)
  • VSCodeInsiders 1.27 - doesn't work. (core.editor is not set). This looks into HKCU.

@shiftkey
Copy link
Contributor

I think the VSCode team is going to try and improve their installer so that they can set the global Git config to VSCode, meaning it might be unnecessary to figure out how to support these new "user install" modes in a system config: microsoft/vscode#56973

cc @joaomoreno for any other initial details

@joaomoreno
Copy link

@shiftkey I would actually update the git's installer to also detect Code's user install. Is there any way I can help here?

@shiftkey
Copy link
Contributor

@joaomoreno I think the big questions I have were in previous comments from @dscho - #200 (comment)

@anthcool
Copy link

Can't you just all get along, both take partial blame, and both fix their respective parts to this issue? Bottom line is: Github for desktop team's installer does not work, and VS Code's choice to have a user install option did not account for 3rd party integrations that, up till the new user install option was made available, looks for the system directory install location... Both items should work regardless of whether the one plays nice with the other. This is called loose coupling in our industry...

Sincerely, a fan of both, attempting to use both...

@shiftkey
Copy link
Contributor

Bottom line is: Github for desktop team's installer does not work ...

@anthcool I'm a maintainer of GitHub Desktop and we should support both user and admin VSCode installers since 1.3.4 last month. Please ensure you're up to date, or log an issue over on our repository with more details: https://github.com/desktop/desktop.

This issue is for the Git for Windows installer.

@anthcool
Copy link

anthcool commented Sep 20, 2018

@shiftkey Thanks for clearing this up for me. I feel like a dork. The whole time I was trying to update Git itself to version 2.19.0 from 2.18.0.windows.1 and am having this same problem. If I try to update Git as described, using the installer downloaded from https://git-scm.com/downloads, and get to the default editor picker part, as soon as I go to choose VS Code as the default, or anything other than Vim, the Next button grays out/disables.... Ugh.. Thanks again though!

@shiftkey
Copy link
Contributor

@anthcool it's this bit here that I believe is the blocker on any real solution #200 (comment)

@dscho
Copy link
Member

dscho commented Sep 28, 2018

* VSCodeInsiders 1.27 - doesn't work. (`core.editor` is not set). This looks into HKCU.

I tried with VS Code Insiders 1.28 and it did work...

Also, I verified that, for some reason, InnoSetup can see my user registry even if I run the installer elevated (as asked for in non-debugging mode). I have a couple of patches on top that I will push in a moment.

The majority of the lines in the `install.iss` script are indented using
spaces, using 4 spaces per indentation levels.

Over the course of the years, this has not exactly been maintained and a
few changes crept in that are inconsistent with this practice.

Let's make everything consistent again, and while at it, adjust the
.gitattributes accordingly.

Note: there was one stray, lonely semicolon that this patch also
deletes, which is not exactly an indentation fix, but it was indented
using tabs, too, so...

Signed-off-by: Johannes Schindelin <[email protected]>
InnoSetup's Pascal uses 1-based indices into strings. Oddly enough,
starting a substring at index 0 pretends that the caller asked for index
1, but this is not documented, and it is confusing to read for people
who have to switch between Pascal mindset and C mindset. So let's use
the Pascal mindset consistently.

Signed-off-by: Johannes Schindelin <[email protected]>
Many editors on Windows are called through `.exe` files that return
immediately, without waiting for the editor to close. This is not what
Git needs: it needs the process to exit only when the file in question
has been edited. To this end, editors often support options to enable
that behavior, e.g. Visual Studio Code's `--wait` option.

To support this, we allow specifying options after the path to the
editor, but there was no indication so far. Up until now! :-)

Signed-off-by: Johannes Schindelin <[email protected]>
The convention when calling a command with options where the path to the
command contains spaces is to quote the path, e.g.

  "C:\Users\me\AppData\Local\Programs\Microsoft VS Code\Code.exe" --wait

However, the current code expects it to be specified without quotes:

  C:\Users\me\AppData\Local\Programs\Microsoft VS Code\Code.exe --wait

As it is not exactly clear in which form we want the command-line to be
specified, let's allow both forms.

Signed-off-by: Johannes Schindelin <[email protected]>
To allow specifying command-line options after the path to the custom
editor, we always add a trailing space just in case that no options were
specified, so that ".exe " can be found anyway.

However, we failed to cull that extra space when extracting the options.
Let's fix this.

Signed-off-by: Johannes Schindelin <[email protected]>
It *may* be possible that Sublime Text works fine with `--wait`, but
https://help.github.com/articles/associating-text-editors-with-git/ (and
other hits in a web search) suggest `-w` specifically. So let's use the
known-good option.

Signed-off-by: Johannes Schindelin <[email protected]>
It would be a bit unfortunate to configure a custom editor, then install
all of Git for Windows, only to find out the next time the user wants to
e.g. commit something that the editor does not, in fact, work, e.g. when
it requires options to exit after the file in question was closed.

Let's offer a convenient method to test things by adding a button to
edit a test file. Whether everything works as Git expects, or whether
anything went wrong, we'll tell the user about it (and enable/disable
the "Next" button accordingly).

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member

dscho commented Sep 28, 2018

Actually, I had tried only the debug mode to see the editor page, and replicated the issue reported by @ungps. I figured it out and only need to clean up the changes, but I have to be away from the keyboard for a few hours first.

We now support a *lot* more editors than just GNU nano.

Signed-off-by: Johannes Schindelin <[email protected]>
The config entry for the user-wide VS Code [Insiders] installations
needs to be written in a conditional block, but the `else` of this block
was mistaken for the `else` arm of the "inner `if`". Hence it was never
called, or at least not under the intended circumstances.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho merged commit 1b35532 into git-for-windows:master Sep 29, 2018
@dscho
Copy link
Member

dscho commented Sep 29, 2018

There is only one remaining problem I can see with this PR: when a user chose VS Code that was installer for the current user only, and next time chooses an editor that is not only for the current user, the ~/.gitconfig entry will not be removed. However, this entry might have been changed in the meantime, too, in which case we should not touch it anyway! Not sure what to do about it... I guess we could store (in the PreviousData) the editor path installed via git config --global, and the next time, just before we are about to configure an editor option, test whether the user config still refers to the same path and if so, unset that setting.

Any thoughts?

dscho added a commit that referenced this pull request Sep 29, 2018
Sublime Text, Atom, and even the new user-specific VS
Code installations [can now by used as Git's default
editor](#200).

Signed-off-by: Johannes Schindelin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants