CHE-3614: Rework Git commit window #5081

vinokurig · 2017-05-15T14:17:22Z

What does this PR do?

Reworked Git commit window with changed files panel and check-boxes to select witch file or folder to commit or not:

Made Git chnged files panel as a separate widget to reuse it in 'Commit window' and in 'Changed files window'.

What issues does this PR fix or reference?

#3614

Changelog

Introduction of a new Git commit popup

Release Notes

As preliminary of a bigger work to improve the Git experience inside of Che, this version is introducing a new way to handle your Git commits, with less interactions needed. A new Git commit popup has been added which allow to see the list of changed files from your current HEAD commit.

You can double-click on one of the file in order to review the changes with the diff view.
[add screenshot]
The list of changes can either be displayed as a flat list, or as a tree view. You can select the files you want to commit by using the checkboxes.
We also added options to the popup to allow:

amending the previous commit
automatically push the changes to the remote repository
[add animated gif]

Docs PR

N/A Is not mantioned in current documentation.

codenvy-ci · 2017-05-15T18:37:50Z

Build # 2594 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2594/ to view the results.

tolusha · 2017-05-18T14:17:16Z

...gin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitPresenter.java

-        view.showDialog();
-        view.focusInMessageField();
+        checkState(project != null, "Null project occurred");
+        checkState(project.getLocation().isPrefixOf(appContext.getResource().getLocation()),


Should be CommitAction disabled if one of conditions above are true?

Removed this section

tolusha · 2017-05-18T14:20:36Z

...gin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitPresenter.java

+                           items.put(item.substring(2, item.length()), defineStatus(item.substring(0, 1)));
+                       }
+                       filesToCommit.clear();
+                       allFiles = items.keySet();


I guess items collect files to commit. Separate related code into a method with understandable name.

tolusha · 2017-05-18T14:40:42Z

...gin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitPresenter.java

+        DevMachine devMachine = appContext.getDevMachine();
+        Path location = project.getLocation();
+        Path[] filesToCommitArray = new Path[filesToCommit.size()];
+        filesToCommit.forEach(file -> filesToCommitArray[filesToCommit.indexOf(file)] = Path.valueOf(file));


Use simple for cycle.

I prefer stream, because it fits in one row. Moved to separate method

tolusha · 2017-05-18T14:41:42Z

...gin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitPresenter.java

+                          .then(revision -> {
+                              onCommitSuccess(revision);
+                              if (view.isPushAfterCommit()) {
+                                  String remoteBranch = view.getRemoteBranch();


Push action extract to another method.

tolusha · 2017-05-18T14:44:35Z

...gin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitPresenter.java

+               .then(log -> {
+                   final List<Revision> commits = log.getCommits();
+                   String message = "";
+                   if (commits != null && (!commits.isEmpty())) {


https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/collect/Iterables.html#getFirst-java.lang.Iterable-T-

tolusha · 2017-05-18T14:47:15Z

...ugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitViewImpl.java

+        }
+
+        @Override
+        public Element render(final Node node, final String domID, final Tree.Joint joint, final int depth) {


Hard to understand

Added comments

codenvy-ci · 2017-05-19T07:36:25Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2630/

codenvy-ci · 2017-05-19T11:21:10Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2633/

bmicklea · 2017-05-19T14:29:45Z

@slemeur can you write up a bigger release note section for this PR - it's a pretty big deal.

tolusha · 2017-05-19T14:32:56Z

...gin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitPresenter.java

+                   final List<Revision> commits = log.getCommits();
+                   String message = "";
+                   if (commits != null && (!commits.isEmpty())) {
+                       final Revision tip = commits.get(0);


https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/collect/Iterables.html#getFirst-java.lang.Iterable-T-

tolusha · 2017-05-19T14:34:26Z

...git/src/main/resources/org/eclipse/che/ide/ext/git/client/GitLocalizationConstant.properties

 view.commit.amend_field_title=Amend previous commit
 view.commit.grid.date=Date
 view.commit.grid.commiter=Committer
 view.commit.grid.comment=Comment
+view.commit.push.checkbox.title=Push commited changes to :


tolusha · 2017-05-19T14:35:05Z

...git/src/main/resources/org/eclipse/che/ide/ext/git/client/GitLocalizationConstant.properties

 view.commit.amend_field_title=Amend previous commit
 view.commit.grid.date=Date
 view.commit.grid.commiter=Committer
 view.commit.grid.comment=Comment
+view.commit.push.checkbox.title=Push commited changes to :
+view.commit.nothing_to_commit.text=Nothing to commit, working directory clean, Would you like to perform amend commit?


working directory is clean.

tolusha · 2017-05-19T14:48:08Z

...gin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitPresenter.java

+                   service.log(appContext.getDevMachine(), project.getLocation(), null, -1, -1, false)
+                          .then(arg -> {
+                              if (diff.isEmpty()) {
+                                  dialogFactory.createConfirmDialog(constant.commitTitle(),


Extract to a method to simplify code.

slemeur · 2017-05-19T15:38:01Z

@bmicklea : done. will add the images for the RN once merged.

codenvy-ci · 2017-05-22T09:13:04Z

Build # 2639 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2639/ to view the results.

dmytro-ndp

(sorry - wrong issue)

tolusha · 2017-05-22T11:57:01Z

...ugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitViewImpl.java

-        this.commitAllSelection.setValue(event.getValue());
+    @Override
+    public void
+    checkCheckBoxes(Set<Path> paths) {


Single line

vzhukovs · 2017-05-23T09:12:13Z

...ugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitViewImpl.java

-        }
+    @Override
+    public void setChangedPanelView(ChangedPanelView changedPanelView) {
+        this.render = new ChangedPanelRender(changedPanelView);


Can you initialize renderer once? Inside implementation of ChangedPanelView?

The idea is to have an abbility to set custom render to ChangesPanelView to not to overload this widget with logic that is used only for commit window. Changes list window uses this panel with default render.

vzhukovs · 2017-05-23T09:18:47Z

...ugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitViewImpl.java

+        }
+
+        void handleNodeCheckBox(Path nodePath, boolean value) {
+            allNodePaths.stream()


What does this method do? Can you add a little comment to understand the logic?

added javadoc

vzhukovs · 2017-05-23T09:19:35Z

...ugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/commit/CommitViewImpl.java

+                        .forEach(path -> saveCheckBoxSelection(value, path));
+        }
+
+        private void saveCheckBoxSelection(boolean checkBoxValue, Path path) {


checkBoxValue has greater priority than path?

fliped parameters

vzhukovs · 2017-05-23T09:21:27Z

...c/main/java/org/eclipse/che/ide/ext/git/client/compare/changedlist/ChangedListPresenter.java

+        this.changedPanelPresenter = changedPanelPresenter;
+        this.view.setDelegate(this);
+
+        callBack = node -> {


What callback? Rename variable to more informative

Removed this and used existed SelectionChangedHandler

vzhukovs · 2017-05-23T09:27:07Z

.../main/java/org/eclipse/che/ide/ext/git/client/compare/changedlist/ChangedListViewImpl.ui.xml

+<ui:UiBinder xmlns:ui='urn:ui:com.google.gwt.uibinder'
+             xmlns:g='urn:import:com.google.gwt.user.client.ui'>
+    <g:FlowPanel>
+        <g:FlowPanel ui:field="changedPanel"/>


changedPanel -> changesPanel. It sounds like your panel has been changed.

vzhukovs · 2017-05-23T09:30:50Z

...-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/changedpanel/CallBack.java

+/**
+ * Callback for triggering actions in changed files panel.
+ */
+public interface CallBack {


Node selection callback. But I'd propose to reuse existed components either org.eclipse.che.ide.ui.smartTree.event.SelectionChangedEvent.SelectionChangedHandler or com.google.gwt.event.logical.shared.SelectionHandler

vzhukovs · 2017-05-23T09:33:50Z

...main/java/org/eclipse/che/ide/ext/git/client/compare/changedpanel/ChangedPanelPresenter.java

+    }
+
+    private void viewChangedFiles() {
+        if (treeViewEnabled) {


Replace treeViewEnabled with enum with two values TREE and LIST. It would be better to understand the logic

vzhukovs · 2017-05-23T09:34:54Z

.../src/main/java/org/eclipse/che/ide/ext/git/client/compare/changedpanel/ChangedPanelView.java

+    /**
+     * Clear panel from old nodes.
+     */
+    void clearNodeStorage();


clearNodeStorage -> resetPanelState or smth else.

vzhukovs · 2017-05-23T09:36:02Z

.../src/main/java/org/eclipse/che/ide/ext/git/client/compare/changedpanel/ChangedPanelView.java

@@ -60,49 +59,68 @@
     * @param files
     *         Map of changed files with their status
     */
-    void viewChangedFilesAsList(@NotNull Map<String, Status> files);
+    void viewChangedFilesAsList(Map<String, Status> files);


You have two method to show files, as above I mentioned, create enum and only one method, which will consume file list and view mode.

vzhukovs · 2017-05-23T09:39:17Z

...git/src/main/resources/org/eclipse/che/ide/ext/git/client/GitLocalizationConstant.properties

 view.commit.amend_field_title=Amend previous commit
 view.commit.grid.date=Date
 view.commit.grid.commiter=Committer
 view.commit.grid.comment=Comment
+view.commit.push.checkbox.title=Push committed changes to :
+view.commit.nothing_to_commit.text=Nothing to commit, working directory is clean, Would you like to perform amend commit?


Would -> would ?

Changed the question to new sentence

codenvy-ci · 2017-05-24T08:18:48Z

Build # 2656 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2656/ to view the results.

codenvy-ci · 2017-05-24T12:04:48Z

Build # 2660 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2660/ to view the results.

codenvy-ci · 2017-05-24T13:53:17Z

Build # 2663 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2663/ to view the results.

vinokurig · 2017-05-31T07:18:03Z

Selenium tests report:

[TEST] Command line: ./webdriver.sh --test=com.codenvy.ide.git.*
[TEST]
[TEST] Local results:
[TEST] 	- Run: 	     36
[TEST] 	- Failed:     4
[TEST]
[TEST] REGRESSION (4):
[TEST] 	com.codenvy.ide.git.ImportProjectIntoSpecifiedBranchTest.prepare
[TEST] 	com.codenvy.ide.git.ImportRecursiveSubmoduleTest.checkImportProjectSubmoduleByHttpsUrl
[TEST] 	com.codenvy.ide.git.ImportRecursiveSubmoduleTest.checkImportProjectSubmoduleBySshUrl
[TEST] 	com.codenvy.ide.git.ImportRecursiveSubmoduleTest.checkImportProjectSubmoduleFromGithub

Same tests are also failed on nightly-onprem-selenium-tests of current ci job

tolusha · 2017-05-31T07:20:21Z

What is the reason. It is possible to fix them?

vinokurig · 2017-05-31T07:33:39Z

ImportProjectIntoSpecifiedBranchTest: reruned with local mode without regression
ImportRecursiveSubmoduleTest is failing with a known bug (#5178)

Reworked Git commit window with changed files panel and check-boxes to select witch file or folder to commit or not

CHE-3614: Rework Git commit window

be271ff

vinokurig added the kind/enhancement A feature request - must adhere to the feature request template. label May 15, 2017

vinokurig requested review from vparfonov and vzhukovs May 15, 2017 14:17

Igor Vinokur added 5 commits May 17, 2017 09:30

fixup! CHE-3614: Rework Git commit window

ffd4f3f

Merge branch 'master' of github.com:eclipse/che into CHE-3614

7b4c4ce

Merge branch 'master' of github.com:eclipse/che into CHE-3614

2f63cb6

Merge branch 'master' of github.com:eclipse/che into CHE-3614

c3be9d1

fixup! CHE-3614: Rework Git commit window

0aaa755

tolusha reviewed May 19, 2017

View reviewed changes

Merge branch 'master' of github.com:eclipse/che into CHE-3614

c9a5476

iamslite mentioned this pull request May 19, 2017

Git dedicated panel and integration #5128

Closed

15 tasks

fixup! CHE-3614: Rework Git commit window

163907f

vinokurig requested a review from bmicklea May 19, 2017 12:10

bmicklea requested review from slemeur and removed request for bmicklea May 19, 2017 14:29

tolusha reviewed May 19, 2017

View reviewed changes

Igor Vinokur added 2 commits May 22, 2017 09:35

Merge branch 'master' of github.com:eclipse/che into CHE-3614

c975066

fixup! CHE-3614: Rework Git commit window

942e659

dmytro-ndp reviewed May 22, 2017

View reviewed changes

tolusha reviewed May 22, 2017

View reviewed changes

vzhukovs reviewed May 23, 2017

View reviewed changes

Merge branch 'master' of github.com:eclipse/che into CHE-3614

fbf4318

vzhukovs reviewed May 23, 2017

View reviewed changes

Igor Vinokur added 2 commits May 24, 2017 10:18

fixup! CHE-3614: Rework Git commit window

ad233c0

Merge branch 'master' of github.com:eclipse/che into CHE-3614

8924ec9

vzhukovs approved these changes May 24, 2017

View reviewed changes

fixup! CHE-3614: Rework Git commit window

d0ec0a7

slemeur approved these changes May 24, 2017

View reviewed changes

fixup! CHE-3614: Rework Git commit window

e6563e6

vparfonov approved these changes May 29, 2017

View reviewed changes

vinokurig added the status/pending-merge label May 30, 2017

vinokurig merged commit f2047ea into master May 31, 2017

vinokurig deleted the CHE-3614 branch May 31, 2017 19:00

vinokurig added this to the 5.12.0 milestone Jun 12, 2017

slemeur removed the status/pending-merge label Jun 15, 2017

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

CHE-3614: Rework Git commit window (eclipse-che#5081)

21a35d2

Reworked Git commit window with changed files panel and check-boxes to select witch file or folder to commit or not

CHE-3614: Rework Git commit window #5081

CHE-3614: Rework Git commit window #5081

Conversation

vinokurig commented May 15, 2017 • edited by slemeur Loading

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

codenvy-ci commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented May 19, 2017

codenvy-ci commented May 19, 2017

bmicklea commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slemeur commented May 19, 2017

codenvy-ci commented May 22, 2017

dmytro-ndp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzhukovs May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinokurig May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented May 24, 2017

codenvy-ci commented May 24, 2017

codenvy-ci commented May 24, 2017

vinokurig commented May 31, 2017 • edited Loading

tolusha commented May 31, 2017

vinokurig commented May 31, 2017

vinokurig commented May 15, 2017 •

edited by slemeur

Loading

dmytro-ndp left a comment •

edited

Loading

vzhukovs May 23, 2017 •

edited

Loading

vinokurig May 24, 2017 •

edited

Loading

vinokurig commented May 31, 2017 •

edited

Loading