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

[WIP] Update VCS Integration UX and rework EditorVCSInterface for network features #53371

Closed
wants to merge 24 commits into from

Conversation

twaritwaikar
Copy link
Contributor

@twaritwaikar twaritwaikar commented Oct 3, 2021

This PR solves the conflicts in #39255, makes it merge ready, fixes a few bugs, and updates the EditorVCSInterface API.

This PR adds breaking changes in the VCS integration. VCS plugins made in the past will need heavy changes to comply with the newer API which supports features like push, pull, fetch, branch changes, etc.

The Git plugin is also being updated with this PR at the same pace (godotengine/godot-git-plugin#84).

4.x will use GDExtension instead of GDNative, so I have only opened the PR against 3.x.

@akien-mga akien-mga added this to the 3.5 milestone Oct 5, 2021
@twaritwaikar twaritwaikar changed the title [WIP] Update VCS Interface changes [WIP] Update VCS Editor Plugin UX and change EditorVCSInterface to accommodate network features Oct 5, 2021
Add force push checkbox in the Commit dock. Also add some missing
opportunities for checking the VCS state again on from UI inputs
@twaritwaikar
Copy link
Contributor Author

twaritwaikar commented Oct 6, 2021

I am attaching a few screenshots from a build to date.

A large part of this UI is thanks to @Janglee123's work in the past!

Requesting any comments, suggestions because this PR is going to reach merge-quality soon enough.

image

Split/Unified diffs.
image
image
Binary file paths also appear in the diffs, but their content changes don't show up in the diff viewer.

The Editor stores the remote username and password only in memory, it doesn't save them on the file system due to security issues. Thus, the user has to always add their credentials whenever they open Godot. Could there be a better way where the editor is able to pick up the username and password but not a hacker? A Git remote can be configured to use a PAT token by default (by embedding the token in the remote URL itself) but that also doesn't seem to be much secure either.
image

SSH access needs testing. Access to repositories using a Github PAT token is tested.

@Ansraer
Copy link
Contributor

Ansraer commented Oct 7, 2021

Honestly I am not a fan of having the commit history and the branch toolbar at the bottom. As you already mentioned on rocket chat, the commit UI flows from the top to the bottom. IMO the same should hold true for the rest.
I would have the branch selection (and the accompanying buttons) at the very top. Next would be the history of all commits in this branch, followed by the changes I made on top of them (unstaged, staged files) finally would be the commit message and commit button.

@twaritwaikar
Copy link
Contributor Author

twaritwaikar commented Oct 7, 2021

@Ansraer Yes that makes a lot of sense. I guess users should really consider they are on the proper branch and their HEAD is pointing at the right commit before they even stage anything, and for that the branch selection and commit history should be at the top.

A couple follow ups on that.

  1. Do you feel the commit history should go in chronological order instead of the reverse chronologocal order (like it is now) when the commit history is moved to the top?
  2. Should the push/pull/fetch button necessarily be in the same toolbar? In a more real life workflow, a developer would likely want to pull/fetch changes before making a commit and push changes only after that, which indicates to me that these buttons are probably not placed according to the unidirectional UI flow.

@twaritwaikar
Copy link
Contributor Author

However separating out the network related operations into different areas sounds a bit un-intuitive because the normal sentiment for seeing a pull button is to assume the push button is somewhere close

@groud
Copy link
Member

groud commented Oct 7, 2021

I can't comment on the code much, but nothing stood out as obviously wrong. So that's fine for me.

The interface looks great IMO, the order seems fine to me. I don't mind the branch change at the bottom, as it's not something you would need to use often compared to the other things. Also, I think it makes sense to keep the push/pull button at the bottom, as pushing is usually done when you are done committing things (and it also makes sense to keep the branch next to those buttons).

@twaritwaikar
Copy link
Contributor Author

twaritwaikar commented Oct 7, 2021

Branch creation UI
image

@twaritwaikar
Copy link
Contributor Author

twaritwaikar commented Oct 8, 2021

Remote creation UI
image

I found that having multiple remotes could be a use-case for teams using the fork and merge workflow for contributing code so it made sense to add the remote dialog box right near the branch list and also add an option from creating a new remote.

@twaritwaikar twaritwaikar changed the title [WIP] Update VCS Editor Plugin UX and change EditorVCSInterface to accommodate network features [WIP] Update VCS Integration UX and rework EditorVCSInterface for network features Oct 11, 2021
Store username and SSH key paths in Editor Settings.
Shift the VCS autoload settings registrations to project_settings.cpp
@twaritwaikar twaritwaikar marked this pull request as ready for review October 13, 2021 22:54
@twaritwaikar twaritwaikar requested review from a team as code owners October 13, 2021 22:54
@twaritwaikar
Copy link
Contributor Author

Marking as ready for review ✔️
However, there are a couple of features I want to add to this like being able to set the commit list size from the UI and then writing documentation for the new functions.

But opening this for review because the main feature set is pretty much done.

@twaritwaikar twaritwaikar changed the title [WIP] Update VCS Integration UX and rework EditorVCSInterface for network features Update VCS Integration UX and rework EditorVCSInterface for network features Oct 14, 2021
@twaritwaikar
Copy link
Contributor Author

I am also adding a few more features for deleting branches and remote, and possibly commits, so turning this in WIP back again

@twaritwaikar twaritwaikar changed the title Update VCS Integration UX and rework EditorVCSInterface for network features [WIP] Update VCS Integration UX and rework EditorVCSInterface for network features Oct 15, 2021
@twaritwaikar
Copy link
Contributor Author

Added branch and remote deletion UI. For this, I also added a MenuButton at the end of the network features toolbar and shifted all the less used features over there to reduce clutter in the main UI.

image
image

@twaritwaikar
Copy link
Contributor Author

twaritwaikar commented Oct 16, 2021

Listing a higher-level overview (from my initial notes I made before starting working on this) of the changes in this PR so that it is easier to review. I will be updating this list till the PR reaches a conclusion.

  • Add push workflow
  • Add pull workflow
  • Add fetch workflow
  • Add force push option
  • Make script editor reload the opened files in case their contents are dirty
    • Did this by making ScriptEditor::_reload_scripts() public.
  • Add remote creation/deletion UI
  • Add branch and remote selection UI
  • Add branch creation/deletion UI
  • Display more commit details than just the commit diff in the Version Control dock
  • Add HTTPS and SSH remote support
  • Add HTTPS and SSH credentials saving (this doesn't save passwords in any files, passwords still need to be added per editor startup for security)
  • Fix errors that popup when trying to see the diff of an initial commit (first commit in the repository)
  • Add max commit list size selection UI
  • [NOT DONE YET] Grab the project file path from the UI instead of interpreting it from the project folder.
    • Some people have their Godot projects inside of subdirectories present inside a single Git repository.
  • Add docs for new EditorVCSInterface API functions.
  • Major reworks in godot/godot-git-plugin that fix all observable memory leaks so far and enhances error checking and crash avoidance in some areas.

@fire
Copy link
Member

fire commented Oct 16, 2021

I recommend splitting the work and keeping the pending work not pending for too long. Other contributors have other opinions.

@twaritwaikar
Copy link
Contributor Author

@fire Yes, I would agree that breaking it up makes the most sense. However, this PR is adding only those features which directly affect the EditorVCSInterface API, which needs to stay consistent in the different versions of the API, in other words, the API was revised once for 3.2, and the API is being revised the 2nd time in this PR.

If I were to break the changes to this API into different PRs, then there would be commits in the Godot history that would not have a fully complete revision of the API so the VCS plugins would break for those intermediate versions of Godot, which is where I would like to hear more opinions from other contributors.

@twaritwaikar
Copy link
Contributor Author

In reply to my last comment:

I have squashed a few commits and will now close this PR. I will be opening new PRs with each new PR starting with a commit that directly changes the EditorVCSInterface API.

This way, the PR count for the same features shouldn't get out of control and still make it possible for this PR to be broken into smaller ones.

(I realized the cost of adding in code that is not reviewed or risking the work to be pending for long is not worth adding a few commits in history which don't work with the plugin that's currently in sync with the most recent version of EditorVCSInterface)

@twaritwaikar
Copy link
Contributor Author

twaritwaikar commented Oct 16, 2021

FYI: New PR with only the first batch of changes is up here: #53900

I will add any code changes that I make to address any review comments in the PR, and then rebase the next batch of commits on it, and then send another PR with those commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants