Add ability to switch between files in Git Diff widget #5965

mmorhun · 2017-08-10T10:01:11Z

What does this PR do?

Adds ability to switch to the next/previous file in git compare widget. The order is defined by git. The editable content is saved on Next/Previous button click.
Adds Save Changes button for git compare widget.
Performs refactoring of git-compare-related functionality.
Fixes compare with deleted file bug.
Moves Git Diff widget from iframe to IDE.
Adds hotkeys for next and previous diff: alt + , and alt + . correspondingly. (actually alt + < or > but without shift).

What issues does this PR fix or reference?

Changelog

Added ability to switch between files in Git Diff widget.

Release Notes

In this version, we continued to work on improving Git support in Eclipse Che. We have improved the Git diff window so that it's simpler and faster to review changes between two states of code.

To view the diff, use Git->Compare-><Select-to-what> from main menu. If more than one file has changed the list of changed files will be opened first. To select a file to compare double-click on it or select a file then click the Compare button.

Your changes are displayed in the left editor and the file being compared to is on the right. The left editor can be used for editing and fixing your changes.

When you have multiple files to review you have the ability to navigate under all the files that are changed. The number of files that are reviewable is displayed in the title of the wizard. Use Next and Previous buttons to navigate to the next or previous file correspondingly. We also introduce new keyboard shortcut for those actions Alt + . for Next and Alt + , for Previous.

Docs PR

eclipse-che/che-docs#283

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

tolusha · 2017-08-10T10:11:07Z

...ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/action/CompareWithLatestAction.java

-                               items.put(item.substring(2, item.length()), defineStatus(item.substring(0, 1)));
-                           }
-                           changesListPresenter.show(items, REVISION, null, project);
+                           changesListPresenter.show(changedItems, REVISION, null);


I personally don't like methods which can accept null values.

Well, I don't like it too, but here I see no problem. It is well documented and mean undefined.

I'm a bit out of the scope however in general I agree with @tolusha. In this very example I would recommend to avoid public method calls with null values (even well documented). I think it would be nice if you could add a corresponding method so we could call changesListPresenter.show(changedItems, REVISION) and it would internally recall changesListPresenter.show(changedItems, REVISION, null) or something similar to this.

Good piece of advice, but not for this case, unfortunately...
Otherwise we have to make additional methods with different names (to cover two nullable parameters), which will cause additional logic on the invocations.

tolusha · 2017-08-10T10:13:45Z

...lugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ChangedItems.java

+
+        changedFilesStatuses = new LinkedHashMap<>();
+        for (String item : diff.split("\n")) {
+            changedFilesStatuses.put(item.substring(2, item.length()), defineStatus(item.substring(0, 1)));


if diff.isEmpty() then you will have an issue

Make sure item.length() > 2
If defineStatus(item.substring(0, 1)) throws IllegalArgumentException then catch it

To have clear code extract variables for item.substring(2, item.length()) and defineStatus(item.substring(0, 1))

No, it will end up with empty diff

vinokurig · 2017-08-10T10:18:09Z

...lugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ChangedItems.java

+    }
+
+    /**
+     * @return number of files in the diff


I prefer not to use single @return annotation in javadoc, but to have something like: "Returns number of files in the diff"

tolusha · 2017-08-10T10:18:48Z

...lugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ChangedItems.java

+
+        changedFilesList = new ArrayList<>(changedFilesStatuses.keySet());
+
+        length = changedFilesList.size();


Redundant field

Just cached. But ok, I'll get rid of it.

vinokurig · 2017-08-10T10:20:35Z

...lugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ChangedItems.java

+    private final int                           length;
+
+    /**
+     * Creates user-friendly representation of git diff.


What do you mean user-frendly? How does it related to user?

well, programmer friendly)

realy? 😄

tolusha · 2017-08-10T10:22:55Z

...lugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ChangedItems.java

+ *
+ * @author Mykola Morhun
+ */
+public class ChangedItems {


ModifiedFiles ?

What if it is added or deleted?

I dont think that Modified or Changed are good words for this class because such words are present in Git Status. This object can contain not only modified/changed but new or deleted
How about DiffFiles?

I think TouchedItems is worse. But still open for discussion.

I'm not sure I understand why do we use term Item here if we're talking exactly about files only according to javadoc? What do you think about something like AlteredFiles? In general I think we can't just name this class ChangedItems as it does not reflect it's nature, personally I would split it into several classes something like ChangedItemParser, ChangedItemHolder and ChangedItem itself, what do you think?

AlteredFiles is good name, think no one will be against it.
As about splitting, I see no point to do this. Constructor just do its own work. Its logic isn't complicated to move into separate class. And according to current representation of diff, storing data into internal object will make it more complicated and add small overhead I think.

tolusha · 2017-08-10T10:30:23Z

...n-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ComparePresenter.java

        this.compareWithLatest = true;
+        view.setEnableSaveChangesButton(true);
+
+        setupCurrentFile(currentFile);


You don't setup anything. Rename it respectively.

I disagree to remove it. But name could be changed, ok.

tolusha · 2017-08-10T10:34:07Z

...n-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ComparePresenter.java

+        changedItems.getProject()
+                    .getFile(changedItems.getItemByIndex(currentItemIndex))
+                    .then(file -> {
+                        if (file.isPresent()) {


if !file.isPresent() it means it has been deleted. So, we have to show diff also.

Good catch!

By the way, seems that this is broken in master

tolusha · 2017-08-10T10:37:04Z

...n-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ComparePresenter.java

+     * Searches for project with git repository to which given file belongs.
+     */
+    @Nullable
+    private Container getGitDir(final File file) {


getGitRootFolder

See no logical difference here.

Root means not any

tolusha · 2017-08-10T10:37:44Z

...n-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ComparePresenter.java

-
-        if (rootProject == null) {
+        final Container gitDir = getGitDir(file);
+        if (gitDir == null) {


Should we notify user with some message?

tolusha · 2017-08-10T10:40:19Z

...n-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ComparePresenter.java


        CancelCallback cancelCallback = view::hide;

        dialogFactory.createConfirmDialog(locale.compareSaveTitle(), locale.compareSaveQuestion(), locale.buttonYes(), locale.buttonNo(),
                                          confirmCallback, cancelCallback).show();
    }

+    @Override
+    public void onSaveChangesClicked() {
+        if (compareWithLatest) {


Why do we have to check if compareWithLatest ?

Just for case.
The Save Changes button should be disabled when compare between revisions is shown.

tolusha · 2017-08-10T10:42:31Z

...n-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ComparePresenter.java

+    }
+
+    /** @return relative path of given file from specified project */
+    private Path getRelPath(final Container project, final File file) {


It seems like a method for File class.

Maybe, but I think it doesn't 'fit' for current File interface

tolusha · 2017-08-10T10:44:24Z

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

@@ -29,6 +29,9 @@ button.no=No
 button.fetch=Fetch
 button.push=Push
 button.pull=Pull
+button.save_changes=Save Changes
+button.next_diff=Next
+button.previous_diff=Prev


Previous ?

codenvy-ci · 2017-08-10T10:51:53Z

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

vinokurig · 2017-08-10T11:29:11Z

...n-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/compare/ComparePresenter.java

@@ -75,45 +78,106 @@ public ComparePresenter(AppContext appContext,
    }

    /**
-     * Show compare window.
+     * Show compare window for given set of files between given revision and latest code version.


latest code version is not clear enough, I think it should be HEAD

codenvy-ci · 2017-08-23T10:08:30Z

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

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

codenvy-ci · 2017-09-06T10:51:36Z

Build # 3557 - FAILED

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

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

codenvy-ci · 2017-09-06T12:55:43Z

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

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

codenvy-ci · 2017-09-06T13:43:57Z

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

codenvy-ci · 2017-09-07T08:39:21Z

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

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun · 2017-09-07T11:25:05Z

@vparfonov please review new changes

mmorhun · 2017-09-07T11:55:47Z

@slemeur done.
I used alt + , / . (i.e. alt + </> without shift) because alt + left/right are already used in the editor too.

slemeur · 2017-09-07T12:43:05Z

@mmorhun : Great ! Where are the docs ;) ?

codenvy-ci · 2017-09-07T13:05:36Z

Build # 3580 - FAILED

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

mmorhun · 2017-09-07T13:59:37Z

@slemeur well, I wanted to add them ;) , but I failed to find appropriate place. We have some docs in Git and SVN section which mainly about how to use different types of credentials for VCSs. We do not have a page or a section about using git functionality like change branch, commit, push, pull etc. Most of them are intuitively, but some details for git diff widget, like autosave on next/prev or hotkeys combinations, probably will be useful.
So, should I create a section for git diff widget or just leave it as is?

mmorhun · 2017-09-07T14:02:57Z

Maybe we should create separate page in the docs for using git functionality in CHE to not to mess credentials settings and just IDE details/tips.

slemeur · 2017-09-08T06:52:36Z

You should move forward and add the information about Git Diff in the user documentation - the page you found is fine.

I agree with you that the content of the page Git and SVN needs to be revisited and re-organized:

The Git Using OAuth section is more pointing the configuration of the system. So I don't think we should have that in the user documentation, but in the Configuration (which BTW is already listed https://www.eclipse.org/che/docs/setup/configuration/index.html#oauth).
We should probably re-organize the different sections, so that the explanation on the features are first and then we have explanations how to use it with SSH.
The SVN documentation will need to be moved (see Remove svn plugin from default assembly #6144)

Let's start on adding the docs for this PR. If you are able to start to re-organize the content, that would be perfect, but I'll help you on that.

Thanks Mykola.

vparfonov · 2017-09-08T11:29:29Z

ide/che-core-ide-ui/src/main/java/org/eclipse/che/ide/ui/window/Window.java

+        break;
+      case RIGHT:
+        button.addStyleName(resources.windowCss().buttonAlignRight());
+        break;


add default statement or replace with if..else

codenvy-ci · 2017-09-08T13:01:24Z

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

mmorhun added 10 commits August 4, 2017 15:27

CHE-5892: Temporary state

2318d28

CHE-5892: Use an object to hold git diff data

28ea7d4

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

CHE-5892: Draft solution.

5814596

Merge branch 'master' into CHE-5892

9f7d30c

CHE-5892: Fix build and tests

88bc819

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

CHE-5892: Fix inverted comparation type

a3e2c7c

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

CHE-5892: Fix comparation between revisions

e27895c

CHE-5892: Small refactor in ComparePresenter

72fb977

CHE-5892: Fix Save Changes button

347b2e0

CHE-5892: Remove unnecessary imports

638153b

mmorhun added the kind/enhancement A feature request - must adhere to the feature request template. label Aug 10, 2017

mmorhun requested review from dkuleshov, tolusha and vinokurig August 10, 2017 10:01

mmorhun requested a review from vparfonov as a code owner August 10, 2017 10:01

tolusha reviewed Aug 10, 2017

View reviewed changes

vinokurig reviewed Aug 10, 2017

View reviewed changes

tolusha reviewed Aug 10, 2017

View reviewed changes

vinokurig reviewed Aug 10, 2017

View reviewed changes

tolusha reviewed Aug 10, 2017

View reviewed changes

vinokurig reviewed Aug 10, 2017

View reviewed changes

mmorhun added 2 commits September 4, 2017 11:14

Merge branch 'master' into CHE-5892

c4c0cdf

Draft. Move git diff widget from iframe to IDE

bea914d

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun requested a review from azatsarynnyy as a code owner September 6, 2017 10:28

azatsarynnyy approved these changes Sep 6, 2017

View reviewed changes

CHE-5892: Fix build

21619ad

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Prevent jumping to non-existing diff with hotkeys

eaa74c5

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun added 2 commits September 7, 2017 10:57

CHE-5892: Temporary state. Moving git diff grom iframe to IDE

2a1147e

Merge branch 'master' into CHE-5892

7195d82

mmorhun requested a review from evidolob as a code owner September 7, 2017 07:59

CHE-5892: Move git diff widget from iframe to IDE

e2b5c84

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun mentioned this pull request Sep 7, 2017

CHE-5892: Move Git Diff widget from iframe to IDE eclipse-che/che-lib#38

Merged

vparfonov reviewed Sep 8, 2017

View reviewed changes

CHE-5892: code clean up

0bd2dd9

slemeur approved these changes Sep 12, 2017

View reviewed changes

mmorhun merged commit c7ed1d8 into master Sep 12, 2017

mmorhun deleted the CHE-5892 branch September 12, 2017 11:13

slemeur added this to the 5.18.0 milestone Sep 28, 2017


		changedFilesList = new ArrayList<>(changedFilesStatuses.keySet());

		length = changedFilesList.size();

Add ability to switch between files in Git Diff widget #5965

Add ability to switch between files in Git Diff widget #5965

Conversation

mmorhun commented Aug 10, 2017 • edited by slemeur Loading

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

Choose a reason for hiding this comment

mmorhun Aug 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolusha Aug 10, 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

mmorhun Aug 10, 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

codenvy-ci commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Aug 23, 2017

codenvy-ci commented Sep 6, 2017

codenvy-ci commented Sep 6, 2017

codenvy-ci commented Sep 6, 2017

codenvy-ci commented Sep 7, 2017

mmorhun commented Sep 7, 2017

mmorhun commented Sep 7, 2017 • edited Loading

slemeur commented Sep 7, 2017

codenvy-ci commented Sep 7, 2017

mmorhun commented Sep 7, 2017 • edited Loading

mmorhun commented Sep 7, 2017

slemeur commented Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

codenvy-ci commented Sep 8, 2017

mmorhun commented Aug 10, 2017 •

edited by slemeur

Loading

mmorhun Aug 10, 2017 •

edited

Loading

mmorhun Aug 10, 2017 •

edited

Loading

mmorhun Aug 10, 2017 •

edited

Loading

tolusha Aug 10, 2017 •

edited

Loading

mmorhun Aug 10, 2017 •

edited

Loading

mmorhun commented Sep 7, 2017 •

edited

Loading

mmorhun commented Sep 7, 2017 •

edited

Loading

slemeur commented Sep 8, 2017 •

edited

Loading