Revert "Make DifferentialChangesetParser explicitly map display to storage for comments"

+26 -137

2 changed files

expand all

src

applications

differential

controller

changesetview

DifferentialChangesetViewController.php

parser

changeset

DifferentialChangesetParser.php

+4 -13

src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php

··· 154 154 $parser->setLeftSideCommentMapping($left_source, $left_new); 155 155 $parser->setWhitespaceMode($request->getStr('whitespace')); 156 156 157 - // Load both left-side and right-side inline comments. 158 - $inlines = $this->loadInlineComments( 159 - array($left_source, $right_source), 160 - $author_phid); 161 - 162 157 $phids = array(); 158 + $inlines = $this->loadInlineComments($id, $author_phid); 163 159 foreach ($inlines as $inline) { 164 160 $parser->parseInlineComment($inline); 165 161 $phids[$inline->getAuthorPHID()] = true; ··· 232 228 )); 233 229 } 234 230 235 - private function loadInlineComments(array $changeset_ids, $author_phid) { 236 - $changeset_ids = array_unique(array_filter($changeset_ids)); 237 - if (!$changeset_ids) { 238 - return; 239 - } 240 - 231 + private function loadInlineComments($changeset_id, $author_phid) { 241 232 return id(new DifferentialInlineComment())->loadAllWhere( 242 - 'changesetID IN (%Ld) AND (commentID IS NOT NULL OR authorPHID = %s)', 243 - $changeset_ids, 233 + 'changesetID = %d AND (commentID IS NOT NULL OR authorPHID = %s)', 234 + $changeset_id, 244 235 $author_phid); 245 236 } 246 237

+22 -124

src/applications/differential/parser/changeset/DifferentialChangesetParser.php

··· 39 39 protected $whitespaceMode = null; 40 40 41 41 protected $subparser; 42 + protected $oldChangesetID = null; 42 43 protected $noHighlight; 43 44 44 45 protected $renderCacheKey = null; ··· 46 47 private $handles; 47 48 private $user; 48 49 49 - private $leftSideChangesetID; 50 - private $leftSideAttachesToNewFile; 51 - 52 - private $rightSideChangesetID; 53 - private $rightSideAttachesToNewFile; 54 - 55 50 const CACHE_VERSION = 4; 56 51 57 52 const ATTR_GENERATED = 'attr:generated'; ··· 65 60 const WHITESPACE_IGNORE_TRAILING = 'ignore-trailing'; 66 61 const WHITESPACE_IGNORE_ALL = 'ignore-all'; 67 62 68 - /** 69 - * Configure which Changeset comments added to the right side of the visible 70 - * diff will be attached to. The ID must be the ID of a real Differential 71 - * Changeset. 72 - * 73 - * The complexity here is that we may show an arbitrary side of an arbitrary 74 - * changeset as either the left or right part of a diff. This method allows 75 - * the left and right halves of the displayed diff to be correctly mapped to 76 - * storage changesets. 77 - * 78 - * @param id The Differential Changeset ID that comments added to the right 79 - * side of the visible diff should be attached to. 80 - * @param bool If true, attach new comments to the right side of the storage 81 - * changeset. Note that this may be false, if the left side of 82 - * some storage changeset is being shown as the right side of 83 - * a display diff. 84 - * @return this 85 - */ 86 63 public function setRightSideCommentMapping($id, $is_new) { 87 - $this->rightSideChangesetID = $id; 88 - $this->rightSideAttachesToNewFile = $is_new; 89 - return $this; 64 + 90 65 } 91 66 92 - /** 93 - * See setRightSideCommentMapping(), but this sets information for the left 94 - * side of the display diff. 95 - */ 96 67 public function setLeftSideCommentMapping($id, $is_new) { 97 - $this->leftSideChangesetID = $id; 98 - $this->leftSideAttachesToNewFile = $is_new; 99 - return $this; 68 + 100 69 } 101 70 102 71 /** ··· 132 101 133 102 public function setWhitespaceMode($whitespace_mode) { 134 103 $this->whitespaceMode = $whitespace_mode; 104 + return $this; 105 + } 106 + 107 + public function setOldChangesetID($old_changeset_id) { 108 + $this->oldChangesetID = $old_changeset_id; 135 109 return $this; 136 110 } 137 111 ··· 262 236 } 263 237 264 238 public function parseInlineComment(DifferentialInlineComment $comment) { 265 - // Parse only comments which are actually visible. 266 - if ($this->isCommentVisibleOnRenderedDiff($comment)) { 267 - $this->comments[] = $comment; 268 - } 239 + $this->comments[] = $comment; 269 240 return $this; 270 241 } 271 242 ··· 866 837 $end = $comment->getLineNumber() + 867 838 $comment->getLineLength() + 868 839 self::LINES_CONTEXT; 869 - $new = $this->isCommentOnRightSideWhenDisplayed($comment); 840 + $new = $this->isCommentInNewFile($comment); 870 841 for ($ii = $start; $ii <= $end; $ii++) { 871 842 if ($new) { 872 843 $new_mask[$ii] = true; ··· 891 862 foreach ($this->comments as $comment) { 892 863 $final = $comment->getLineNumber() + 893 864 $comment->getLineLength(); 894 - if ($this->isCommentOnRightSideWhenDisplayed($comment)) { 865 + if ($this->isCommentInNewFile($comment)) { 895 866 $new_comments[$final][] = $comment; 896 867 } else { 897 868 $old_comments[$final][] = $comment; ··· 910 881 return $this->renderChangesetTable($this->changeset, $html); 911 882 } 912 883 913 - /** 914 - * Determine if an inline comment will appear on the rendered diff, 915 - * taking into consideration which halves of which changesets will actually 916 - * be shown. 917 - * 918 - * @param DifferentialInlineComment Comment to test for visibility. 919 - * @return bool True if the comment is visible on the rendered diff. 920 - */ 921 - private function isCommentVisibleOnRenderedDiff( 922 - DifferentialInlineComment $comment) { 923 - 924 - $changeset_id = $comment->getChangesetID(); 925 - $is_new = $comment->getIsNewFile(); 926 - 927 - if ($changeset_id == $this->rightSideChangesetID && 928 - $is_new == $this->rightSideAttachesToNewFile) { 929 - return true; 930 - } 931 - 932 - if ($changeset_id == $this->leftSideChangesetID && 933 - $is_new == $this->leftSideAttachesToNewFile) { 934 - return true; 935 - } 936 - 937 - return false; 938 - } 939 - 940 - 941 - /** 942 - * Determine if a comment will appear on the right side of the display diff. 943 - * Note that the comment must appear somewhere on the rendered changeset, as 944 - * per isCommentVisibleOnRenderedDiff(). 945 - * 946 - * @param DifferentialInlineComment Comment to test for display location. 947 - * @return bool True for right, false for left. 948 - */ 949 - private function isCommentOnRightSideWhenDisplayed( 950 - DifferentialInlineComment $comment) { 951 - 952 - if (!$this->isCommentVisibleOnRenderedDiff($comment)) { 953 - throw new Exception("Comment is not visible on changeset!"); 884 + private function isCommentInNewFile(DifferentialInlineComment $comment) { 885 + if ($this->oldChangesetID) { 886 + return ($comment->getChangesetID() != $this->oldChangesetID); 887 + } else { 888 + return $comment->getIsNewFile(); 954 889 } 955 - 956 - $changeset_id = $comment->getChangesetID(); 957 - $is_new = $comment->getIsNewFile(); 958 - 959 - if ($changeset_id == $this->rightSideChangesetID && 960 - $is_new == $this->rightSideAttachesToNewFile) { 961 - return true; 962 - } 963 - 964 - return false; 965 890 } 966 891 967 892 protected function renderShield($message, $more) { ··· 1036 961 1037 962 $range_len = min($range_len, $rows - $range_start); 1038 963 1039 - // Gaps - compute gaps in the visible display diff, where we will render 1040 - // "Show more context" spacers. This builds an aggregate $mask of all the 1041 - // lines we must show (because they are near changed lines, near inline 1042 - // comments, or the request has explicitly asked for them, i.e. resulting 1043 - // from the user clicking "show more") and then finds all the gaps between 1044 - // visible lines. If a gap is smaller than the context size, we just 1045 - // display it. Otherwise, we record it into $gaps and will render a 1046 - // "show more context" element instead of diff text below. 1047 - 1048 964 $gaps = array(); 1049 965 $gap_start = 0; 1050 966 $in_gap = false; ··· 1073 989 1074 990 $gaps = array_reverse($gaps); 1075 991 992 + $changeset = $this->changesetID; 1076 993 $reference = $this->getChangeset()->getRenderingReference(); 1077 994 1078 - $left_id = $this->leftSideChangesetID; 1079 - $right_id = $this->rightSideChangesetID; 1080 - 1081 - // "N" stands for 'new' and means the comment should attach to the new file 1082 - // when stored, i.e. DifferentialInlineComment->setIsNewFile(). 1083 - // "O" stands for 'old' and means the comment should attach to the old file. 1084 - 1085 - $left_char = $this->leftSideAttachesToNewFile 1086 - ? 'N' 1087 - : 'O'; 1088 - $right_char = $this->rightSideAttachesToNewFile 1089 - ? 'N' 1090 - : 'O'; 1091 - 1092 995 for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) { 1093 996 if (empty($mask[$ii])) { 1094 - // If we aren't going to show this line, we've just entered a gap. 1095 - // Pop information about the next gap off the $gaps stack and render 1096 - // an appropriate "Show more context" element. This branch eventually 1097 - // increments $ii by the entire size of the gap and then continues 1098 - // the loop. 1099 997 $gap = array_pop($gaps); 1100 998 $top = $gap[0]; 1101 999 $len = $gap[1]; ··· 1202 1100 $html[] = $context_not_available; 1203 1101 } 1204 1102 1205 - if ($o_num && $left_id) { 1206 - $o_id = ' id="C'.$left_id.$left_char.'L'.$o_num.'"'; 1103 + if ($o_num && $changeset) { 1104 + $o_id = ' id="C'.$changeset.'OL'.$o_num.'"'; 1207 1105 } else { 1208 1106 $o_id = null; 1209 1107 } 1210 1108 1211 - if ($n_num && $right_id) { 1212 - $n_id = ' id="C'.$right_id.$right_char.'L'.$n_num.'"'; 1109 + if ($n_num && $changeset) { 1110 + $n_id = ' id="C'.$changeset.'NL'.$n_num.'"'; 1213 1111 } else { 1214 1112 $n_id = null; 1215 1113 } ··· 1258 1156 ($comment->getAuthorPHID() == $user->getPHID()) && 1259 1157 (!$comment->getCommentID()); 1260 1158 1261 - $on_right = $this->isCommentOnRightSideWhenDisplayed($comment); 1159 + $on_right = $this->isCommentInNewFile($comment); 1262 1160 1263 1161 return id(new DifferentialInlineCommentView()) 1264 1162 ->setInlineComment($comment)

Configure Feed

Configure Feed