[log] IDEA-115480 Correctly identify details of selected rows

DetailsPanel, Actions:
Don't ask the Graph about nodes or details of the selected rows,
because in the case of non-graph filter the result will be incorrect.
Instead take the value from the table model.

Let the CommitCell hold the Hash value. In some cases it might be
null (for example, when there are "empty" lines in the table
containing only graph fragments).

Let DataGetter return value for the hash, not only for the node.
This can be slow potentially, will be optimized or moved to the
background later.

Let Copy Hash action operate on Hash instead of VcsFullCommitDetails.
Make Cherry-Pick and Open on GitHub be unavailable if details of the
selected hashes were not loaded yet.

platform/vcs-log/api/src/com/intellij/vcs/log/VcsLog.java

@@ -30,7 +30,16 @@ public interface VcsLog {
    * Returns commits currently selected in the log.
    */
   @NotNull
-  List<VcsFullCommitDetails> getSelectedCommits();
+  List<Hash> getSelectedCommits();
+
+  /**
+   * Returns details of the given commit, if they have been already loaded.
+   * In most cases they are already in the cache, and will be returned.
+   * Otherwise null is returned.
+   * Asynchronous loading of the details which are not yet available is done automatically from the log table component.
+   */
+  @Nullable
+  VcsFullCommitDetails getDetailsIfAvailable(@NotNull Hash hash);
 
   /**
    * Returns names of branches which contain the given commit, or null if this information is unavailable.

platform/vcs-log/graph/src/com/intellij/vcs/log/graph/render/CommitCell.java

@@ -1,6 +1,9 @@
 package com.intellij.vcs.log.graph.render;
 
+import com.intellij.vcs.log.Hash;
 import com.intellij.vcs.log.VcsRef;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
 import java.util.Collection;
 
@@ -11,8 +14,14 @@ public class CommitCell {
 
   private final String text;
   private final Collection<VcsRef> refsToThisCommit;
+  private Hash myHash;
 
-  public CommitCell(String text, Collection<VcsRef> refsToThisCommit) {
+  /**
+   * Hash can be null, if, for example, this is a cell which doesn't contain a commit, but contains only a part of the graph
+   * (such situations may appear, for example, if graph is filtered by branch, as described in IDEA-115442).
+   */
+  public CommitCell(@Nullable Hash hash, @NotNull String text, @NotNull Collection<VcsRef> refsToThisCommit) {
+    myHash = hash;
     this.text = text;
     this.refsToThisCommit = refsToThisCommit;
   }
@@ -25,4 +34,8 @@ public class CommitCell {
     return refsToThisCommit;
   }
 
+  @Nullable
+  public Hash getHash() {
+    return myHash;
+  }
 }

platform/vcs-log/graph/src/com/intellij/vcs/log/graph/render/GraphCommitCell.java

@@ -1,7 +1,10 @@
 package com.intellij.vcs.log.graph.render;
 
+import com.intellij.vcs.log.Hash;
 import com.intellij.vcs.log.VcsRef;
 import com.intellij.vcs.log.printmodel.GraphPrintCell;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
 import java.util.Collection;
 
@@ -13,8 +16,9 @@ public class GraphCommitCell extends CommitCell {
 
   private final GraphPrintCell row;
 
-  public GraphCommitCell(GraphPrintCell row, String text, Collection<VcsRef> refsToThisCommit) {
-    super(text, refsToThisCommit);
+  public GraphCommitCell(@Nullable Hash hash, @NotNull GraphPrintCell row, @NotNull String text,
+                         @NotNull Collection<VcsRef> refsToThisCommit) {
+    super(hash, text, refsToThisCommit);
     this.row = row;
   }
 

platform/vcs-log/impl/src/com/intellij/vcs/log/data/DataGetter.java

@@ -64,18 +64,43 @@ public abstract class DataGetter<T extends VcsShortCommitDetails> implements Dis
   public T getCommitData(@NotNull final Node node) {
     assert EventQueue.isDispatchThread();
     Hash hash = node.getCommitHash();
+    T details = getFromCache(hash);
+    if (details != null) {
+      return details;
+    }
+    return loadingDetails(node, hash);
+  }
+
+  @NotNull
+  private T loadingDetails(Node node, Hash hash) {
+    T loadingDetails = (T)new LoadingDetails(hash);
+    runLoadAroundCommitData(node);
+    return loadingDetails;
+  }
+
+  @NotNull
+  public T getCommitData(@NotNull Hash hash) {
+    assert EventQueue.isDispatchThread();
+    T details = getFromCache(hash);
+    if (details != null) {
+      return details;
+    }
+    Node node = myDataHolder.getDataPack().getNodeByHash(hash); // TODO this may possibly be slow => need to add to the Task as well
+    return loadingDetails(node, hash);
+  }
+
+  @Nullable
+  public T getCommitDataIfAvailable(@NotNull Hash hash) {
+    return getFromCache(hash);
+  }
+
+  @Nullable
+  private T getFromCache(@NotNull Hash hash) {
     T details = myCache.get(hash);
     if (details != null) {
       return details;
     }
-    details = (T)myDataHolder.getTopCommitDetails(hash);
-    if (details != null) {
-      return details;
-    }
-
-    T loadingDetails = (T)new LoadingDetails(hash);
-    runLoadAroundCommitData(node);
-    return loadingDetails;
+    return (T)myDataHolder.getTopCommitDetails(hash);
   }
 
   @Nullable

platform/vcs-log/impl/src/com/intellij/vcs/log/impl/VcsLogImpl.java

@@ -15,14 +15,15 @@
  */
 package com.intellij.vcs.log.impl;
 
+import com.intellij.ui.table.JBTable;
 import com.intellij.util.containers.ContainerUtil;
 import com.intellij.vcs.log.Hash;
 import com.intellij.vcs.log.VcsFullCommitDetails;
 import com.intellij.vcs.log.VcsLog;
-import com.intellij.vcs.log.data.LoadingDetails;
 import com.intellij.vcs.log.data.VcsLogDataHolder;
-import com.intellij.vcs.log.graph.elements.Node;
+import com.intellij.vcs.log.graph.render.CommitCell;
 import com.intellij.vcs.log.ui.VcsLogUI;
+import com.intellij.vcs.log.ui.tables.AbstractVcsLogTableModel;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -50,17 +51,25 @@ public class VcsLogImpl implements VcsLog {
     return myDataHolder != null && myUi != null;
   }
 
-  @NotNull
   @Override
-  public List<VcsFullCommitDetails> getSelectedCommits() {
-    List<VcsFullCommitDetails> selectedDetails = ContainerUtil.newArrayList();
-    for (int row : myUi.getTable().getSelectedRows()) {
-      VcsFullCommitDetails data = getDetailsAtRow(row);
-      if (data != null) {
-        selectedDetails.add(data);
+  @NotNull
+  public List<Hash> getSelectedCommits() {
+    List<Hash> hashes = ContainerUtil.newArrayList();
+    JBTable table = myUi.getTable();
+    for (int row : table.getSelectedRows()) {
+      CommitCell cell = (CommitCell)table.getModel().getValueAt(row, AbstractVcsLogTableModel.COMMIT_COLUMN);
+      Hash hash = cell.getHash();
+      if (hash != null) {
+        hashes.add(hash);
       }
     }
-    return selectedDetails;
+    return hashes;
+  }
+
+  @Override
+  @Nullable
+  public VcsFullCommitDetails getDetailsIfAvailable(@NotNull final Hash hash) {
+    return myDataHolder.getCommitDetailsGetter().getCommitDataIfAvailable(hash);
   }
 
   @Nullable
@@ -69,14 +78,4 @@ public class VcsLogImpl implements VcsLog {
     return null;
   }
 
-  @Nullable
-  private VcsFullCommitDetails getDetailsAtRow(int row) {
-    Node commitNode = myDataHolder.getDataPack().getGraphModel().getGraph().getCommitNodeInRow(row);
-    if (commitNode == null) {
-      return null;
-    }
-    VcsFullCommitDetails details = myDataHolder.getCommitDetailsGetter().getCommitData(commitNode);
-    return details instanceof LoadingDetails ? null : details;
-  }
-
 }

platform/vcs-log/impl/src/com/intellij/vcs/log/ui/VcsLogCopyHashAction.java

@@ -23,7 +23,7 @@ import com.intellij.openapi.project.DumbAwareAction;
 import com.intellij.openapi.project.Project;
 import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.util.Function;
-import com.intellij.vcs.log.VcsFullCommitDetails;
+import com.intellij.vcs.log.Hash;
 import com.intellij.vcs.log.VcsLog;
 import org.jetbrains.annotations.Nullable;
 
@@ -42,15 +42,15 @@ public class VcsLogCopyHashAction extends DumbAwareAction {
     if (log == null) {
       return;
     }
-    List<VcsFullCommitDetails> commits = log.getSelectedCommits();
+    List<Hash> commits = log.getSelectedCommits();
     if (commits.isEmpty()) {
       return;
     }
 
-    String hashes = StringUtil.join(commits, new Function<VcsFullCommitDetails, String>() {
+    String hashes = StringUtil.join(commits, new Function<Hash, String>() {
       @Override
-      public String fun(VcsFullCommitDetails details) {
-        return details.getHash().asString();
+      public String fun(Hash hash) {
+        return hash.asString();
       }
     }, "\n");
     CopyPasteManager.getInstance().setContents(new StringSelection(hashes));

platform/vcs-log/impl/src/com/intellij/vcs/log/ui/frame/DetailsPanel.java

@@ -15,10 +15,11 @@ import com.intellij.vcs.log.VcsFullCommitDetails;
 import com.intellij.vcs.log.VcsRef;
 import com.intellij.vcs.log.data.LoadingDetails;
 import com.intellij.vcs.log.data.VcsLogDataHolder;
-import com.intellij.vcs.log.graph.elements.Node;
-import com.intellij.vcs.log.ui.VcsLogColorManager;
+import com.intellij.vcs.log.graph.render.CommitCell;
 import com.intellij.vcs.log.graph.render.PrintParameters;
+import com.intellij.vcs.log.ui.VcsLogColorManager;
 import com.intellij.vcs.log.ui.render.RefPainter;
+import com.intellij.vcs.log.ui.tables.AbstractVcsLogTableModel;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
@@ -75,25 +76,21 @@ class DetailsPanel extends JPanel implements ListSelectionListener {
   public void valueChanged(@Nullable ListSelectionEvent notUsed) {
     int[] rows = myGraphTable.getSelectedRows();
     if (rows.length < 1) {
-      myLoadingPanel.stopLoading();
-      ((CardLayout)getLayout()).show(this, MESSAGE_LAYER);
-      myMessagePanel.setText("Nothing selected");
+      showMessage("Nothing selected");
     }
     else if (rows.length > 1) {
-      myLoadingPanel.stopLoading();
-      ((CardLayout)getLayout()).show(this, MESSAGE_LAYER);
-      myMessagePanel.setText("Several commits selected");
+      showMessage("Several commits selected");
     }
     else {
       ((CardLayout)getLayout()).show(this, STANDARD_LAYER);
-      Node node = myLogDataHolder.getDataPack().getNode(rows[0]);
-      if (node == null) {
-        LOG.info("Couldn't find node for row " + rows[0] +
-                 ". All nodes: " + myLogDataHolder.getDataPack().getGraphModel().getGraph().getNodeRows());
+      CommitCell cell = (CommitCell)myGraphTable.getModel().getValueAt(rows[0], AbstractVcsLogTableModel.COMMIT_COLUMN);
+      Hash hash = cell.getHash();
+      if (hash == null) {
+        showMessage("Nothing selected");
         return;
       }
-      Hash hash = node.getCommitHash();
-      VcsFullCommitDetails commitData = myLogDataHolder.getCommitDetailsGetter().getCommitData(node);
+
+      VcsFullCommitDetails commitData = myLogDataHolder.getCommitDetailsGetter().getCommitData(hash);
       if (commitData instanceof LoadingDetails) {
         myLoadingPanel.startLoading();
         myDataPanel.setData(null);
@@ -102,11 +99,17 @@ class DetailsPanel extends JPanel implements ListSelectionListener {
       else {
         myLoadingPanel.stopLoading();
         myDataPanel.setData(commitData);
-        myRefsPanel.setRefs(sortRefs(hash, node.getBranch().getRepositoryRoot()));
+        myRefsPanel.setRefs(sortRefs(hash, commitData.getRoot()));
       }
     }
   }
 
+  private void showMessage(String text) {
+    myLoadingPanel.stopLoading();
+    ((CardLayout)getLayout()).show(this, MESSAGE_LAYER);
+    myMessagePanel.setText(text);
+  }
+
   @NotNull
   private List<VcsRef> sortRefs(@NotNull Hash hash, @NotNull VirtualFile root) {
     Collection<VcsRef> refs = myLogDataHolder.getDataPack().getRefsModel().refsToCommit(hash);

platform/vcs-log/impl/src/com/intellij/vcs/log/ui/tables/GraphTableModel.java

@@ -5,6 +5,7 @@ import com.intellij.openapi.util.EmptyRunnable;
 import com.intellij.openapi.vcs.changes.Change;
 import com.intellij.openapi.vcs.changes.committed.CommittedChangesTreeBrowser;
 import com.intellij.openapi.vfs.VirtualFile;
+import com.intellij.vcs.log.Hash;
 import com.intellij.vcs.log.VcsFullCommitDetails;
 import com.intellij.vcs.log.VcsRef;
 import com.intellij.vcs.log.VcsShortCommitDetails;
@@ -114,11 +115,13 @@ public class GraphTableModel extends AbstractVcsLogTableModel<GraphCommitCell> {
     GraphPrintCell graphPrintCell = myDataPack.getPrintCellModel().getGraphPrintCell(rowIndex);
     String message = "";
     List<VcsRef> refs = Collections.emptyList();
+    Hash hash = null;
     if (details != null) {
+      hash = details.getHash();
       message = details.getSubject();
       refs = (List<VcsRef>)myDataPack.getRefsModel().refsToCommit(details.getHash());
     }
-    return new GraphCommitCell(graphPrintCell, message, refs);
+    return new GraphCommitCell(hash, graphPrintCell, message, refs);
   }
 
   @NotNull

platform/vcs-log/impl/src/com/intellij/vcs/log/ui/tables/NoGraphTableModel.java

@@ -96,7 +96,7 @@ public class NoGraphTableModel extends AbstractVcsLogTableModel<CommitCell> {
       subject = details.getSubject();
       refs = myRefsModel.refsToCommit(details.getHash());
     }
-    return new CommitCell(subject, refs);
+    return new CommitCell(myCommits.get(index).getHash(), subject, refs);
   }
 
   @NotNull

plugins/git4idea/src/git4idea/cherrypick/GitCherryPickAction.java

@@ -24,7 +24,6 @@ import com.intellij.openapi.progress.ProgressIndicator;
 import com.intellij.openapi.progress.Task;
 import com.intellij.openapi.project.DumbAwareAction;
 import com.intellij.openapi.project.Project;
-import com.intellij.openapi.util.Condition;
 import com.intellij.util.Function;
 import com.intellij.util.containers.ContainerUtil;
 import com.intellij.vcs.log.Hash;
@@ -175,20 +174,27 @@ public class GitCherryPickAction extends DumbAwareAction {
     if (project == null) {
       return null;
     }
-    VcsLog log = getVcsLog(project);
+    final VcsLog log = getVcsLog(project);
     if (log == null) {
       return null;
     }
-    List<VcsFullCommitDetails> selectedCommits = log.getSelectedCommits();
-    // don't allow to cherry-pick if a non-Git commit was selected
-    // we could cherry-pick just Git commits filtered from the list, but it might provide confusion
-    boolean nonGitCommitSelected = ContainerUtil.find(selectedCommits, new Condition<VcsFullCommitDetails>() {
-      @Override
-      public boolean value(VcsFullCommitDetails details) {
-        return myPlatformFacade.getRepositoryManager(project).getRepositoryForRoot(details.getRoot()) == null;
+
+    List<Hash> selectedCommits = log.getSelectedCommits();
+    List<VcsFullCommitDetails> selectedDetails = ContainerUtil.newArrayList();
+    for (Hash commit : selectedCommits) {
+      VcsFullCommitDetails details = log.getDetailsIfAvailable(commit);
+      if (details == null) { // let the action be unavailable until all details are loaded
+        return null;
       }
-    }) != null;
-    return nonGitCommitSelected ? null : selectedCommits;
+      GitRepository root = myPlatformFacade.getRepositoryManager(project).getRepositoryForRoot(details.getRoot());
+      // don't allow to cherry-pick if a non-Git commit was selected
+      // we could cherry-pick just Git commits filtered from the list, but it might provide confusion
+      if (root == null) {
+        return null;
+      }
+      selectedDetails.add(details);
+    }
+    return selectedDetails;
   }
 
   private static List<? extends VcsFullCommitDetails> convertHeavyCommitToFullDetails(List<GitHeavyCommit> commits) {

plugins/github/src/org/jetbrains/plugins/github/GithubShowCommitInBrowserFromLogAction.java

@@ -22,7 +22,11 @@ import com.intellij.openapi.project.Project;
 import com.intellij.openapi.vfs.VirtualFile;
 import com.intellij.util.Function;
 import com.intellij.util.containers.ContainerUtil;
-import com.intellij.vcs.log.*;
+import com.intellij.vcs.log.Hash;
+import com.intellij.vcs.log.VcsLog;
+import com.intellij.vcs.log.VcsLogObjectsFactory;
+import com.intellij.vcs.log.VcsShortCommitDetails;
+import com.intellij.vcs.log.impl.VcsLogImpl;
 import git4idea.GitUtil;
 import git4idea.GitVcs;
 import git4idea.history.browser.GitHeavyCommit;
@@ -80,9 +84,13 @@ public class GithubShowCommitInBrowserFromLogAction extends GithubShowCommitInBr
       return factory.createShortDetails(factory.createHash(heavyCommit.getHash().getValue()), parents, heavyCommit.getAuthorTime(),
                                         heavyCommit.getRoot(), heavyCommit.getSubject(), heavyCommit.getAuthor());
     }
-    List<VcsFullCommitDetails> selectedCommits = ServiceManager.getService(e.getProject(), VcsLog.class).getSelectedCommits();
+    VcsLog log = ServiceManager.getService(e.getProject(), VcsLog.class);
+    if (log == null || !((VcsLogImpl)log).isReady()) {
+      return null;
+    }
+    List<Hash> selectedCommits = log.getSelectedCommits();
     if (selectedCommits.size() == 1) {
-      return selectedCommits.get(0);
+      return log.getDetailsIfAvailable(selectedCommits.get(0));
     }
     return null;
   }