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

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

Merged
merged 40 commits into from
Sep 12, 2017
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
2318d28
CHE-5892: Temporary state
mmorhun Aug 4, 2017
28ea7d4
CHE-5892: Use an object to hold git diff data
mmorhun Aug 7, 2017
5814596
CHE-5892: Draft solution.
mmorhun Aug 9, 2017
9f7d30c
Merge branch 'master' into CHE-5892
mmorhun Aug 9, 2017
88bc819
CHE-5892: Fix build and tests
mmorhun Aug 9, 2017
a3e2c7c
CHE-5892: Fix inverted comparation type
mmorhun Aug 9, 2017
e27895c
CHE-5892: Fix comparation between revisions
mmorhun Aug 10, 2017
72fb977
CHE-5892: Small refactor in ComparePresenter
mmorhun Aug 10, 2017
347b2e0
CHE-5892: Fix Save Changes button
mmorhun Aug 10, 2017
638153b
CHE-5892: Remove unnecessary imports
mmorhun Aug 10, 2017
9f18acd
CHE-5892: Small fixes
mmorhun Aug 10, 2017
3cd740e
CHE-5892: Rename ChangedItems
mmorhun Aug 11, 2017
f22b649
CHE-5892: Fix compare with deleted file bug.
mmorhun Aug 11, 2017
2f82bec
License format
mmorhun Aug 14, 2017
d148050
License Format
mmorhun Aug 14, 2017
5d8744a
CHE-5892: Add AlteredFiles test
mmorhun Aug 14, 2017
6f3ea37
CHE-5892: Code cleanup
mmorhun Aug 14, 2017
241dca7
CHE-5892: Add additional check in AlteredFiles constructor
mmorhun Aug 14, 2017
a1eac87
Merge branch 'master' into CHE-5892
mmorhun Aug 14, 2017
18ed447
Merge branch 'master' into CHE-5892
mmorhun Aug 14, 2017
5a6d745
CHE-5892: Fix constraint mistake
mmorhun Aug 14, 2017
e1bb8f4
CHE-5892: Generalize showCompareForCurrentFile in ComparePresenter. B…
mmorhun Aug 15, 2017
c50272a
CHE-5892: Code cleanup
mmorhun Aug 15, 2017
69715a6
CHE-5892: Code cleanup
mmorhun Aug 15, 2017
96fed94
CHE-5892: Add missing new line
mmorhun Aug 16, 2017
436dc59
CHE-5892: Code cleanup
mmorhun Aug 16, 2017
286339c
Apply new code style
mmorhun Aug 22, 2017
e5208f5
Merge master
mmorhun Aug 22, 2017
d121a3d
Fix build
mmorhun Aug 22, 2017
cd6f922
Add ability to align a button left
mmorhun Aug 23, 2017
89ad74e
CHE-5892: Add current diff number in the widget title
mmorhun Aug 23, 2017
65204e0
CHE-5892: Fix formatting
mmorhun Aug 23, 2017
c4c0cdf
Merge branch 'master' into CHE-5892
mmorhun Sep 4, 2017
bea914d
Draft. Move git diff widget from iframe to IDE
mmorhun Sep 6, 2017
21619ad
CHE-5892: Fix build
mmorhun Sep 6, 2017
eaa74c5
Prevent jumping to non-existing diff with hotkeys
mmorhun Sep 6, 2017
2a1147e
CHE-5892: Temporary state. Moving git diff grom iframe to IDE
mmorhun Sep 7, 2017
7195d82
Merge branch 'master' into CHE-5892
mmorhun Sep 7, 2017
e2b5c84
CHE-5892: Move git diff widget from iframe to IDE
mmorhun Sep 7, 2017
0bd2dd9
CHE-5892: code clean up
mmorhun Sep 8, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,25 @@ public interface GitLocalizationConstant extends Messages {
@Key("button.compare")
String buttonCompare();

@Key("button.save_changes")
String buttonSaveChanges();

@Key("button.next_diff")
String buttonNextDiff();

@Key("button.previous_diff")
String buttonPreviousDiff();

@Key("console.tooltip.clear")
String buttonClear();

@Key("console.tooltip.scroll")
String buttonScroll();

// MESSAGES
@Key("messages.file_is_not_under_git")
String messageFileIsNotUnderGit(String file);

@Key("messages.unableGetSshKey")
String messagesUnableGetSshKey();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,16 @@
import org.eclipse.che.ide.api.resources.Project;
import org.eclipse.che.ide.api.resources.Resource;
import org.eclipse.che.ide.ext.git.client.GitLocalizationConstant;
import org.eclipse.che.ide.ext.git.client.compare.AlteredFiles;
import org.eclipse.che.ide.ext.git.client.compare.ComparePresenter;
import org.eclipse.che.ide.ext.git.client.compare.FileStatus.Status;
import org.eclipse.che.ide.ext.git.client.compare.changeslist.ChangesListPresenter;
import org.eclipse.che.ide.api.dialogs.DialogFactory;

import java.util.HashMap;
import java.util.Map;

import static com.google.common.base.Preconditions.checkState;
import static java.util.Collections.singletonList;
import static org.eclipse.che.api.git.shared.DiffType.NAME_STATUS;
import static org.eclipse.che.ide.api.notification.StatusNotification.DisplayMode.NOT_EMERGE_MODE;
import static org.eclipse.che.ide.api.notification.StatusNotification.Status.FAIL;
import static org.eclipse.che.ide.ext.git.client.compare.FileStatus.defineStatus;

/**
* Action for comparing with latest repository version
Expand Down Expand Up @@ -91,21 +87,11 @@ public void actionPerformed(ActionEvent e) {
dialogFactory.createMessageDialog(locale.compareMessageIdenticalContentTitle(),
locale.compareMessageIdenticalContentText(), null).show();
} else {
final String[] changedFiles = diff.split("\n");
if (changedFiles.length == 1) {
project.getFile(changedFiles[0].substring(2)).then(file -> {
if (file.isPresent()) {
comparePresenter.showCompareWithLatest(file.get(),
defineStatus(changedFiles[0].substring(0, 1)),
REVISION);
}
});
AlteredFiles alteredFiles = new AlteredFiles(project, diff);
if (alteredFiles.getFilesQuantity() == 1) {
comparePresenter.showCompareWithLatest(alteredFiles, null, REVISION);
} else {
Map<String, Status> items = new HashMap<>();
for (String item : changedFiles) {
items.put(item.substring(2, item.length()), defineStatus(item.substring(0, 1)));
}
changesListPresenter.show(items, REVISION, null, project);
changesListPresenter.show(alteredFiles, REVISION, null);
}
}
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import org.eclipse.che.ide.commons.exception.ServerException;
import org.eclipse.che.ide.ext.git.client.DateTimeFormatter;
import org.eclipse.che.ide.ext.git.client.GitLocalizationConstant;
import org.eclipse.che.ide.ext.git.client.compare.FileStatus.Status;
import org.eclipse.che.ide.ext.git.client.compare.AlteredFiles;
import org.eclipse.che.ide.ext.git.client.outputconsole.GitOutputConsole;
import org.eclipse.che.ide.ext.git.client.outputconsole.GitOutputConsoleFactory;
import org.eclipse.che.ide.ext.git.client.compare.changespanel.ChangesPanelPresenter;
Expand All @@ -33,13 +33,9 @@

import javax.validation.constraints.NotNull;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.collect.Iterables.getFirst;
import static java.util.Arrays.stream;
import static java.util.Collections.singletonList;
Expand All @@ -50,7 +46,6 @@
import static org.eclipse.che.ide.api.notification.StatusNotification.DisplayMode.NOT_EMERGE_MODE;
import static org.eclipse.che.ide.api.notification.StatusNotification.Status.FAIL;
import static org.eclipse.che.ide.api.notification.StatusNotification.Status.SUCCESS;
import static org.eclipse.che.ide.ext.git.client.compare.FileStatus.defineStatus;
import static org.eclipse.che.ide.util.ExceptionUtils.getErrorCode;

/**
Expand All @@ -76,7 +71,7 @@ public class CommitPresenter implements CommitView.ActionDelegate {
private final ProcessesPanelPresenter consolesPanelPresenter;

private Project project;
private Set<String> allFiles;
private List<String> allFiles;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain why we're changing a set for a list? We need to store same files several times?

Copy link
Contributor Author

@mmorhun mmorhun Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we needn't, but git returns modified files in some order, so I just do not want to loose it. Also it is needed to navigate through this list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be
private AlteredFiles alteredFiles; ?

private List<String> filesToCommit;

@Inject
Expand Down Expand Up @@ -133,7 +128,7 @@ public void showDialog(Project project) {
List<String> newFiles = new ArrayList<>();
newFiles.addAll(status.getAdded());
newFiles.addAll(status.getUntracked());
show(newFiles.stream().collect(joining("\nA ", "A ", "")));
show(newFiles.stream().collect(joining("\nA\t", "A\t", "")));
});
}
});
Expand Down Expand Up @@ -165,26 +160,18 @@ private void showAskForAmendDialog() {
}

private void show(@Nullable String diff) {
Map<String, Status> files = toFileStatusMap(diff);
AlteredFiles alteredFiles = new AlteredFiles(project, diff);
filesToCommit.clear();
allFiles = files.keySet();
allFiles = alteredFiles.getAlteredFilesList();

view.setEnableCommitButton(!view.getMessage().isEmpty());
view.focusInMessageField();
view.showDialog();
changesPanelPresenter.show(files);
changesPanelPresenter.show(alteredFiles);
view.setMarkedCheckBoxes(stream(appContext.getResources()).map(resource -> resource.getLocation().removeFirstSegments(1))
.collect(Collectors.toSet()));
}

private Map<String, Status> toFileStatusMap(@Nullable String diff) {
Map<String, Status> items = new HashMap<>();
if (!isNullOrEmpty(diff)) {
stream(diff.split("\n")).forEach(item -> items.put(item.substring(2, item.length()), defineStatus(item.substring(0, 1))));
}
return items;
}

@Override
public void onCommitClicked() {
Path location = project.getLocation();
Expand Down Expand Up @@ -279,7 +266,7 @@ public void onFileNodeCheckBoxValueChanged(Path path, boolean newCheckBoxValue)
}

@Override
public Set<String> getChangedFiles() {
public List<String> getChangedFiles() {
return allFiles;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ interface ActionDelegate {
void setAmendCommitMessage();

/** Get list of changed files paths. */
Set<String> getChangedFiles();
List<String> getChangedFiles();
}

/** @return entered message */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*******************************************************************************
* Copyright (c) 2012-2017 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*******************************************************************************/
package org.eclipse.che.ide.ext.git.client.compare;

import org.eclipse.che.ide.api.resources.Project;
import org.eclipse.che.ide.ext.git.client.compare.FileStatus.Status;
import org.eclipse.che.ide.util.loging.Log;

import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;

import static com.google.common.base.Strings.isNullOrEmpty;
import static org.eclipse.che.ide.ext.git.client.compare.FileStatus.defineStatus;

/**
* Describes changed in any way files in git comparison process.
*
* @author Mykola Morhun
*/
public class AlteredFiles {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see some tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AlteredFilesHolder ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think the current name is better because this class also parses raw diff.


private final Project project;
private final LinkedHashMap<String, Status> alteredFilesStatuses;
private final List<String> alteredFilesList;

/**
* Parses raw git diff string and creates advanced representation.
*
* @param project
* the project under diff operation
* @param diff
* plain result of git diff operation
*/
public AlteredFiles(Project project, String diff) {
this.project = project;

alteredFilesStatuses = new LinkedHashMap<>();
if (!isNullOrEmpty(diff)) {
for (String item : diff.split("\n")) {
if (item.length() < 3 || item.charAt(1) != '\t') {
throw new IllegalArgumentException("Invalid git diff format. Invalid record: " + item);
}
alteredFilesStatuses.put(item.substring(2, item.length()), defineStatus(item.substring(0, 1)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validate the length of the item.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but usually we obtain raw diff from git, so it should be valid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trust no one!

}
}

alteredFilesList = new ArrayList<>(alteredFilesStatuses.keySet());
}

/**
* Returns project in which git repository is located.
*/
public Project getProject() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc?

return project;
}

/**
* Returns number of files in the diff.
*/
public int getFilesQuantity() {
return alteredFilesList.size();
}

public boolean isEmpty() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need javadoc for obviously methods?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? if you added a javadoc for one method why don't you add it for others

return 0 == alteredFilesList.size();
}

/**
* Returns this diff in map representation: altered file to its git status.
*/
public Map<String, Status> getChangedFilesMap() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc?

return alteredFilesStatuses;
}

/**
* Returns list of altered files in this git diff.
*/
public List<String> getAlteredFilesList() {
return alteredFilesList;
}

public Status getStatusByFilePath(String relativePathToChangedFile) {
return alteredFilesStatuses.get(relativePathToChangedFile);
}

public Status getStatusByIndex(int index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it is already clear form the method name.

return alteredFilesStatuses.get(alteredFilesList.get(index));
}

public String getFileByIndex(int index) {
return alteredFilesList.get(index);
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
AlteredFiles that = (AlteredFiles)o;
return Objects.equals(project, that.project) &&
Objects.equals(alteredFilesStatuses, that.alteredFilesStatuses) &&
Objects.equals(alteredFilesList, that.alteredFilesList);
}

@Override
public int hashCode() {
return Objects.hash(project, alteredFilesStatuses, alteredFilesList);
}

}
Loading