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

Improve Git commit window #3614

Closed
vinokurig opened this issue Jan 5, 2017 · 30 comments
Closed

Improve Git commit window #3614

vinokurig opened this issue Jan 5, 2017 · 30 comments
Assignees
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.

Comments

@vinokurig
Copy link
Contributor

vinokurig commented Jan 5, 2017

Git commit window needs to be improved.

Proposal of the new Git commit window view:
git commit
Commit will be performed for files that are marked with the help of check-boxes.

  • 'delete' button will remove the selected file
  • 'reset index' button will revert changes in selected file
  • double click on item will show 'git compare' window

Important! : For now we do not have possibility to paste check-boxes in tree widget, so we can have only a list of changed files with check boxes. 'Group by directory' button will not be implemented.

@vinokurig vinokurig added the kind/task Internal things, technical debt, and to-do tasks to be performed. label Jan 5, 2017
@vkuznyetsov vkuznyetsov added sprint/next status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it. team/enterprise labels Jan 5, 2017
@slemeur
Copy link
Contributor

slemeur commented Jan 9, 2017

@vinokurig : First it's really nice to see your thinking on this.

I wanted to give you some feedbacks so you can think about them:

1- Options header in explorer
I don't think you need the 'delete' and 'reset index' - those should be doable with a right-click on selected files directly in the list of files.
Same for 'collapse' and 'expand' - clicking in the list of file as you do in tree should be enough.
The reason is that, it is confusing to the user to understand which files will be "reset", are the ones selected in the list, or the ones with the checkbox active?

2- Git compare
I actually think that quickly checking the changes before doing the commit is quite important flow. We should make that even simpler.
I would put a git compare view (similar to the one on github) on the right of the list of files. This way, the user can navigate in the list of file (with the 'up' and 'down' arrow on the keyboard) and review all changes, very quickly - without having to double click on a file, to get a new popup and then close it, and again to review all changes.

3- Amend
I think the check box for amend could be between the list of files and the commit message.

4- Push to remote
I would put that check box at the same level than the commit button - but before. This way it associates the two actions 'commit + push' - otherwise they feel a bit disconnected.
Instead of having the dropdown to select the remote - I would list branches - wdyt?

@vinokurig
Copy link
Contributor Author

@slemeur

  1. Agree
  2. We use Orion to in our compare widget and it does not support viewing particular changes of the file (only changed parts of the file) like in GitHub, it can only show all the file with changes. If user will have many changed files the list would be realy long. I propose to show only selected file in that widget and to change the diff by changing the selection in the files list. Also I propose to place this widget under the files list to make it wider.
  3. Agree
  4. What if user will have several remotes? I think that if it is needed to push to another branch than master, user can use push action.

@vinokurig
Copy link
Contributor Author

@slemeur WDYT?

@slemeur
Copy link
Contributor

slemeur commented Jan 12, 2017

2- Git compare:

  • Great, I think we agreed on the goal.
  • By "git compare view", I don't want to display the side to side view (between old version and new version). Providing the git diff result for the selected file would be enough.
  • GitHub put all git diff for all files modified by the commit in a single page. We would not do that, we would only display the git diff of the selected file from the tree you have in your mockup.
  • User select the file to review in the list of all files changed, he sees changes details (= git diff) on the right.

4- Push to remote
Agree, if user needs to push to another branch than the one we default, then, he can commit and do the push separately.
Push option, would push only on the same branch on the remote (and create it if not exisiting).
if local/feature-branch would be pushed to origin/feature-branch

For managing the remotes:

  • if there is only one remote configured, then we do only the check box;
  • if there are multiple remotes configured, then we add the select box has you had in your mockup.

WDYT?

@jgroom33
Copy link

jgroom33 commented Feb 16, 2017

@slemeur @vinokurig
Please pardon the interruption and consider the following:
Disclaimer: I'm a huge fan of Visual Studio Code.
The simplicity of Git handling is top-notch. It is also Electron based and imitation is the sincerest form of flattery. 😄
https://github.com/Microsoft/vscode
A 'Git View' button would fit nicely below the existing 'Explorer' button on the far left.

@sunix
Copy link
Contributor

sunix commented Apr 13, 2017

Will git -s similar option (to add Signed-off-by) will be added to the dialog ?
Would be nice to have it

@slemeur
Copy link
Contributor

slemeur commented Apr 13, 2017

+1

@vinokurig vinokurig self-assigned this Apr 19, 2017
@vinokurig vinokurig added status/in-progress This issue has been taken by an engineer and is under active development. and removed status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it. labels Apr 19, 2017
@sunix
Copy link
Contributor

sunix commented Apr 19, 2017

Also, for me it makes sense to have 2 lists of changed files:

  1. not indexed
  2. indexed (staged)

Of course a git commit will only take the changes that are in staged

@slemeur
Copy link
Contributor

slemeur commented Apr 19, 2017

Avoid two list because it will be confusing. (could be shown differently than two different lists)
The list of changed files will provide checkboxes for staging or unstaging them.
Only the files that are staged (= with the checkbox active)

Does it resolve your use case?

@sunix
Copy link
Contributor

sunix commented Apr 19, 2017

It is confusing for any Git users to have only 1 list ... and parts of a file can be staged and other parts not staged so check box doesn't help.
I understand that it may be confusing for SVN users to have 2 lists though :)

Example of Eclipse IDE one: https://wiki.eclipse.org/EGit/User_Guide#Committing_with_the_Staging_View

@vinokurig
Copy link
Contributor Author

-1 for 2 lists. Commit will automatically add unstaged files before committing, If user wants to exclude some files from commit he will unmark related checkbox.

@slemeur
Copy link
Contributor

slemeur commented Apr 19, 2017

@sunix :
I'm glad you have been able to find an example with 2 lists. I checked on SourceTree, GitHub desktop and IntelliJ and none of them are using 2 lists.
Also, take into account that if you provide 2 lists, users will expect a nice experience for staging and unstaging files, I don't see intuitive solutions considering that we don't have drag&drop capability.
You are pointing a very particular case, I don't see how, in this issue, we could cover it - even with 2 lists.

Checkboxes here are providing:

  • visual feedback about what are the files which will be part of the commit
  • intuitive interaction to stage/unstage files.

-1 for 2 lists.

@jgroom33
Copy link

@slemeur
Copy link
Contributor

slemeur commented Apr 20, 2017

@jgroom33 : Yes and they have drag&drop capability.
I agree with you when you are pointing the complete "Git View". It's a nice experience. We discussed that for Che as well - but for now we have not prioritize a complete revamp of our Git plugin.

For now, without having drag&drop capability and a rethink git experience - proposed solution by @vinokurig will still provide pleasant and welcomed improvements ^^ !

@jgroom33
Copy link

@slemeur Thanks. You guys rock.
no drag/drop is needed to support the dual list... +/- can be used onHover

@mterente
Copy link

Hi guys, I can't find a way to revert the changes in the currently unstaged files to a previously committed version? Is this available? What about reverting the changes to an earlier commit in the history?

@sunix
Copy link
Contributor

sunix commented May 2, 2017

@jgroom33 @slemeur maybe 2 lists don't have to be at the commit window, but more at the add phase when git user is preparing the commit. Ideally it would be nice to have a UI for the git add -p command.
@slemeur if you have only 1 list and checkboxes, how would you tell if a file is partially in stage ? (part of the file changed not staged, other part staged). maybe the list should be a tree: files with children for each parts of the file

@vinokurig
Copy link
Contributor Author

vinokurig commented May 3, 2017

@sunix The widget will be able to switch between list mode and tree mode, like in 'git compare', when you have more than one changed files. All selected files and folders will be automatically added to index before commit, so there is no need to show if the file is staged or unstaged.

@vinokurig
Copy link
Contributor Author

@mterente Try Git -> Reset and choose 'Hard reset' radio button if you dont want to keep any changes from your previous commit.

@sunix
Copy link
Contributor

sunix commented May 3, 2017

@vinokurig but if the developer uses git add -p to choose part of files to stage or not, this developers won't be able to use the new commit dialog ? what will the commit dialog display ?

@vinokurig
Copy link
Contributor Author

@sunix The commit dialog will display all not committed changes, staged and unstated.The commit button will perform git add and git commit to all selected files. git add -p will not affect to commit dialog at all.

@wernight
Copy link

wernight commented May 7, 2017

Remember to include a forced push as it's necessary in some rare cases.

@sunix
Copy link
Contributor

sunix commented May 7, 2017

@vinokurig so for a file that is partially staged, will it be pre selected or not ? in both case it will be confusing. The problem is not if git add -p will affect the commit dialog, but how the commit dialog will affect your previous git add -p. Sorry to insist but this is important to me. If a git user spend somemtimes in staging complete or partial changes of files, from what you say is that you are going to break this work.

If you don't pre select at all what have been staged previously ... it is even more confusing ... personally will continue in using the command line :p

@slemeur
Copy link
Contributor

slemeur commented May 7, 2017

@sunix : Partially staged files will not be handle at first in this task. If you already have staged files, they will be displayed with "staged" state.
We'll have a separate issue for handling partial files.

As part of a bigger UI/UX work, we'll consider moving to a complete "Git View" view where we can better handle staging and discarding lines as well.
I'll add a design so that the community can comment and discuss.

@sunix
Copy link
Contributor

sunix commented May 8, 2017

@slemeur What happens when it is "displayed" with "staged" state needs to be clarified.
From what I understand, it will stage the whole file. We could do something "simple" in this iteration though making all git users happy:

  • have a selected checkbox in another color. Grey for instance: unchecked = not staged, checked black = staged, checked grey = partially staged. If a user unticks and ticks again a grey checked box, it becomes black.
  • Do not change the staging status, unless any user interaction. If a user do not change a preselected checkbox, the dialog should not change the stage area of this file at commit time.

That way, it will be less confusing and you don't change that much the logic and the UI.

@vinokurig
Copy link
Contributor Author

+1 I like this idea. @slemeur if you agree, I think we need to create separate issue for that.

@vinokurig
Copy link
Contributor Author

@wernight I think forced push should be placed in push window. The commit window should contain only minimum of push functionality.

@wernight
Copy link

+vinokurig Sure if there is also a push dialog for more advanted pushed sure.

@iamslite
Copy link

As a secondary part of this, consider using indicators in the file explorer to show the current state of a file relative to the index. (This has certainly been an option within eclipse in the past.)

@vinokurig vinokurig added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels May 15, 2017
@slemeur
Copy link
Contributor

slemeur commented May 19, 2017

As discussed we have published a specification for a better Git integration in the next version of Che.
Please review the issue #5128 and share your feedbacks.

@iamslite, @wernight, @jgroom33 we hope that this solution will address most of your concerns.

@vinokurig vinokurig added status/pending-merge and removed status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. status/pending-merge labels May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

No branches or pull requests

8 participants