diff --git a/externals/phpmailer/class.phpmailer.php b/externals/phpmailer/class.phpmailer.php index 5ddbddc144..69f9c45ba5 100644 --- a/externals/phpmailer/class.phpmailer.php +++ b/externals/phpmailer/class.phpmailer.php @@ -1110,8 +1110,6 @@ class PHPMailer { if($this->MessageID != '') { $result .= $this->HeaderLine('Message-ID',$this->MessageID); - } else { - $result .= sprintf("Message-ID: <%s@%s>%s", $uniq_id, $this->ServerHostname(), $this->LE); } $result .= $this->HeaderLine('X-Priority', $this->Priority); $result .= $this->HeaderLine('X-Mailer', 'PHPMailer '.$this->Version.' (phpmailer.sourceforge.net)'); diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 53cf215ae8..1d98650a7e 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,8 +12,8 @@ return array( 'core.pkg.css' => 'ba768cdb', 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', - 'differential.pkg.css' => '42a2334f', - 'differential.pkg.js' => '8f59bce2', + 'differential.pkg.css' => '5c459f92', + 'differential.pkg.js' => '2b4a7014', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -65,7 +65,7 @@ return array( 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', 'rsrc/css/application/differential/changeset-view.css' => '60c3d405', 'rsrc/css/application/differential/core.css' => '7300a73e', - 'rsrc/css/application/differential/phui-inline-comment.css' => 'd5749acc', + 'rsrc/css/application/differential/phui-inline-comment.css' => '9863a85e', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', 'rsrc/css/application/differential/revision-history.css' => '8aa3eac5', 'rsrc/css/application/differential/revision-list.css' => '93d2df7d', @@ -379,14 +379,15 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', - 'rsrc/js/application/diff/DiffChangeset.js' => '0c083409', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'db615898', - 'rsrc/js/application/diff/DiffInline.js' => 'b00168c1', + 'rsrc/js/application/diff/DiffChangeset.js' => '39dcf2c3', + 'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5', + 'rsrc/js/application/diff/DiffInline.js' => '008b6a15', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', 'rsrc/js/application/differential/behavior-populate.js' => 'b86ef6c2', 'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => '94243d89', + 'rsrc/js/application/diffusion/ExternalEditorLinkEngine.js' => '48a8641f', 'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'b7b73831', 'rsrc/js/application/diffusion/behavior-commit-branches.js' => '4b671572', 'rsrc/js/application/diffusion/behavior-commit-graph.js' => 'ef836bf2', @@ -484,7 +485,7 @@ return array( 'rsrc/js/core/behavior-keyboard-pager.js' => '1325b731', 'rsrc/js/core/behavior-keyboard-shortcuts.js' => '42c44e8b', 'rsrc/js/core/behavior-lightbox-attachments.js' => 'c7e748bf', - 'rsrc/js/core/behavior-line-linker.js' => '590e6527', + 'rsrc/js/core/behavior-line-linker.js' => '0d915ff5', 'rsrc/js/core/behavior-linked-container.js' => '74446546', 'rsrc/js/core/behavior-more.js' => '506aa3f4', 'rsrc/js/core/behavior-object-selector.js' => '98ef467f', @@ -645,7 +646,7 @@ return array( 'javelin-behavior-phabricator-gesture-example' => '242dedd0', 'javelin-behavior-phabricator-keyboard-pager' => '1325b731', 'javelin-behavior-phabricator-keyboard-shortcuts' => '42c44e8b', - 'javelin-behavior-phabricator-line-linker' => '590e6527', + 'javelin-behavior-phabricator-line-linker' => '0d915ff5', 'javelin-behavior-phabricator-notification-example' => '29819b75', 'javelin-behavior-phabricator-object-selector' => '98ef467f', 'javelin-behavior-phabricator-oncopy' => 'da8f5259', @@ -709,6 +710,7 @@ return array( 'javelin-dom' => '94681e22', 'javelin-dynval' => '202a2e85', 'javelin-event' => 'c03f2fb4', + 'javelin-external-editor-link-engine' => '48a8641f', 'javelin-fx' => '34450586', 'javelin-history' => '030b4f7a', 'javelin-install' => '5902260c', @@ -774,9 +776,9 @@ return array( 'phabricator-darklog' => '3b869402', 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', - 'phabricator-diff-changeset' => '0c083409', - 'phabricator-diff-changeset-list' => 'db615898', - 'phabricator-diff-inline' => 'b00168c1', + 'phabricator-diff-changeset' => '39dcf2c3', + 'phabricator-diff-changeset-list' => 'cc2c5de5', + 'phabricator-diff-inline' => '008b6a15', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', 'phabricator-drag-and-drop-file-upload' => '4370900d', @@ -854,7 +856,7 @@ return array( 'phui-icon-view-css' => '4cbc684a', 'phui-image-mask-css' => '62c7f4d2', 'phui-info-view-css' => 'a10a909b', - 'phui-inline-comment-view-css' => 'd5749acc', + 'phui-inline-comment-view-css' => '9863a85e', 'phui-invisible-character-view-css' => 'c694c4a4', 'phui-left-right-css' => '68513c34', 'phui-lightbox-css' => '4ebf22da', @@ -917,6 +919,9 @@ return array( 'unhandled-exception-css' => '9ecfc00d', ), 'requires' => array( + '008b6a15' => array( + 'javelin-dom', + ), '0116d3e8' => array( 'javelin-behavior', 'javelin-dom', @@ -1000,19 +1005,6 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), - '0c083409' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - 'phabricator-diff-inline', - 'phabricator-diff-path-view', - 'phuix-button-view', - ), '0cf79f45' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1023,6 +1015,13 @@ return array( '0d2490ce' => array( 'javelin-install', ), + '0d915ff5' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-history', + 'javelin-external-editor-link-engine', + ), '0eaa33a9' => array( 'javelin-behavior', 'javelin-dom', @@ -1232,6 +1231,20 @@ return array( 'trigger-rule', 'trigger-rule-type', ), + '39dcf2c3' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + 'phabricator-diff-inline', + 'phabricator-diff-path-view', + 'phuix-button-view', + 'javelin-external-editor-link-engine', + ), '3ae89b20' => array( 'phui-workcard-view-css', ), @@ -1316,6 +1329,9 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), + '48a8641f' => array( + 'javelin-install', + ), '48fe33d0' => array( 'javelin-behavior', 'javelin-dom', @@ -1452,12 +1468,6 @@ return array( 'javelin-util', 'javelin-magical-init', ), - '590e6527' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-history', - ), '5a6f6a06' => array( 'javelin-behavior', 'javelin-quicksand', @@ -1935,9 +1945,6 @@ return array( 'javelin-behavior-device', 'javelin-vector', ), - 'b00168c1' => array( - 'javelin-dom', - ), 'b105a3a6' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2077,6 +2084,11 @@ return array( 'phuix-icon-view', 'phabricator-busy', ), + 'cc2c5de5' => array( + 'javelin-install', + 'phuix-button-view', + 'phabricator-diff-tree-view', + ), 'cef53b3e' => array( 'javelin-install', 'javelin-dom', @@ -2119,11 +2131,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'db615898' => array( - 'javelin-install', - 'phuix-button-view', - 'phabricator-diff-tree-view', - ), 'e150bd50' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2422,6 +2429,7 @@ return array( 'phuix-formation-view', 'phuix-formation-column-view', 'phuix-formation-flank-view', + 'javelin-external-editor-link-engine', ), 'diffusion.pkg.css' => array( 'diffusion-icons-css', diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index a3df0dd154..a5dabb6014 100644 --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -220,6 +220,8 @@ return array( 'phuix-formation-view', 'phuix-formation-column-view', 'phuix-formation-flank-view', + + 'javelin-external-editor-link-engine', ), 'diffusion.pkg.css' => array( 'diffusion-icons-css', diff --git a/resources/sql/autopatches/20200520.inline.01.remcache.sql b/resources/sql/autopatches/20200520.inline.01.remcache.sql new file mode 100644 index 0000000000..356bf5a5a5 --- /dev/null +++ b/resources/sql/autopatches/20200520.inline.01.remcache.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_differential.differential_changeset_parse_cache; diff --git a/resources/sql/autopatches/20200520.inline.02.addcache.sql b/resources/sql/autopatches/20200520.inline.02.addcache.sql new file mode 100644 index 0000000000..7b9ed64aac --- /dev/null +++ b/resources/sql/autopatches/20200520.inline.02.addcache.sql @@ -0,0 +1,7 @@ +CREATE TABLE {$NAMESPACE}_differential.differential_changeset_parse_cache ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + cacheIndex BINARY(12) NOT NULL, + cache LONGBLOB NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + UNIQUE KEY `key_cacheIndex` (cacheIndex) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20200520.inline.03.dropcommit.sql b/resources/sql/autopatches/20200520.inline.03.dropcommit.sql new file mode 100644 index 0000000000..a757a8a0a7 --- /dev/null +++ b/resources/sql/autopatches/20200520.inline.03.dropcommit.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS {$NAMESPACE}_differential.differential_commit; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 37d7435eaf..fa3eaabe7a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3160,6 +3160,8 @@ phutil_register_library_map(array( 'PhabricatorDestructionEngineExtensionModule' => 'applications/system/engine/PhabricatorDestructionEngineExtensionModule.php', 'PhabricatorDeveloperConfigOptions' => 'applications/config/option/PhabricatorDeveloperConfigOptions.php', 'PhabricatorDeveloperPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDeveloperPreferencesSettingsPanel.php', + 'PhabricatorDiffInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php', + 'PhabricatorDiffInlineCommentContext' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php', 'PhabricatorDiffInlineCommentQuery' => 'infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php', 'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php', 'PhabricatorDiffScopeEngine' => 'infrastructure/diff/PhabricatorDiffScopeEngine.php', @@ -3592,6 +3594,8 @@ phutil_register_library_map(array( 'PhabricatorInfrastructureTestCase' => '__tests__/PhabricatorInfrastructureTestCase.php', 'PhabricatorInlineComment' => 'infrastructure/diff/interface/PhabricatorInlineComment.php', 'PhabricatorInlineCommentAdjustmentEngine' => 'infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php', + 'PhabricatorInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorInlineCommentContentState.php', + 'PhabricatorInlineCommentContext' => 'infrastructure/diff/inline/PhabricatorInlineCommentContext.php', 'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php', 'PhabricatorInlineCommentInterface' => 'applications/transactions/interface/PhabricatorInlineCommentInterface.php', 'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php', @@ -9627,6 +9631,8 @@ phutil_register_library_map(array( 'PhabricatorDestructionEngineExtensionModule' => 'PhabricatorConfigModule', 'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorDeveloperPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel', + 'PhabricatorDiffInlineCommentContentState' => 'PhabricatorInlineCommentContentState', + 'PhabricatorDiffInlineCommentContext' => 'PhabricatorInlineCommentContext', 'PhabricatorDiffInlineCommentQuery' => 'PhabricatorApplicationTransactionCommentQuery', 'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel', 'PhabricatorDiffScopeEngine' => 'Phobject', @@ -10118,6 +10124,8 @@ phutil_register_library_map(array( 'PhabricatorMarkupInterface', ), 'PhabricatorInlineCommentAdjustmentEngine' => 'Phobject', + 'PhabricatorInlineCommentContentState' => 'Phobject', + 'PhabricatorInlineCommentContext' => 'Phobject', 'PhabricatorInlineCommentController' => 'PhabricatorController', 'PhabricatorInlineSummaryView' => 'AphrontView', 'PhabricatorInstructionsEditField' => 'PhabricatorEditField', diff --git a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php index 01f173bd9a..cb9d5e20eb 100644 --- a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php +++ b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php @@ -17,6 +17,7 @@ final class PhabricatorAuditTransactionComment protected $attributes = array(); private $replyToComment = self::ATTACHABLE; + private $inlineContext = self::ATTACHABLE; public function getApplicationTransactionObject() { return new PhabricatorAuditTransaction(); @@ -83,12 +84,18 @@ final class PhabricatorAuditTransactionComment return $this; } - public function isEmptyInlineComment() { - return !strlen($this->getContent()); - } - public function newInlineCommentObject() { return PhabricatorAuditInlineComment::newFromModernComment($this); } + public function getInlineContext() { + return $this->assertAttached($this->inlineContext); + } + + public function attachInlineContext( + PhabricatorInlineCommentContext $context = null) { + $this->inlineContext = $context; + return $this; + } + } diff --git a/src/applications/config/schema/PhabricatorConfigColumnSchema.php b/src/applications/config/schema/PhabricatorConfigColumnSchema.php index 091b5caef9..e3e2b6f23b 100644 --- a/src/applications/config/schema/PhabricatorConfigColumnSchema.php +++ b/src/applications/config/schema/PhabricatorConfigColumnSchema.php @@ -68,6 +68,37 @@ final class PhabricatorConfigColumnSchema return $this->characterSet; } + public function hasSameColumnTypeAs(PhabricatorConfigColumnSchema $other) { + $u_type = $this->getColumnType(); + $v_type = $other->getColumnType(); + + if ($u_type === $v_type) { + return true; + } + + // See T13536. Display widths for integers were deprecated in MySQL 8.0.17 + // and removed from some display contexts in or around 8.0.19. Older + // MySQL versions will report "int(10)"; newer versions will report "int". + // Accept these as equivalent. + + static $map = array( + 'int(10) unsigned' => 'int unsigned', + 'int(10)' => 'int', + 'bigint(20) unsigned' => 'bigint unsigned', + 'bigint(20)' => 'bigint', + ); + + if (isset($map[$u_type])) { + $u_type = $map[$u_type]; + } + + if (isset($map[$v_type])) { + $v_type = $map[$v_type]; + } + + return ($u_type === $v_type); + } + public function getKeyByteLength($prefix = null) { $type = $this->getColumnType(); @@ -138,7 +169,7 @@ final class PhabricatorConfigColumnSchema $issues[] = self::ISSUE_COLLATION; } - if ($this->getColumnType() != $expect->getColumnType()) { + if (!$this->hasSameColumnTypeAs($expect)) { $issues[] = self::ISSUE_COLUMNTYPE; } diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index df1ffa1d05..0e263d013c 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -200,6 +200,7 @@ final class DifferentialChangesetViewController extends DifferentialController { ->withPublishableComments(true) ->withPublishedComments(true) ->needHidden(true) + ->needInlineContext(true) ->execute(); $inlines = mpull($inlines, 'newInlineCommentObject'); diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index c5b8311630..cc90e36a61 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -328,6 +328,13 @@ final class DifferentialRevisionEditEngine $content = array(); if ($inlines) { + // Reload inlines to get inline context. + $inlines = id(new DifferentialDiffInlineCommentQuery()) + ->setViewer($viewer) + ->withIDs(mpull($inlines, 'getID')) + ->needInlineContext(true) + ->execute(); + $inline_preview = id(new PHUIDiffInlineCommentPreviewListView()) ->setViewer($viewer) ->setInlineComments($inlines); diff --git a/src/applications/differential/engine/DifferentialRevisionDraftEngine.php b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php index 22b103c525..da45f5bc82 100644 --- a/src/applications/differential/engine/DifferentialRevisionDraftEngine.php +++ b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php @@ -11,6 +11,7 @@ final class DifferentialRevisionDraftEngine ->setViewer($viewer) ->withRevisionPHIDs(array($revision->getPHID())) ->withPublishableComments(true) + ->setReturnPartialResultsOnOverheat(true) ->setLimit(1) ->execute(); diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index d9ebad0663..b7ea61b7a9 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -295,8 +295,6 @@ final class DifferentialChangesetParser extends Phobject { * By default, there is no render cache key and parsers do not use the cache. * This is appropriate for rarely-viewed changesets. * - * NOTE: Currently, this key must be a valid Differential Changeset ID. - * * @param string Key for identifying this changeset in the render cache. * @return this */ @@ -376,9 +374,9 @@ final class DifferentialChangesetParser extends Phobject { $conn_r = $changeset->establishConnection('r'); $data = queryfx_one( $conn_r, - 'SELECT * FROM %T WHERE id = %d', - $changeset->getTableName().'_parse_cache', - $render_cache_key); + 'SELECT * FROM %T WHERE cacheIndex = %s', + DifferentialChangeset::TABLE_CACHE, + PhabricatorHash::digestForIndex($render_cache_key)); if (!$data) { return false; @@ -480,12 +478,12 @@ final class DifferentialChangesetParser extends Phobject { try { queryfx( $conn_w, - 'INSERT INTO %T (id, cache, dateCreated) VALUES (%d, %B, %d) + 'INSERT INTO %T (cacheIndex, cache, dateCreated) VALUES (%s, %B, %d) ON DUPLICATE KEY UPDATE cache = VALUES(cache)', DifferentialChangeset::TABLE_CACHE, - $render_cache_key, + PhabricatorHash::digestForIndex($render_cache_key), $cache, - time()); + PhabricatorTime::getNow()); } catch (AphrontQueryException $ex) { // Ignore these exceptions. A common cause is that the cache is // larger than 'max_allowed_packet', in which case we're better off @@ -834,7 +832,6 @@ final class DifferentialChangesetParser extends Phobject { ->setNewAttachesToNewFile($this->rightSideAttachesToNewFile) ->setCodeCoverage($this->getCoverage()) ->setRenderingReference($this->getRenderingReference()) - ->setMarkupEngine($this->markupEngine) ->setHandles($this->handles) ->setOldLines($this->old) ->setNewLines($this->new) @@ -845,6 +842,10 @@ final class DifferentialChangesetParser extends Phobject { ->setHighlightingDisabled($this->highlightingDisabled) ->setDepthOnlyLines($this->getDepthOnlyLines()); + if ($this->markupEngine) { + $renderer->setMarkupEngine($this->markupEngine); + } + list($engine, $old_ref, $new_ref) = $this->newDocumentEngine(); if ($engine) { $engine_blocks = $engine->newEngineBlocks( diff --git a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php index b40589bb36..7e2f7127b1 100644 --- a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php @@ -67,4 +67,87 @@ final class DifferentialDiffInlineCommentQuery return $id_map; } + protected function newInlineContextFromCacheData(array $map) { + return PhabricatorDiffInlineCommentContext::newFromCacheData($map); + } + + protected function newInlineContextMap(array $inlines) { + $viewer = $this->getViewer(); + $map = array(); + + $changeset_ids = mpull($inlines, 'getChangesetID'); + + $changesets = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withIDs($changeset_ids) + ->needHunks(true) + ->execute(); + $changesets = mpull($changesets, null, 'getID'); + + foreach ($inlines as $key => $inline) { + $changeset = idx($changesets, $inline->getChangesetID()); + + if (!$changeset) { + continue; + } + + $hunks = $changeset->getHunks(); + + $is_simple = + (count($hunks) === 1) && + ((int)head($hunks)->getOldOffset() <= 1) && + ((int)head($hunks)->getNewOffset() <= 1); + + if (!$is_simple) { + continue; + } + + if ($inline->getIsNewFile()) { + $vector = $changeset->getNewStatePathVector(); + $filename = last($vector); + $corpus = $changeset->makeNewFile(); + } else { + $vector = $changeset->getOldStatePathVector(); + $filename = last($vector); + $corpus = $changeset->makeOldFile(); + } + + $corpus = phutil_split_lines($corpus); + + // Adjust the line number into a 0-based offset. + $offset = $inline->getLineNumber(); + $offset = $offset - 1; + + // Adjust the inclusive range length into a row count. + $length = $inline->getLineLength(); + $length = $length + 1; + + $head_min = max(0, $offset - 3); + $head_max = $offset; + $head_len = $head_max - $head_min; + + if ($head_len) { + $head = array_slice($corpus, $head_min, $head_len, true); + $head = $this->simplifyContext($head, true); + } else { + $head = array(); + } + + $body = array_slice($corpus, $offset, $length, true); + + $tail = array_slice($corpus, $offset + $length, 3, true); + $tail = $this->simplifyContext($tail, false); + + $context = id(new PhabricatorDiffInlineCommentContext()) + ->setFilename($filename) + ->setHeadLines($head) + ->setBodyLines($body) + ->setTailLines($tail); + + $map[$key] = $context; + } + + return $map; + } + } diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index f4ed05545b..186924e359 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -3,6 +3,17 @@ final class DifferentialChangesetOneUpRenderer extends DifferentialChangesetHTMLRenderer { + private $simpleMode; + + public function setSimpleMode($simple_mode) { + $this->simpleMode = $simple_mode; + return $this; + } + + public function getSimpleMode() { + return $this->simpleMode; + } + public function isOneUpRenderer() { return true; } @@ -36,6 +47,8 @@ final class DifferentialChangesetOneUpRenderer protected function renderPrimitives(array $primitives, $rows) { list($left_prefix, $right_prefix) = $this->getLineIDPrefixes(); + $is_simple = $this->getSimpleMode(); + $no_copy = phutil_tag('td', array('class' => 'copy')); $no_coverage = null; @@ -185,6 +198,12 @@ final class DifferentialChangesetOneUpRenderer $cells[] = $no_coverage; } + // In simple mode, only render the text. This is used to render + // "Edit Suggestions" in inline comments. + if ($is_simple) { + $cells = array($cells[3]); + } + $out[] = phutil_tag('tr', array(), $cells); break; @@ -231,11 +250,17 @@ final class DifferentialChangesetOneUpRenderer } } + $result = null; + if ($out) { - return $this->wrapChangeInTable(phutil_implode_html('', $out)); + if ($is_simple) { + $result = $this->newSimpleTable($out); + } else { + $result = $this->wrapChangeInTable(phutil_implode_html('', $out)); + } } - return null; + return $result; } public function renderDocumentEngineBlocks( @@ -488,4 +513,14 @@ final class DifferentialChangesetOneUpRenderer ->addInlineView($view); } + + private function newSimpleTable($content) { + return phutil_tag( + 'table', + array( + 'class' => 'diff-1up-simple-table', + ), + $content); + } + } diff --git a/src/applications/differential/storage/DifferentialSchemaSpec.php b/src/applications/differential/storage/DifferentialSchemaSpec.php index 4f747af06c..cf8b996e97 100644 --- a/src/applications/differential/storage/DifferentialSchemaSpec.php +++ b/src/applications/differential/storage/DifferentialSchemaSpec.php @@ -9,7 +9,8 @@ final class DifferentialSchemaSpec extends PhabricatorConfigSchemaSpec { id(new DifferentialRevision())->getApplicationName(), DifferentialChangeset::TABLE_CACHE, array( - 'id' => 'id', + 'id' => 'auto', + 'cacheIndex' => 'bytes12', 'cache' => 'bytes', 'dateCreated' => 'epoch', ), @@ -18,7 +19,11 @@ final class DifferentialSchemaSpec extends PhabricatorConfigSchemaSpec { 'columns' => array('id'), 'unique' => true, ), - 'dateCreated' => array( + 'key_cacheIndex' => array( + 'columns' => array('cacheIndex'), + 'unique' => true, + ), + 'key_created' => array( 'columns' => array('dateCreated'), ), ), @@ -26,27 +31,6 @@ final class DifferentialSchemaSpec extends PhabricatorConfigSchemaSpec { 'persistence' => PhabricatorConfigTableSchema::PERSISTENCE_CACHE, )); - // TODO: All readers and writers for this table were removed in April - // 2019. Destroy this table once we're sure we won't miss it. - - $this->buildRawSchema( - id(new DifferentialRevision())->getApplicationName(), - 'differential_commit', - array( - 'revisionID' => 'id', - 'commitPHID' => 'phid', - ), - array( - 'PRIMARY' => array( - 'columns' => array('revisionID', 'commitPHID'), - 'unique' => true, - ), - 'commitPHID' => array( - 'columns' => array('commitPHID'), - 'unique' => true, - ), - )); - $this->buildRawSchema( id(new DifferentialRevision())->getApplicationName(), ArcanistDifferentialRevisionHash::TABLE_NAME, diff --git a/src/applications/differential/storage/DifferentialTransactionComment.php b/src/applications/differential/storage/DifferentialTransactionComment.php index f5cb772d48..491d89a968 100644 --- a/src/applications/differential/storage/DifferentialTransactionComment.php +++ b/src/applications/differential/storage/DifferentialTransactionComment.php @@ -18,6 +18,7 @@ final class DifferentialTransactionComment private $replyToComment = self::ATTACHABLE; private $isHidden = self::ATTACHABLE; private $changeset = self::ATTACHABLE; + private $inlineContext = self::ATTACHABLE; public function getApplicationTransactionObject() { return new DifferentialTransaction(); @@ -129,12 +130,18 @@ final class DifferentialTransactionComment return $this; } - public function isEmptyInlineComment() { - return !strlen($this->getContent()); - } - public function newInlineCommentObject() { return DifferentialInlineComment::newFromModernComment($this); } + public function getInlineContext() { + return $this->assertAttached($this->inlineContext); + } + + public function attachInlineContext( + PhabricatorInlineCommentContext $context = null) { + $this->inlineContext = $context; + return $this; + } + } diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php index 357dc375cf..d23ddc62a4 100644 --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/DifferentialChangesetDetailView.php @@ -252,7 +252,7 @@ final class DifferentialChangesetDetailView extends AphrontView { 'isLowImportance' => $changeset->getIsLowImportanceChangeset(), 'isOwned' => $changeset->getIsOwnedChangeset(), - 'editorURI' => $this->getEditorURI(), + 'editorURITemplate' => $this->getEditorURITemplate(), 'editorConfigureURI' => $this->getEditorConfigureURI(), 'loaded' => $is_loaded, @@ -321,7 +321,7 @@ final class DifferentialChangesetDetailView extends AphrontView { return $this->diff; } - private function getEditorURI() { + private function getEditorURITemplate() { $repository = $this->getRepository(); if (!$repository) { return null; @@ -342,9 +342,7 @@ final class DifferentialChangesetDetailView extends AphrontView { $path = $changeset->getAbsoluteRepositoryPath($repository, $diff); $path = ltrim($path, '/'); - $line = idx($changeset->getMetadata(), 'line:first', 1); - - return $link_engine->getURIForPath($path, $line); + return $link_engine->getURITokensForPath($path); } private function getEditorConfigureURI() { diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index bf8aa8fae5..cab63eb998 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -373,6 +373,9 @@ final class DifferentialChangesetListView extends AphrontView { 'Add new inline comment on selected source text.' => pht('Add new inline comment on selected source text.'), + + 'Suggest Edit' => pht('Suggest Edit'), + 'Discard Edit' => pht('Discard Edit'), ), )); diff --git a/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php b/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php index 8837dbe63f..e5d45f315b 100644 --- a/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitDraftEngine.php @@ -11,6 +11,7 @@ final class DiffusionCommitDraftEngine ->setViewer($viewer) ->withCommitPHIDs(array($commit->getPHID())) ->withPublishableComments(true) + ->setReturnPartialResultsOnOverheat(true) ->setLimit(1) ->execute(); diff --git a/src/applications/diffusion/protocol/DiffusionCommandEngine.php b/src/applications/diffusion/protocol/DiffusionCommandEngine.php index a0fe568168..a0f1adaf1f 100644 --- a/src/applications/diffusion/protocol/DiffusionCommandEngine.php +++ b/src/applications/diffusion/protocol/DiffusionCommandEngine.php @@ -120,6 +120,7 @@ abstract class DiffusionCommandEngine extends Phobject { public function newFuture() { $argv = $this->newCommandArgv(); $env = $this->newCommandEnvironment(); + $is_passthru = $this->getPassthru(); if ($this->getSudoAsDaemon()) { $command = call_user_func_array('csprintf', $argv); @@ -127,7 +128,7 @@ abstract class DiffusionCommandEngine extends Phobject { $argv = array('%C', $command); } - if ($this->getPassthru()) { + if ($is_passthru) { $future = newv('PhutilExecPassthru', $argv); } else { $future = newv('ExecFuture', $argv); @@ -139,7 +140,9 @@ abstract class DiffusionCommandEngine extends Phobject { // to try to avoid cases where `git fetch` hangs for some reason and we're // left sitting with a held lock forever. $repository = $this->getRepository(); - $future->setTimeout($repository->getEffectiveCopyTimeLimit()); + if (!$is_passthru) { + $future->setTimeout($repository->getEffectiveCopyTimeLimit()); + } return $future; } diff --git a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php index af116a4097..01dc49a91e 100644 --- a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php +++ b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php @@ -66,4 +66,12 @@ final class DiffusionDiffInlineCommentQuery return array(); } + protected function newInlineContextMap(array $inlines) { + return array(); + } + + protected function newInlineContextFromCacheData(array $map) { + return PhabricatorDiffInlineCommentContext::newFromCacheData($map); + } + } diff --git a/src/applications/people/controller/PhabricatorPeopleNewController.php b/src/applications/people/controller/PhabricatorPeopleNewController.php index 44dfe0e8a6..858ebef234 100644 --- a/src/applications/people/controller/PhabricatorPeopleNewController.php +++ b/src/applications/people/controller/PhabricatorPeopleNewController.php @@ -50,9 +50,12 @@ final class PhabricatorPeopleNewController if (!strlen($new_email)) { $errors[] = pht('Email is required.'); $e_email = pht('Required'); - } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) { + } else if (!PhabricatorUserEmail::isValidAddress($new_email)) { + $errors[] = PhabricatorUserEmail::describeValidAddresses(); $e_email = pht('Invalid'); + } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) { $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); + $e_email = pht('Not Allowed'); } else { $e_email = null; } diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 4d4b1c0d2c..e1b8dc7264 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -39,7 +39,6 @@ abstract class PhabricatorInlineCommentController private $isOnRight; private $lineNumber; private $lineLength; - private $commentText; private $operation; private $commentID; private $renderer; @@ -53,10 +52,6 @@ abstract class PhabricatorInlineCommentController return $this->operation; } - public function getCommentText() { - return $this->commentText; - } - public function getLineLength() { return $this->lineLength; } @@ -99,16 +94,16 @@ abstract class PhabricatorInlineCommentController $request = $this->getRequest(); $viewer = $this->getViewer(); + if (!$request->validateCSRF()) { + return new Aphront404Response(); + } + $this->readRequestParameters(); $op = $this->getOperation(); switch ($op) { case 'hide': case 'show': - if (!$request->validateCSRF()) { - return new Aphront404Response(); - } - $ids = $request->getStrList('ids'); if ($ids) { if ($op == 'hide') { @@ -120,9 +115,6 @@ abstract class PhabricatorInlineCommentController return id(new AphrontAjaxResponse())->setContent(array()); case 'done': - if (!$request->validateCSRF()) { - return new Aphront404Response(); - } $inline = $this->loadCommentForDone($this->getCommentID()); $is_draft_state = false; @@ -158,10 +150,6 @@ abstract class PhabricatorInlineCommentController case 'delete': case 'undelete': case 'refdelete': - if (!$request->validateCSRF()) { - return new Aphront404Response(); - } - // NOTE: For normal deletes, we just process the delete immediately // and show an "Undo" action. For deletes by reference from the // preview ("refdelete"), we prompt first (because the "Undo" may @@ -193,15 +181,14 @@ abstract class PhabricatorInlineCommentController return $this->buildEmptyResponse(); case 'edit': + case 'save': $inline = $this->loadCommentByIDForEdit($this->getCommentID()); - $text = $this->getCommentText(); + if ($op === 'save') { + $this->updateCommentContentState($inline); - if ($request->isFormPost()) { - if (strlen($text)) { - $inline - ->setContent($text) - ->setIsEditing(false); + $inline->setIsEditing(false); + if (!$inline->isVoidComment($viewer)) { $this->saveComment($inline); return $this->buildRenderedCommentResponse( @@ -216,7 +203,7 @@ abstract class PhabricatorInlineCommentController } } else { // NOTE: At time of writing, the "editing" state of inlines is - // preserved by simluating a click on "Edit" when the inline loads. + // preserved by simulating a click on "Edit" when the inline loads. // In this case, we don't want to "saveComment()", because it // recalculates object drafts and purges versioned drafts. @@ -234,8 +221,8 @@ abstract class PhabricatorInlineCommentController $is_dirty = true; } - if (strlen($text)) { - $inline->setContent($text); + if ($this->hasContentState()) { + $this->updateCommentContentState($inline); $is_dirty = true; } else { PhabricatorInlineComment::loadAndAttachVersionedDrafts( @@ -253,7 +240,7 @@ abstract class PhabricatorInlineCommentController $view = $this->buildScaffoldForView($edit_dialog); - return $this->newInlineResponse($inline, $view); + return $this->newInlineResponse($inline, $view, true); case 'cancel': $inline = $this->loadCommentByIDForEdit($this->getCommentID()); @@ -262,13 +249,9 @@ abstract class PhabricatorInlineCommentController // If the user uses "Undo" to get into an edited state ("AB"), then // clicks cancel to return to the previous state ("A"), we also want // to set the stored state back to "A". - $text = $this->getCommentText(); - if (strlen($text)) { - $inline->setContent($text); - } + $this->updateCommentContentState($inline); - $content = $inline->getContent(); - if (!strlen($content)) { + if ($inline->isVoidComment($viewer)) { $inline->setIsDeleted(1); } @@ -283,11 +266,9 @@ abstract class PhabricatorInlineCommentController $viewer->getPHID(), $inline->getID()); - $text = $this->getCommentText(); - - $versioned_draft - ->setProperty('inline.text', $text) - ->save(); + $map = $this->newRequestContentState($inline)->newStorageMap(); + $versioned_draft->setProperty('inline.state', $map); + $versioned_draft->save(); // We have to synchronize the draft engine after saving a versioned // draft, because taking an inline comment from "no text, no draft" @@ -318,11 +299,11 @@ abstract class PhabricatorInlineCommentController ->setIsNewFile($is_new) ->setLineNumber($number) ->setLineLength($length) - ->setContent((string)$this->getCommentText()) ->setReplyToCommentPHID($this->getReplyToCommentPHID()) ->setIsEditing(true) ->setStartOffset($request->getInt('startOffset')) - ->setEndOffset($request->getInt('endOffset')); + ->setEndOffset($request->getInt('endOffset')) + ->setContent(''); $document_engine_key = $request->getStr('documentEngineKey'); if ($document_engine_key !== null) { @@ -338,8 +319,15 @@ abstract class PhabricatorInlineCommentController } } + if ($this->hasContentState()) { + $this->updateCommentContentState($inline); + } + $this->saveComment($inline); + // Reload the inline to attach context. + $inline = $this->loadCommentByIDForEdit($inline->getID()); + $edit_dialog = $this->buildEditDialog($inline); if ($this->getOperation() == 'reply') { @@ -350,7 +338,7 @@ abstract class PhabricatorInlineCommentController $view = $this->buildScaffoldForView($edit_dialog); - return $this->newInlineResponse($inline, $view); + return $this->newInlineResponse($inline, $view, true); } } @@ -365,7 +353,6 @@ abstract class PhabricatorInlineCommentController $this->isOnRight = $request->getBool('on_right'); $this->lineNumber = $request->getInt('number'); $this->lineLength = $request->getInt('length'); - $this->commentText = $request->getStr('text'); $this->commentID = $request->getInt('id'); $this->operation = $request->getStr('op'); $this->renderer = $request->getStr('renderer'); @@ -447,7 +434,7 @@ abstract class PhabricatorInlineCommentController $view = $this->buildScaffoldForView($view); - return $this->newInlineResponse($inline, $view); + return $this->newInlineResponse($inline, $view, false); } private function buildScaffoldForView(PHUIDiffInlineCommentView $view) { @@ -462,11 +449,29 @@ abstract class PhabricatorInlineCommentController private function newInlineResponse( PhabricatorInlineComment $inline, - $view) { + $view, + $is_edit) { + + if ($inline->getReplyToCommentPHID()) { + $can_suggest = false; + } else { + $can_suggest = (bool)$inline->getInlineContext(); + } + + if ($is_edit) { + $viewer = $this->getViewer(); + $content_state = $inline->getContentStateForEdit($viewer); + } else { + $content_state = $inline->getContentState(); + } + + $state_map = $content_state->newStorageMap(); $response = array( 'inline' => array( 'id' => $inline->getID(), + 'contentState' => $state_map, + 'canSuggestEdit' => $can_suggest, ), 'view' => hsprintf('%s', $view), ); @@ -493,7 +498,8 @@ abstract class PhabricatorInlineCommentController $viewer = $this->getViewer(); $query = $this->newInlineCommentQuery() - ->withIDs(array($id)); + ->withIDs(array($id)) + ->needInlineContext(true); $inline = $this->loadCommentByQuery($query); @@ -529,6 +535,28 @@ abstract class PhabricatorInlineCommentController return $inline; } + private function hasContentState() { + $request = $this->getRequest(); + return (bool)$request->getBool('hasContentState'); + } + + private function newRequestContentState($inline) { + $request = $this->getRequest(); + return $inline->newContentStateFromRequest($request); + } + + private function updateCommentContentState(PhabricatorInlineComment $inline) { + if (!$this->hasContentState()) { + throw new Exception( + pht( + 'Attempting to update comment content state, but request has no '. + 'content state.')); + } + + $state = $this->newRequestContentState($inline); + $inline->setContentState($state); + } + private function saveComment(PhabricatorInlineComment $inline) { $viewer = $this->getViewer(); $draft_engine = $this->newDraftEngine(); diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php new file mode 100644 index 0000000000..0f379acc3e --- /dev/null +++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php @@ -0,0 +1,69 @@ +getContentHasSuggestion()) { + if (strlen($this->getContentSuggestionText())) { + return false; + } + } + + return true; + } + + public function setContentSuggestionText($suggestion_text) { + $this->suggestionText = $suggestion_text; + return $this; + } + + public function getContentSuggestionText() { + return $this->suggestionText; + } + + public function setContentHasSuggestion($has_suggestion) { + $this->hasSuggestion = $has_suggestion; + return $this; + } + + public function getContentHasSuggestion() { + return $this->hasSuggestion; + } + + public function newStorageMap() { + return parent::writeStorageMap() + array( + 'hasSuggestion' => $this->getContentHasSuggestion(), + 'suggestionText' => $this->getContentSuggestionText(), + ); + } + + public function readStorageMap(array $map) { + $result = parent::readStorageMap($map); + + $has_suggestion = (bool)idx($map, 'hasSuggestion'); + $this->setContentHasSuggestion($has_suggestion); + + $suggestion_text = (string)idx($map, 'suggestionText'); + $this->setContentSuggestionText($suggestion_text); + + return $result; + } + + protected function newStorageMapFromRequest(AphrontRequest $request) { + $map = parent::newStorageMapFromRequest($request); + + $map['hasSuggestion'] = (bool)$request->getBool('hasSuggestion'); + $map['suggestionText'] = (string)$request->getStr('suggestionText'); + + return $map; + } + +} diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php new file mode 100644 index 0000000000..f09d53092d --- /dev/null +++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php @@ -0,0 +1,67 @@ +setFilename(idx($map, 'filename')); + $context->setHeadLines(idx($map, 'headLines')); + $context->setBodyLines(idx($map, 'bodyLines')); + $context->setTailLines(idx($map, 'tailLines')); + + return $context; + } + + public function newCacheDataMap() { + return array( + 'filename' => $this->getFilename(), + 'headLines' => $this->getHeadLines(), + 'bodyLines' => $this->getBodyLines(), + 'tailLines' => $this->getTailLines(), + ); + } + + public function setFilename($filename) { + $this->filename = $filename; + return $this; + } + + public function getFilename() { + return $this->filename; + } + + public function setHeadLines(array $head_lines) { + $this->headLines = $head_lines; + return $this; + } + + public function getHeadLines() { + return $this->headLines; + } + + public function setBodyLines(array $body_lines) { + $this->bodyLines = $body_lines; + return $this; + } + + public function getBodyLines() { + return $this->bodyLines; + } + + public function setTailLines(array $tail_lines) { + $this->tailLines = $tail_lines; + return $this; + } + + public function getTailLines() { + return $this->tailLines; + } + +} diff --git a/src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php b/src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php new file mode 100644 index 0000000000..97d444b711 --- /dev/null +++ b/src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php @@ -0,0 +1,47 @@ +contentText = $content_text; + return $this; + } + + public function getContentText() { + return $this->contentText; + } + + public function isEmptyContentState() { + return !strlen($this->getContentText()); + } + + public function writeStorageMap() { + return array( + 'text' => $this->getContentText(), + ); + } + + public function readStorageMap(array $map) { + $text = (string)idx($map, 'text'); + $this->setContentText($text); + + return $this; + } + + final public function readFromRequest(AphrontRequest $request) { + $map = $this->newStorageMapFromRequest($request); + return $this->readStorageMap($map); + } + + protected function newStorageMapFromRequest(AphrontRequest $request) { + $map = array(); + + $map['text'] = (string)$request->getStr('text'); + + return $map; + } + +} diff --git a/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php new file mode 100644 index 0000000000..a292dc72ee --- /dev/null +++ b/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php @@ -0,0 +1,4 @@ +storageObject; } + public function getInlineCommentCacheFragment() { + $phid = $this->getPHID(); + + if ($phid === null) { + return null; + } + + return sprintf('inline(%s)', $phid); + } + abstract protected function newStorageObject(); abstract public function getControllerURI(); @@ -299,27 +309,59 @@ abstract class PhabricatorInlineComment } public function isVoidComment(PhabricatorUser $viewer) { - return !strlen($this->getContentForEdit($viewer)); + return $this->getContentStateForEdit($viewer)->isEmptyContentState(); } - public function getContentForEdit(PhabricatorUser $viewer) { - $content = $this->getContent(); + public function getContentStateForEdit(PhabricatorUser $viewer) { + $state = $this->getContentState(); - if (!$this->hasVersionedDraftForViewer($viewer)) { - return $content; + if ($this->hasVersionedDraftForViewer($viewer)) { + $versioned_draft = $this->getVersionedDraftForViewer($viewer); + if ($versioned_draft) { + $storage_map = $versioned_draft->getProperty('inline.state'); + if (is_array($storage_map)) { + $state->readStorageMap($storage_map); + } + } } - $versioned_draft = $this->getVersionedDraftForViewer($viewer); - if (!$versioned_draft) { - return $content; + return $state; + } + + protected function newContentState() { + return new PhabricatorDiffInlineCommentContentState(); + } + + public function newContentStateFromRequest(AphrontRequest $request) { + return $this->newContentState()->readFromRequest($request); + } + + public function getContentState() { + $state = $this->newContentState(); + + $storage = $this->getStorageObject(); + $storage_map = $storage->getAttribute('inline.state'); + if (is_array($storage_map)) { + $state->readStorageMap($storage_map); } - $draft_text = $versioned_draft->getProperty('inline.text'); - if ($draft_text === null) { - return $content; - } + $state->setContentText($this->getContent()); - return $draft_text; + return $state; + } + + public function setContentState(PhabricatorInlineCommentContentState $state) { + $storage = $this->getStorageObject(); + $storage_map = $state->newStorageMap(); + $storage->setAttribute('inline.state', $storage_map); + + $this->setContent($state->getContentText()); + + return $this; + } + + public function getInlineContext() { + return $this->getStorageObject()->getInlineContext(); } diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php index 88b6abddec..275a9a5aea 100644 --- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php +++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php @@ -3,12 +3,15 @@ abstract class PhabricatorDiffInlineCommentQuery extends PhabricatorApplicationTransactionCommentQuery { + const INLINE_CONTEXT_CACHE_VERSION = 1; + private $fixedStates; private $needReplyToComments; private $publishedComments; private $publishableComments; private $needHidden; private $needAppliedDrafts; + private $needInlineContext; abstract protected function buildInlineCommentWhereClauseParts( AphrontDatabaseConnection $conn); @@ -17,6 +20,9 @@ abstract class PhabricatorDiffInlineCommentQuery $viewer_phid, array $comments); + abstract protected function newInlineContextMap(array $inlines); + abstract protected function newInlineContextFromCacheData(array $map); + final public function withFixedStates(array $states) { $this->fixedStates = $states; return $this; @@ -42,6 +48,11 @@ abstract class PhabricatorDiffInlineCommentQuery return $this; } + final public function needInlineContext($need_context) { + $this->needInlineContext = $need_context; + return $this; + } + final public function needAppliedDrafts($need_applied) { $this->needAppliedDrafts = $need_applied; return $this; @@ -173,26 +184,6 @@ abstract class PhabricatorDiffInlineCommentQuery return $inlines; } - if ($this->needHidden) { - $viewer_phid = $viewer->getPHID(); - - if ($viewer_phid) { - $hidden = $this->loadHiddenCommentIDs( - $viewer_phid, - $inlines); - } else { - $hidden = array(); - } - - foreach ($inlines as $inline) { - $inline->attachIsHidden(isset($hidden[$inline->getID()])); - } - } - - if (!$inlines) { - return $inlines; - } - $need_drafts = $this->needAppliedDrafts; $drop_void = $this->publishableComments; $convert_objects = ($need_drafts || $drop_void); @@ -218,9 +209,9 @@ abstract class PhabricatorDiffInlineCommentQuery // as it is currently shown to the user, not as it was stored the last // time they clicked "Save". - $draft_content = $inline->getContentForEdit($viewer); - if (strlen($draft_content)) { - $inline->setContent($draft_content); + $draft_state = $inline->getContentStateForEdit($viewer); + if (!$draft_state->isEmptyContentState()) { + $inline->setContentState($draft_state); } } } @@ -247,4 +238,145 @@ abstract class PhabricatorDiffInlineCommentQuery return $inlines; } + protected function didFilterPage(array $inlines) { + $viewer = $this->getViewer(); + + if ($this->needHidden) { + $viewer_phid = $viewer->getPHID(); + + if ($viewer_phid) { + $hidden = $this->loadHiddenCommentIDs( + $viewer_phid, + $inlines); + } else { + $hidden = array(); + } + + foreach ($inlines as $inline) { + $inline->attachIsHidden(isset($hidden[$inline->getID()])); + } + } + + if ($this->needInlineContext) { + $need_context = array(); + foreach ($inlines as $inline) { + $object = $inline->newInlineCommentObject(); + + if ($object->getDocumentEngineKey() !== null) { + $inline->attachInlineContext(null); + continue; + } + + $need_context[] = $inline; + } + + if ($need_context) { + $this->loadInlineCommentContext($need_context); + } + } + + return $inlines; + } + + private function loadInlineCommentContext(array $inlines) { + $cache_keys = array(); + foreach ($inlines as $key => $inline) { + $object = $inline->newInlineCommentObject(); + $fragment = $object->getInlineCommentCacheFragment(); + + if ($fragment === null) { + continue; + } + + $cache_keys[$key] = sprintf( + '%s.context(v%d)', + $fragment, + self::INLINE_CONTEXT_CACHE_VERSION); + } + + $cache = PhabricatorCaches::getMutableStructureCache(); + + $cache_map = $cache->getKeys($cache_keys); + + $context_map = array(); + $need_construct = array(); + + foreach ($inlines as $key => $inline) { + $cache_key = idx($cache_keys, $key); + + if ($cache_key !== null) { + if (array_key_exists($cache_key, $cache_map)) { + $cache_data = $cache_map[$cache_key]; + $context_map[$key] = $this->newInlineContextFromCacheData( + $cache_data); + continue; + } + } + + $need_construct[$key] = $inline; + } + + if ($need_construct) { + $construct_map = $this->newInlineContextMap($need_construct); + + $write_map = array(); + foreach ($construct_map as $key => $context) { + if ($context === null) { + $cache_data = $context; + } else { + $cache_data = $this->newCacheDataFromInlineContext($context); + } + + $cache_key = idx($cache_keys, $key); + if ($cache_key !== null) { + $write_map[$cache_key] = $cache_data; + } + } + + if ($write_map) { + $cache->setKeys($write_map); + } + + $context_map += $construct_map; + } + + foreach ($inlines as $key => $inline) { + $inline->attachInlineContext(idx($context_map, $key)); + } + } + + protected function newCacheDataFromInlineContext( + PhabricatorInlineCommentContext $context) { + return $context->newCacheDataMap(); + } + + final protected function simplifyContext(array $lines, $is_head) { + // We want to provide the smallest amount of context we can while still + // being useful, since the actual code is visible nearby and showing a + // ton of context is silly. + + // Examine each line until we find one that looks "useful" (not just + // whitespace or a single bracket). Once we find a useful piece of context + // to anchor the text, discard the rest of the lines beyond it. + + if ($is_head) { + $lines = array_reverse($lines, true); + } + + $saw_context = false; + foreach ($lines as $key => $line) { + if ($saw_context) { + unset($lines[$key]); + continue; + } + + $saw_context = (strlen(trim($line)) > 3); + } + + if ($is_head) { + $lines = array_reverse($lines, true); + } + + return $lines; + } } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index 900491e00b..fdaa99ac7e 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -101,7 +101,6 @@ final class PHUIDiffInlineCommentDetailView $classes[] = 'inline-comment-element'; } - $content = $inline->getContent(); $handles = $this->handles; $links = array(); @@ -428,6 +427,15 @@ final class PHUIDiffInlineCommentDetailView $metadata['menuItems'] = $menu_items; + $suggestion_content = $this->newSuggestionView($inline); + + $inline_content = phutil_tag( + 'div', + array( + 'class' => 'phabricator-remarkup', + ), + $content); + $markup = javelin_tag( 'div', array( @@ -446,9 +454,15 @@ final class PHUIDiffInlineCommentDetailView $group_left, $group_right, )), - phutil_tag_div( - 'differential-inline-comment-content', - phutil_tag_div('phabricator-remarkup', $content)), + phutil_tag( + 'div', + array( + 'class' => 'differential-inline-comment-content', + ), + array( + $suggestion_content, + $inline_content, + )), )); $summary = phutil_tag( @@ -492,4 +506,77 @@ final class PHUIDiffInlineCommentDetailView return true; } + private function newSuggestionView(PhabricatorInlineComment $inline) { + $content_state = $inline->getContentState(); + if (!$content_state->getContentHasSuggestion()) { + return null; + } + + $context = $inline->getInlineContext(); + if (!$context) { + return null; + } + + $head_lines = $context->getHeadLines(); + $head_lines = implode('', $head_lines); + + $tail_lines = $context->getTailLines(); + $tail_lines = implode('', $tail_lines); + + $old_lines = $context->getBodyLines(); + $old_lines = implode('', $old_lines); + $old_lines = $head_lines.$old_lines.$tail_lines; + if (strlen($old_lines) && !preg_match('/\n\z/', $old_lines)) { + $old_lines .= "\n"; + } + + $new_lines = $content_state->getContentSuggestionText(); + $new_lines = $head_lines.$new_lines.$tail_lines; + if (strlen($new_lines) && !preg_match('/\n\z/', $new_lines)) { + $new_lines .= "\n"; + } + + if ($old_lines === $new_lines) { + return null; + } + + $viewer = $this->getViewer(); + + $changeset = id(new PhabricatorDifferenceEngine()) + ->generateChangesetFromFileContent($old_lines, $new_lines); + + $changeset->setFilename($context->getFilename()); + + $viewstate = new PhabricatorChangesetViewState(); + + $parser = id(new DifferentialChangesetParser()) + ->setViewer($viewer) + ->setViewstate($viewstate) + ->setChangeset($changeset); + + $fragment = $inline->getInlineCommentCacheFragment(); + if ($fragment !== null) { + $cache_key = sprintf( + '%s.suggestion-view(v1, %s)', + $fragment, + PhabricatorHash::digestForIndex($new_lines)); + $parser->setRenderCacheKey($cache_key); + } + + $renderer = new DifferentialChangesetOneUpRenderer(); + $renderer->setSimpleMode(true); + + $parser->setRenderer($renderer); + + $diff_view = $parser->render(0, 0xFFFF, array()); + + $view = phutil_tag( + 'div', + array( + 'class' => 'inline-suggestion-view PhabricatorMonospaced', + ), + $diff_view); + + return $view; + } } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php index b70b0bcd1f..5c29c1666c 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php @@ -22,39 +22,12 @@ final class PHUIDiffInlineCommentEditView 'sigil' => 'inline-edit-form', ), array( - $this->renderInputs(), $this->renderBody(), )); return $content; } - private function renderInputs() { - $inputs = array(); - $inline = $this->getInlineComment(); - - $inputs[] = array('op', 'edit'); - $inputs[] = array('id', $inline->getID()); - - $inputs[] = array('on_right', $this->getIsOnRight()); - $inputs[] = array('renderer', $this->getRenderer()); - - $out = array(); - - foreach ($inputs as $input) { - list($name, $value) = $input; - $out[] = phutil_tag( - 'input', - array( - 'type' => 'hidden', - 'name' => $name, - 'value' => $value, - )); - } - - return $out; - } - private function renderBody() { $buttons = array(); @@ -73,17 +46,23 @@ final class PHUIDiffInlineCommentEditView ), $this->title); + $corpus_view = $this->newCorpusView(); + $body = phutil_tag( 'div', array( 'class' => 'differential-inline-comment-edit-body', ), - $this->newTextarea()); + array( + $corpus_view, + $this->newTextarea(), + )); - $edit = phutil_tag( + $edit = javelin_tag( 'div', array( 'class' => 'differential-inline-comment-edit-buttons grouped', + 'sigil' => 'inline-edit-buttons', ), array( $buttons, @@ -109,14 +88,130 @@ final class PHUIDiffInlineCommentEditView $viewer = $this->getViewer(); $inline = $this->getInlineComment(); - $text = $inline->getContentForEdit($viewer); + $state = $inline->getContentStateForEdit($viewer); return id(new PhabricatorRemarkupControl()) ->setViewer($viewer) - ->setSigil('differential-inline-comment-edit-textarea') - ->setName('text') - ->setValue($text) + ->setSigil('inline-content-text') + ->setValue($state->getContentText()) ->setDisableFullScreen(true); } + private function newCorpusView() { + $viewer = $this->getViewer(); + $inline = $this->getInlineComment(); + + $context = $inline->getInlineContext(); + if ($context === null) { + return null; + } + + $head = $context->getHeadLines(); + $head = $this->newContextView($head); + + $state = $inline->getContentStateForEdit($viewer); + + $main = $state->getContentSuggestionText(); + $main_count = count(phutil_split_lines($main)); + + $default = $context->getBodyLines(); + $default = implode('', $default); + + // Browsers ignore one leading newline in text areas. Add one so that + // any actual leading newlines in the content are preserved. + $main = "\n".$main; + + $textarea = javelin_tag( + 'textarea', + array( + 'class' => 'inline-suggestion-input PhabricatorMonospaced', + 'rows' => max(3, $main_count + 1), + 'sigil' => 'inline-content-suggestion', + 'meta' => array( + 'defaultText' => $default, + ), + ), + $main); + + $main = phutil_tag( + 'tr', + array( + 'class' => 'inline-suggestion-input-row', + ), + array( + phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-line-cell', + ), + null), + phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-input-cell', + ), + $textarea), + )); + + $tail = $context->getTailLines(); + $tail = $this->newContextView($tail); + + $body = phutil_tag( + 'tbody', + array(), + array( + $head, + $main, + $tail, + )); + + $table = phutil_tag( + 'table', + array( + 'class' => 'inline-suggestion-table', + ), + $body); + + $container = phutil_tag( + 'div', + array( + 'class' => 'inline-suggestion', + ), + $table); + + return $container; + } + + private function newContextView(array $lines) { + if (!$lines) { + return array(); + } + + $rows = array(); + foreach ($lines as $index => $line) { + $line_cell = phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-line-cell PhabricatorMonospaced', + ), + $index + 1); + + $text_cell = phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-text-cell PhabricatorMonospaced', + ), + $line); + + $cells = array( + $line_cell, + $text_cell, + ); + + $rows[] = phutil_tag('tr', array(), $cells); + } + + return $rows; + } + } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php index 2481442c1d..11be8d0ec1 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php @@ -82,7 +82,6 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { 'number' => $inline->getLineNumber(), 'length' => $inline->getLineLength(), 'isNewFile' => (bool)$inline->getIsNewFile(), - 'original' => $inline->getContent(), 'replyToCommentPHID' => $inline->getReplyToCommentPHID(), 'isDraft' => $inline->isDraft(), 'isFixed' => $is_fixed, @@ -93,8 +92,8 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { 'documentEngineKey' => $inline->getDocumentEngineKey(), 'startOffset' => $inline->getStartOffset(), 'endOffset' => $inline->getEndOffset(), - 'on_right' => $this->getIsOnRight(), + 'contentState' => $inline->getContentState()->newStorageMap(), ); } diff --git a/webroot/rsrc/css/application/differential/phui-inline-comment.css b/webroot/rsrc/css/application/differential/phui-inline-comment.css index ce5b87effd..246c81013c 100644 --- a/webroot/rsrc/css/application/differential/phui-inline-comment.css +++ b/webroot/rsrc/css/application/differential/phui-inline-comment.css @@ -436,3 +436,72 @@ background: {$lightyellow}; border-color: {$yellow}; } + +.inline-suggestion { + display: none; + margin: 0 -8px; +} + +.has-suggestion .inline-suggestion { + display: block; +} + +.differential-inline-comment-edit-buttons button.inline-button-left { + float: left; + margin: 0 6px 0 0; +} + +.inline-suggestion-table { + table-layout: fixed; + width: 100%; + margin-bottom: 8px; + white-space: pre-wrap; + background: {$greybackground}; + border-width: 1px 0; + border-style: solid; + border-color: {$lightgreyborder}; +} + +textarea.inline-suggestion-input { + width: 100%; + height: auto; + max-width: 100%; +} + +.inline-suggestion-line-cell { + text-align: right; + background: {$darkgreybackground}; + width: 36px; + color: {$greytext}; + border-right: 1px solid {$lightgreyborder}; +} + +.inline-suggestion-table td.inline-suggestion-input-cell { + padding: 8px 4px; +} + +.inline-suggestion-table td.inline-suggestion-text-cell { + /* This is attempting to align the text in the textarea with the text on + the surrounding context lines. */ + padding: 0 8px 0 11px; +} + +.inline-suggestion-view { + padding: 4px 0; + white-space: pre-wrap; + background: {$lightgreybackground}; + margin: 0 -12px 8px; + border-width: 1px 0; + border-style: solid; + border-color: {$lightgreyborder}; +} + +.diff-1up-simple-table { + width: 100%; + table-layout: fixed; +} + +.diff-1up-simple-table > tbody > tr > td { + padding-left: 12px; + padding-right: 12px; +} diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js index 70bd92e6bc..212d19f5c6 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -11,6 +11,7 @@ * phabricator-diff-inline * phabricator-diff-path-view * phuix-button-view + * javelin-external-editor-link-engine * @javelin */ @@ -33,7 +34,7 @@ JX.install('DiffChangeset', { this._pathParts = data.pathParts; this._icon = data.icon; - this._editorURI = data.editorURI; + this._editorURITemplate = data.editorURITemplate; this._editorConfigureURI = data.editorConfigureURI; this._showPathURI = data.showPathURI; this._showDirectoryURI = data.showDirectoryURI; @@ -87,7 +88,7 @@ JX.install('DiffChangeset', { _changesetList: null, _icon: null, - _editorURI: null, + _editorURITemplate: null, _editorConfigureURI: null, _showPathURI: null, _showDirectoryURI: null, @@ -102,8 +103,8 @@ JX.install('DiffChangeset', { _isSelected: false, _viewMenu: null, - getEditorURI: function() { - return this._editorURI; + getEditorURITemplate: function() { + return this._editorURITemplate; }, getEditorConfigureURI: function() { @@ -453,9 +454,24 @@ JX.install('DiffChangeset', { var blocks = []; var block; var ii; + var parent_node = null; for (ii = 0; ii < rows.length; ii++) { var type = this._getRowType(rows[ii]); + // This row might be part of a diff inside an inline comment, showing + // an inline edit suggestion. Before we accept it as a possible target + // for selection, make sure it's a child of the right parent. + + if (parent_node === null) { + parent_node = rows[ii].parentNode; + } + + if (type !== null) { + if (rows[ii].parentNode !== parent_node) { + type = null; + } + } + if (!block || (block.type !== type)) { block = { type: type, @@ -761,14 +777,14 @@ JX.install('DiffChangeset', { return inline; }, - newInlineReply: function(original, text) { + newInlineReply: function(original, state) { var inline = new JX.DiffInline() .setChangeset(this) .bindToReply(original); this._inlines.push(inline); - inline.create(text); + inline.create(state); return inline; }, diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 0397354859..7293f89fe0 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -355,48 +355,11 @@ JX.install('DiffChangesetList', { // lines) reply on the old file. if (cursor.type == 'change') { - var origin = cursor.nodes.begin; - var target = cursor.nodes.end; + var cells = this._getLineNumberCellsForChangeBlock( + cursor.nodes.begin, + cursor.nodes.end); - // The "origin" and "target" are entire rows, but we need to find - // a range of "" nodes to actually create an inline, so go - // fishing. - - var old_list = []; - var new_list = []; - - var row = origin; - while (row) { - var header = row.firstChild; - while (header) { - if (this.getLineNumberFromHeader(header)) { - if (header.className.indexOf('old') !== -1) { - old_list.push(header); - } else if (header.className.indexOf('new') !== -1) { - new_list.push(header); - } - } - header = header.nextSibling; - } - - if (row == target) { - break; - } - - row = row.nextSibling; - } - - var use_list; - if (new_list.length) { - use_list = new_list; - } else { - use_list = old_list; - } - - var src = use_list[0]; - var dst = use_list[use_list.length - 1]; - - cursor.changeset.newInlineForRange(src, dst); + cursor.changeset.newInlineForRange(cells.src, cells.dst); this.setFocus(null); return; @@ -407,6 +370,51 @@ JX.install('DiffChangesetList', { this._warnUser(pht('You must select a comment or change to reply to.')); }, + _getLineNumberCellsForChangeBlock: function(origin, target) { + // The "origin" and "target" are entire rows, but we need to find + // a range of cell nodes to actually create an inline, so go + // fishing. + + var old_list = []; + var new_list = []; + + var row = origin; + while (row) { + var header = row.firstChild; + while (header) { + if (this.getLineNumberFromHeader(header)) { + if (header.className.indexOf('old') !== -1) { + old_list.push(header); + } else if (header.className.indexOf('new') !== -1) { + new_list.push(header); + } + } + header = header.nextSibling; + } + + if (row == target) { + break; + } + + row = row.nextSibling; + } + + var use_list; + if (new_list.length) { + use_list = new_list; + } else { + use_list = old_list; + } + + var src = use_list[0]; + var dst = use_list[use_list.length - 1]; + + return { + src: src, + dst: dst + }; + }, + _onkeyedit: function() { var cursor = this._cursorItem; @@ -505,7 +513,7 @@ JX.install('DiffChangesetList', { return changeset; }, - _onkeyopeneditor: function() { + _onkeyopeneditor: function(e) { var pht = this.getTranslations(); var changeset = this._getChangesetForKeyCommand(); @@ -514,13 +522,61 @@ JX.install('DiffChangesetList', { return; } - var editor_uri = changeset.getEditorURI(); + this._openEditor(changeset); + }, - if (editor_uri === null) { + _openEditor: function(changeset) { + var pht = this.getTranslations(); + + var editor_template = changeset.getEditorURITemplate(); + if (editor_template === null) { this._warnUser(pht('No external editor is configured.')); return; } + var line = null; + + // See PHI1749. We aren't exactly sure what the user intends when they + // use the keyboard to select a change block and then activate the + // "Open in Editor" function: they might mean to open the old or new + // offset, and may have the old or new state (or some other state) in + // their working copy. + + // For now, pick: the new state line number if one exists; or the old + // state line number if one does not. If nothing else, this behavior is + // simple. + + // If there's a document engine, just open the file to the first line. + // We currently can not map display blocks to source lines. + + // If there's an inline, open the file to that line. + + if (changeset.getResponseDocumentEngineKey() === null) { + var cursor = this._cursorItem; + if (cursor && (cursor.changeset === changeset)) { + if (cursor.type == 'change') { + var cells = this._getLineNumberCellsForChangeBlock( + cursor.nodes.begin, + cursor.nodes.end); + line = this.getLineNumberFromHeader(cells.src); + } + + if (cursor.type === 'comment') { + var inline = cursor.target; + line = inline.getLineNumber(); + } + } + } + + var variables = { + l: line || 1 + }; + + var editor_uri = new JX.ExternalEditorLinkEngine() + .setTemplate(editor_template) + .setVariables(variables) + .newURI(); + JX.$U(editor_uri).go(); }, @@ -1029,10 +1085,21 @@ JX.install('DiffChangesetList', { changeset.getShowPathURI()) .setKeyCommand('d'); - var editor_uri = changeset.getEditorURI(); - if (editor_uri !== null) { - add_link('fa-i-cursor', pht('Open in Editor'), editor_uri, true) - .setKeyCommand('\\'); + var editor_template = changeset.getEditorURITemplate(); + if (editor_template !== null) { + var editor_item = new JX.PHUIXActionView() + .setIcon('fa-i-cursor') + .setName(pht('Open in Editor')) + .setKeyCommand('\\') + .setHandler(function(e) { + + changeset_list._openEditor(changeset); + + e.prevent(); + menu.close(); + }); + + list.addItem(editor_item); } else { var configure_uri = changeset.getEditorConfigureURI(); if (configure_uri !== null) { @@ -2410,7 +2477,7 @@ JX.install('DiffChangesetList', { switch (action) { case 'save': - inline.save(e.getTarget()); + inline.save(); break; case 'cancel': inline.cancel(); diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index cfec020544..1e700855f1 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -19,7 +19,7 @@ JX.install('DiffInline', { _displaySide: null, _isNewFile: null, _replyToCommentPHID: null, - _originalText: null, + _originalState: null, _snippet: null, _menuItems: null, _documentEngineKey: null, @@ -42,7 +42,7 @@ JX.install('DiffInline', { _editRow: null, _undoRow: null, _undoType: null, - _undoText: null, + _undoState: null, _draftRequest: null, _skipFocus: false, @@ -51,6 +51,7 @@ JX.install('DiffInline', { _startOffset: null, _endOffset: null, _isSelected: false, + _canSuggestEdit: false, bindToRow: function(row) { this._row = row; @@ -76,12 +77,6 @@ JX.install('DiffInline', { this._number = parseInt(data.number, 10); this._length = parseInt(data.length, 10); - var original = '' + data.original; - if (original.length) { - this._originalText = original; - } else { - this._originalText = null; - } this._isNewFile = data.isNewFile; this._replyToCommentPHID = data.replyToCommentPHID; @@ -314,10 +309,6 @@ JX.install('DiffInline', { return this._hasMenuAction('collapse'); }, - getRawText: function() { - return this._originalText; - }, - _newRow: function() { var attributes = { sigil: 'inline-row' @@ -332,7 +323,7 @@ JX.install('DiffInline', { this._phid = null; this._isCollapsed = false; - this._originalText = null; + this._originalState = null; return row; }, @@ -404,7 +395,7 @@ JX.install('DiffInline', { this._didUpdate(); }, - create: function(text) { + create: function(content_state) { var changeset = this.getChangeset(); if (!this._documentEngineKey) { this._documentEngineKey = changeset.getResponseDocumentEngineKey(); @@ -412,7 +403,7 @@ JX.install('DiffInline', { var uri = this._getInlineURI(); var handler = JX.bind(this, this._oncreateresponse); - var data = this._newRequestData('new', text); + var data = this._newRequestData('new', content_state); this.setLoading(true); @@ -421,22 +412,33 @@ JX.install('DiffInline', { .send(); }, + _getContentState: function() { + var state; + + if (this._editRow) { + state = this._readFormState(this._editRow); + } else { + state = this._originalState; + } + + return state; + }, + reply: function(with_quote) { this._closeMenu(); - var text; + var content_state = this._newContentState(); if (with_quote) { - text = this.getRawText(); + var text = this._getContentState().text; text = '> ' + text.replace(/\n/g, '\n> ') + '\n\n'; - } else { - text = ''; + content_state.text = text; } var changeset = this.getChangeset(); - return changeset.newInlineReply(this, text); + return changeset.newInlineReply(this, content_state); }, - edit: function(text, skip_focus) { + edit: function(content_state, skip_focus) { this._closeMenu(); this._skipFocus = !!skip_focus; @@ -456,7 +458,7 @@ JX.install('DiffInline', { var uri = this._getInlineURI(); var handler = JX.bind(this, this._oneditresponse); - var data = this._newRequestData('edit', text || null); + var data = this._newRequestData('edit', content_state); this.setLoading(true); @@ -545,13 +547,12 @@ JX.install('DiffInline', { return this; }, - _newRequestData: function(operation, text) { + _newRequestData: function(operation, content_state) { var data = { op: operation, is_new: this.isNewFile(), on_right: ((this.getDisplaySide() == 'right') ? 1 : 0), - renderer: this.getChangeset().getRendererKey(), - text: text || null + renderer: this.getChangeset().getRendererKey() }; if (operation === 'new') { @@ -574,6 +575,11 @@ JX.install('DiffInline', { JX.copy(data, edit_data); } + if (content_state) { + data.hasContentState = 1; + JX.copy(data, content_state); + } + return data; }, @@ -596,6 +602,8 @@ JX.install('DiffInline', { _readInlineState: function(state) { this._id = state.id; + this._originalState = state.contentState; + this._canSuggestEdit = state.canSuggestEdit; }, _ondeleteresponse: function() { @@ -608,14 +616,14 @@ JX.install('DiffInline', { // If there's an existing editor, remove it. This happens when you // delete a comment from the comment preview area. In this case, we // read and preserve the text so "Undo" restores it. - var text; + var state = null; if (this._editRow) { - text = this._readText(this._editRow); + state = this._readFormState(this._editRow); JX.DOM.remove(this._editRow); this._editRow = null; } - this._drawUndeleteRows(text); + this._drawUndeleteRows(state); this.setLoading(false); this.setDeleted(true); @@ -623,21 +631,21 @@ JX.install('DiffInline', { this._didUpdate(); }, - _drawUndeleteRows: function(text) { + _drawUndeleteRows: function(content_state) { this._undoType = 'undelete'; - this._undoText = text || null; + this._undoState = content_state || null; return this._drawUndoRows('undelete', this._row); }, - _drawUneditRows: function(text) { + _drawUneditRows: function(content_state) { this._undoType = 'unedit'; - this._undoText = text; + this._undoState = content_state; - return this._drawUndoRows('unedit', null, text); + return this._drawUndoRows('unedit', null); }, - _drawUndoRows: function(mode, cursor, text) { + _drawUndoRows: function(mode, cursor) { var templates = this.getChangeset().getUndoTemplates(); var template; @@ -648,7 +656,7 @@ JX.install('DiffInline', { } template = JX.$H(template).getNode(); - this._undoRow = this._drawRows(template, cursor, mode, text); + this._undoRow = this._drawRows(template, cursor, mode); }, _drawContentRows: function(rows) { @@ -658,9 +666,12 @@ JX.install('DiffInline', { _drawEditRows: function(rows) { this.setEditing(true); this._editRow = this._drawRows(rows, null, 'edit'); + + this._drawSuggestionState(this._editRow); + this.setHasSuggestion(this._originalState.hasSuggestion); }, - _drawRows: function(rows, cursor, type, text) { + _drawRows: function(rows, cursor, type) { var first_row = JX.DOM.scry(rows, 'tr')[0]; var row = first_row; var anchor = cursor || this._row; @@ -695,7 +706,7 @@ JX.install('DiffInline', { var textareas = JX.DOM.scry( row, 'textarea', - 'differential-inline-comment-edit-textarea'); + 'inline-content-text'); if (textareas.length) { var area = textareas[0]; area.focus(); @@ -713,14 +724,102 @@ JX.install('DiffInline', { return result_row; }, - save: function(form) { + _drawSuggestionState: function(row) { + if (this._canSuggestEdit) { + var button = this._getSuggestionButton(); + var node = button.getNode(); + + // As a side effect of form submission, the button may become + // visually disabled. Re-enable it. This is a bit hacky. + JX.DOM.alterClass(node, 'disabled', false); + node.disabled = false; + + var container = JX.DOM.find(row, 'div', 'inline-edit-buttons'); + container.appendChild(node); + } + }, + + _getSuggestionButton: function() { + if (!this._suggestionButton) { + var button = new JX.PHUIXButtonView() + .setIcon('fa-pencil-square-o') + .setColor('grey'); + + var node = button.getNode(); + JX.DOM.alterClass(node, 'inline-button-left', true); + + var onclick = JX.bind(this, this._onSuggestEdit); + JX.DOM.listen(node, 'click', null, onclick); + + this._suggestionButton = button; + } + + return this._suggestionButton; + }, + + _onSuggestEdit: function(e) { + e.kill(); + + this.setHasSuggestion(!this.getHasSuggestion()); + + // The first time the user actually clicks the button and enables + // suggestions for a given editor state, fill the input with the + // underlying text if there isn't any text yet. + if (this.getHasSuggestion()) { + if (this._editRow) { + var node = this._getSuggestionNode(this._editRow); + if (node) { + if (!node.value.length) { + var data = JX.Stratcom.getData(node); + if (!data.hasSetDefault) { + data.hasSetDefault = true; + node.value = data.defaultText; + node.rows = Math.max(3, node.value.split('\n').length); + } + } + } + } + } + + // Save the "hasSuggestion" part of the content state. + this.triggerDraft(); + }, + + setHasSuggestion: function(has_suggestion) { + this._hasSuggestion = has_suggestion; + + var button = this._getSuggestionButton(); + var pht = this.getChangeset().getChangesetList().getTranslations(); + if (has_suggestion) { + button + .setIcon('fa-times') + .setText(pht('Discard Edit')); + } else { + button + .setIcon('fa-plus') + .setText(pht('Suggest Edit')); + } + + if (this._editRow) { + JX.DOM.alterClass(this._editRow, 'has-suggestion', has_suggestion); + } + }, + + getHasSuggestion: function() { + return this._hasSuggestion; + }, + + save: function() { var handler = JX.bind(this, this._onsubmitresponse); this.setLoading(true); - JX.Workflow.newFromForm(form) - .setHandler(handler) - .start(); + var uri = this._getInlineURI(); + var data = this._newRequestData('save', this._getContentState()); + + new JX.Request(uri, handler) + .setData(data) + .send(); }, undo: function() { @@ -740,8 +839,8 @@ JX.install('DiffInline', { .send(); } - if (this._undoText !== null) { - this.edit(this._undoText); + if (this._undoState !== null) { + this.edit(this._undoState); } }, @@ -751,18 +850,20 @@ JX.install('DiffInline', { }, cancel: function() { - var text = this._readText(this._editRow); + var state = this._readFormState(this._editRow); JX.DOM.remove(this._editRow); this._editRow = null; - if (text && text.length && (text != this._originalText)) { - this._drawUneditRows(text); + var is_empty = this._isVoidContentState(state); + var is_same = this._isSameContentState(state, this._originalState); + if (!is_empty && !is_same) { + this._drawUneditRows(state); } // If this was an empty box and we typed some text and then hit cancel, // don't show the empty concrete inline. - if (!this._originalText) { + if (!this._isVoidContentState(this._originalState)) { this.setInvisible(true); } else { this.setInvisible(false); @@ -773,7 +874,7 @@ JX.install('DiffInline', { // text ("A") to the server as the current persistent state. var uri = this._getInlineURI(); - var data = this._newRequestData('cancel', this._originalText); + var data = this._newRequestData('cancel', this._originalState); var handler = JX.bind(this, this._onCancelResponse); this.setLoading(true); @@ -792,7 +893,7 @@ JX.install('DiffInline', { // If the comment was empty when we started editing it (there's no // original text) and empty when we finished editing it (there's no // undo row), just delete the comment. - if (!this._originalText && !this.isUndo()) { + if (this._isVoidContentState(this._originalState) && !this.isUndo()) { this.setDeleted(true); JX.DOM.remove(this._row); @@ -802,18 +903,34 @@ JX.install('DiffInline', { } }, - _readText: function(row) { - var textarea; + _readFormState: function(row) { + var state = this._newContentState(); + + var node; + try { - textarea = JX.DOM.find( - row, - 'textarea', - 'differential-inline-comment-edit-textarea'); + node = JX.DOM.find(row, 'textarea', 'inline-content-text'); + state.text = node.value; + } catch (ex) { + // Ignore. + } + + node = this._getSuggestionNode(row); + if (node) { + state.suggestionText = node.value; + } + + state.hasSuggestion = this.getHasSuggestion(); + + return state; + }, + + _getSuggestionNode: function(row) { + try { + return JX.DOM.find(row, 'textarea', 'inline-content-suggestion'); } catch (ex) { return null; } - - return textarea.value; }, _onsubmitresponse: function(response) { @@ -920,16 +1037,19 @@ JX.install('DiffInline', { return null; } - var text = this._readText(this._editRow); - if (text === null) { + var state = this._readFormState(this._editRow); + if (this._isVoidContentState(state)) { return null; } - return { + var draft_data = { op: 'draft', id: this.getID(), - text: text }; + + JX.copy(draft_data, state); + + return draft_data; }, triggerDraft: function() { @@ -1035,8 +1155,27 @@ JX.install('DiffInline', { if (this._menu) { this._menu.close(); } - } + }, + _newContentState: function() { + return { + text: '', + suggestionText: '', + hasSuggestion: false + }; + }, + + _isVoidContentState: function(state) { + return (!state.text.length && !state.suggestionText.length); + }, + + _isSameContentState: function(u, v) { + return ( + ((u === null) === (v === null)) && + (u.text === v.text) && + (u.suggestionText === v.suggestionText) && + (u.hasSuggestion === v.hasSuggestion)); + } } }); diff --git a/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js b/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js new file mode 100644 index 0000000000..c43f1c360a --- /dev/null +++ b/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js @@ -0,0 +1,41 @@ +/** + * @provides javelin-external-editor-link-engine + * @requires javelin-install + * @javelin + */ + +JX.install('ExternalEditorLinkEngine', { + + properties: { + template: null, + variables: null + }, + + members: { + newURI: function() { + var template = this.getTemplate(); + var variables = this.getVariables(); + + var parts = []; + for (var ii = 0; ii < template.length; ii++) { + var part = template[ii]; + var value = part.value; + + if (part.type === 'literal') { + parts.push(value); + continue; + } + + if (part.type === 'variable') { + if (variables.hasOwnProperty(value)) { + var replacement = variables[value]; + replacement = encodeURIComponent(replacement); + parts.push(replacement); + } + } + } + + return parts.join(''); + } + } +}); diff --git a/webroot/rsrc/js/core/behavior-line-linker.js b/webroot/rsrc/js/core/behavior-line-linker.js index 9042cb0912..fd8510646e 100644 --- a/webroot/rsrc/js/core/behavior-line-linker.js +++ b/webroot/rsrc/js/core/behavior-line-linker.js @@ -4,6 +4,7 @@ * javelin-stratcom * javelin-dom * javelin-history + * javelin-external-editor-link-engine */ JX.behavior('phabricator-line-linker', function() { @@ -170,32 +171,19 @@ JX.behavior('phabricator-line-linker', function() { if (editor_link) { var data = JX.Stratcom.getData(editor_link); - var template = data.template; var variables = { l: parseInt(Math.min(o, t), 10), }; - var parts = []; - for (var ii = 0; ii < template.length; ii++) { - var part = template[ii]; - var value = part.value; + var template = data.template; - if (part.type === 'literal') { - parts.push(value); - continue; - } + var editor_uri = new JX.ExternalEditorLinkEngine() + .setTemplate(template) + .setVariables(variables) + .newURI(); - if (part.type === 'variable') { - if (variables.hasOwnProperty(value)) { - var replacement = variables[value]; - replacement = encodeURIComponent(replacement); - parts.push(replacement); - } - } - } - - editor_link.href = parts.join(''); + editor_link.href = editor_uri; } });