Add ability to switch between files in Git Diff widget #5965
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2012-2017 Codenvy, S.A. | ||
| * 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: | ||
| * Codenvy, S.A. - 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 java.util.ArrayList; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
| import static org.eclipse.che.ide.ext.git.client.compare.FileStatus.defineStatus; | ||
|
|
||
| /** | ||
| * Describes changed files in git comparison process. | ||
| * | ||
| * @author Mykola Morhun | ||
| */ | ||
| public class ChangedItems { | ||
|
There was a problem hiding this comment. What if it is added or deleted? There was a problem hiding this comment. 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 There was a problem hiding this comment. I think There was a problem hiding this comment. 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 There was a problem hiding this comment.
|
||
|
|
||
| private final Project project; | ||
| private final LinkedHashMap<String, Status> changedFilesStatuses; | ||
| private final List<String> changedFilesList; | ||
| private final int length; | ||
|
|
||
| /** | ||
| * Creates user-friendly representation of git diff. | ||
|
There was a problem hiding this comment. What do you mean user-frendly? How does it related to user? There was a problem hiding this comment. well, programmer friendly) |
||
| * | ||
| * @param project | ||
| * the project under diff operation | ||
| * @param diff | ||
| * plain result of git diff operation | ||
| */ | ||
| public ChangedItems(Project project, String diff) { | ||
| this.project = project; | ||
|
|
||
| changedFilesStatuses = new LinkedHashMap<>(); | ||
| for (String item : diff.split("\n")) { | ||
| changedFilesStatuses.put(item.substring(2, item.length()), defineStatus(item.substring(0, 1))); | ||
|
There was a problem hiding this comment. if There was a problem hiding this comment. Make sure There was a problem hiding this comment. To have clear code extract variables for There was a problem hiding this comment. No, it will end up with empty diff |
||
| } | ||
|
|
||
| changedFilesList = new ArrayList<>(changedFilesStatuses.keySet()); | ||
|
|
||
| length = changedFilesList.size(); | ||
|
There was a problem hiding this comment. Just cached. But ok, I'll get rid of it. |
||
| } | ||
|
|
||
| public Project getProject() { | ||
| return project; | ||
| } | ||
|
|
||
| /** | ||
| * @return number of files in the diff | ||
|
There was a problem hiding this comment. I prefer not to use single |
||
| */ | ||
| public int getFilesQuantity() { | ||
| return length; | ||
| } | ||
|
|
||
| public boolean isEmpty() { | ||
| return 0 == length; | ||
| } | ||
|
|
||
| public Map<String, Status> getChangedItemsMap() { | ||
| return changedFilesStatuses; | ||
| } | ||
|
|
||
| public List<String> getChangedItemsList() { | ||
| return changedFilesList; | ||
| } | ||
|
|
||
| public Status getStatusByPath(String pathToChangedItem) { | ||
| return changedFilesStatuses.get(pathToChangedItem); | ||
| } | ||
|
|
||
| public Status getStatusByIndex(int index) { | ||
| return changedFilesStatuses.get(changedFilesList.get(index)); | ||
| } | ||
|
|
||
| public String getItemByIndex(int index) { | ||
| return changedFilesList.get(index); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| ChangedItems that = (ChangedItems)o; | ||
| return Objects.equals(project, that.project) && | ||
| Objects.equals(changedFilesStatuses, that.changedFilesStatuses) && | ||
| Objects.equals(changedFilesList, that.changedFilesList); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(project, changedFilesStatuses, changedFilesList); | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't like methods which can accept null values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't like it too, but here I see no problem. It is well documented and mean undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
nullvalues (even well documented). I think it would be nice if you could add a corresponding method so we could callchangesListPresenter.show(changedItems, REVISION)and it would internally recallchangesListPresenter.show(changedItems, REVISION, null)or something similar to this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.