From 37ffb71c4d5efdb25955105f627849690af0ba4f Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Jul 2020 14:56:39 -0700 Subject: [PATCH 1/8] In source views, wrap display tabs in "user-select: all" to improve cursor selection behavior Summary: Ref T2495. See PHI1814. Currently, Phabricator replaces tabs with spaces when rendering diffs. This may or may not be the best behavior in the long term, but it gives us more control over expansion of tabs than using tab literals. However, one downside is that you can use your mouse cursor to select "half a tab", and can't use your mouse cursor to distinguish between tabs and spaces. Although you probably shouldn't be doing this, this behavior is less accurate/correct than selecting the entire block as a single unit. A specific correctness issue with this behavior is that the entire block is copied to the clipboard as a tab literal if you select any of it, so two different visual selection ranges can produce the same clipboard content. This particular behavior can be improved with "user-select: all", to instruct browsers to select the entire element as a single logical element. Now, selecting part of the tab selects the whole thing, as though it were really a tab literal. (Some future change might abandon this approach and opt to use real tab literals with "tab-size" CSS, but we lose some ability to control alignment behavior if we do that and it doesn't have any obvious advantages over this approach other than cursor selection behavior.) Test Plan: - In Safari and Firefox, dragged text to select a whitespace-expanded tab literal. Saw browsers select the whole sequence as though it were a single tab. - In Chorme, this also mostly works, but there's some glitchiness and flickering. I think this is still a net improvement, it's just not as smooth as Safari and Firefox. Maniphest Tasks: T2495 Differential Revision: https://secure.phabricator.com/D21419 --- resources/celerity/map.php | 12 ++++++------ .../parser/DifferentialChangesetParser.php | 7 +++++++ .../__tests__/DifferentialTabReplacementTestCase.php | 4 ++-- webroot/rsrc/css/core/syntax.css | 5 +++++ 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d10f117847..77242a1384 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => 'da792a0f', + 'core.pkg.css' => '2e175364', 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', @@ -115,7 +115,7 @@ return array( 'rsrc/css/application/uiexample/example.css' => 'b4795059', 'rsrc/css/core/core.css' => '1b29ed61', 'rsrc/css/core/remarkup.css' => '7d3ebc86', - 'rsrc/css/core/syntax.css' => '548567f6', + 'rsrc/css/core/syntax.css' => '98fdb17e', 'rsrc/css/core/z-index.css' => 'ac3bfcd4', 'rsrc/css/diviner/diviner-shared.css' => '4bd263b0', 'rsrc/css/font/font-awesome.css' => '3883938a', @@ -909,7 +909,7 @@ return array( 'sprite-login-css' => '18b368a6', 'sprite-tokens-css' => 'f1896dc5', 'syntax-default-css' => '055fc231', - 'syntax-highlighting-css' => '548567f6', + 'syntax-highlighting-css' => '98fdb17e', 'tokens-css' => 'ce5a50bd', 'trigger-rule' => '41b7b4f6', 'trigger-rule-control' => '5faf27b9', @@ -1422,9 +1422,6 @@ return array( 'phuix-autocomplete', 'javelin-mask', ), - '548567f6' => array( - 'syntax-default-css', - ), '55a24e84' => array( 'javelin-install', 'javelin-dom', @@ -1803,6 +1800,9 @@ return array( 'javelin-request', 'javelin-util', ), + '98fdb17e' => array( + 'syntax-default-css', + ), '995f5102' => array( 'javelin-install', 'javelin-util', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index b7ea61b7a9..45da82118f 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1600,6 +1600,13 @@ final class DifferentialChangesetParser extends Phobject { 'span', array( 'data-copy-text' => "\t", + + // See PHI1814. Mark this as a single logical tab for the purposes + // of text selection behavior: when the user drags their mouse over + // the character sequence, we'd like the whole thing to select as + // a single unit. + + 'class' => 'logical-tab', ), str_repeat(' ', $ii)); $tag = phutil_string_cast($tag); diff --git a/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php b/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php index d63978cb61..494170c668 100644 --- a/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php +++ b/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php @@ -4,8 +4,8 @@ final class DifferentialTabReplacementTestCase extends PhabricatorTestCase { public function testTabReplacement() { - $tab1 = " "; - $tab2 = " "; + $tab1 = " "; + $tab2 = " "; $cat = "\xF0\x9F\x90\xB1"; diff --git a/webroot/rsrc/css/core/syntax.css b/webroot/rsrc/css/core/syntax.css index 78aa83dbdc..822e1777d6 100644 --- a/webroot/rsrc/css/core/syntax.css +++ b/webroot/rsrc/css/core/syntax.css @@ -39,3 +39,8 @@ span.crossreference-item { color: #ffffff; cursor: default; } + +.logical-tab { + user-select: all; + -webkit-user-select: all; +} From 0ed5569e9f7b4ed7220ac559144fdb42488a68e9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Jul 2020 19:42:45 -0700 Subject: [PATCH 2/8] Likely, fix a warning when rendering modified coverage Summary: See PHI1819. This structure may have `null` elements. Test Plan: Will confirm user reproduction case. Differential Revision: https://secure.phabricator.com/D21420 --- .../differential/parser/DifferentialChangesetParser.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 45da82118f..1cc1c3ad9d 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1327,6 +1327,10 @@ final class DifferentialChangesetParser extends Phobject { $not_covered = 0; foreach ($this->new as $k => $new) { + if ($new === null) { + continue; + } + if (!$new['line']) { continue; } From 8f9ba4852861e7628a4e8052afd982efe2817d67 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 22 Jul 2020 11:54:06 -0700 Subject: [PATCH 3/8] Fix an issue with destruction of Revision and Diff objects with viewstates Summary: See . These queries aren't actually constructed properly, and destroying a revision or diff with viewstates currently fails. Test Plan: Used `bin/remove destroy Dxxx` to destroy a revision with viewstates (this also destroys the associated diffs). Differential Revision: https://secure.phabricator.com/D21421 --- src/applications/differential/storage/DifferentialDiff.php | 3 ++- src/applications/differential/storage/DifferentialRevision.php | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 747407ce75..f5cb91c56b 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -737,9 +737,10 @@ final class DifferentialDiff $prop->delete(); } - $viewstates = id(new DifferentialViewStateQuery()) + $viewstate_query = id(new DifferentialViewStateQuery()) ->setViewer($viewer) ->withObjectPHIDs(array($this->getPHID())); + $viewstates = new PhabricatorQueryIterator($viewstate_query); foreach ($viewstates as $viewstate) { $viewstate->delete(); } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 2ab9d59821..15cf219f7b 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -1033,9 +1033,10 @@ final class DifferentialRevision extends DifferentialDAO $dummy_path->getTableName(), $this->getID()); - $viewstates = id(new DifferentialViewStateQuery()) + $viewstate_query = id(new DifferentialViewStateQuery()) ->setViewer($viewer) ->withObjectPHIDs(array($this->getPHID())); + $viewstates = new PhabricatorQueryIterator($viewstate_query); foreach ($viewstates as $viewstate) { $viewstate->delete(); } From fcb75d0503c6d3e21461cdee91c01022bb6b0bab Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jul 2020 07:35:57 -0700 Subject: [PATCH 4/8] Fix an issue where prose diffing may fail after hitting the PCRE backtracking limit Summary: Fixes T13554. For certain prose diff inputs and PCRE backtracking limits, this regular expression may back track too often and fail. A characteristic input is "x x x x ...", i.e. many sequences where `(.*?)\s*\z` looks like it may be able to match but actually can not. I think writing an expression which has all the behavior we'd like without this backtracking issue isn't trivial (at least, I don't think I know how to do it offhand); just use a strategy based on "trim()" insetad, which avoids any PCRE complexities here. Test Plan: Locally, this passes the "x x x ..." test which the previous code failed. I'm not including that test because it won't reproduce across values of "pcre.backtrac_limit", PCRE versions, etc. Maniphest Tasks: T13554 Differential Revision: https://secure.phabricator.com/D21422 --- .../prose/PhutilProseDifferenceEngine.php | 51 +++++++++++++------ .../__tests__/PhutilProseDiffTestCase.php | 33 ++++++++++++ 2 files changed, 68 insertions(+), 16 deletions(-) diff --git a/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php b/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php index a5494aa05f..b8bee73961 100644 --- a/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php +++ b/src/infrastructure/diff/prose/PhutilProseDifferenceEngine.php @@ -142,22 +142,9 @@ final class PhutilProseDifferenceEngine extends Phobject { } if ($level < 2) { - // Split pieces into separate text and whitespace sections: make one - // piece out of all the whitespace at the beginning, one piece out of - // all the actual text in the middle, and one piece out of all the - // whitespace at the end. - - $matches = null; - preg_match('/^(\s*)(.*?)(\s*)\z/s', $result, $matches); - - if (strlen($matches[1])) { - $results[] = $matches[1]; - } - if (strlen($matches[2])) { - $results[] = $matches[2]; - } - if (strlen($matches[3])) { - $results[] = $matches[3]; + $trimmed_pieces = $this->trimApart($result); + foreach ($trimmed_pieces as $trimmed_piece) { + $results[] = $trimmed_piece; } } else { $results[] = $result; @@ -272,4 +259,36 @@ final class PhutilProseDifferenceEngine extends Phobject { return $blocks; } + public static function trimApart($input) { + // Split pieces into separate text and whitespace sections: make one + // piece out of all the whitespace at the beginning, one piece out of + // all the actual text in the middle, and one piece out of all the + // whitespace at the end. + + $parts = array(); + + $length = strlen($input); + + $corpus = ltrim($input); + $l_length = strlen($corpus); + if ($l_length !== $length) { + $parts[] = substr($input, 0, $length - $l_length); + } + + $corpus = rtrim($corpus); + $lr_length = strlen($corpus); + + if ($lr_length) { + $parts[] = $corpus; + } + + if ($lr_length !== $l_length) { + // NOTE: This will be a negative value; we're slicing from the end of + // the input string. + $parts[] = substr($input, $lr_length - $l_length); + } + + return $parts; + } + } diff --git a/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php b/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php index 823fef650a..d2e11cac5b 100644 --- a/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php +++ b/src/infrastructure/diff/prose/__tests__/PhutilProseDiffTestCase.php @@ -3,6 +3,39 @@ final class PhutilProseDiffTestCase extends PhabricatorTestCase { + public function testTrimApart() { + $map = array( + '' => array(), + 'a' => array('a'), + ' a ' => array( + ' ', + 'a', + ' ', + ), + ' a' => array( + ' ', + 'a', + ), + 'a ' => array( + 'a', + ' ', + ), + ' a b ' => array( + ' ', + 'a b', + ' ', + ), + ); + + foreach ($map as $input => $expect) { + $actual = PhutilProseDifferenceEngine::trimApart($input); + $this->assertEqual( + $expect, + $actual, + pht('Trim Apart: %s', $input)); + } + } + public function testProseDiffsDistance() { $this->assertProseParts( '', From 5f0535934d72f0c05ae56c653dd8cc7dd29e3ba4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jul 2020 09:27:14 -0700 Subject: [PATCH 5/8] Manage PIDs more carefully in DaemonHandle Summary: Ref T13555. Although these callsites may not actually impact anything, it's possible for an active handle to have no PID (e.g., if the subprocess failed to start). Handle these cases more carefully. Test Plan: Started daemons, saw them run fine. See also next change. Maniphest Tasks: T13555 Differential Revision: https://secure.phabricator.com/D21424 --- .../daemon/PhutilDaemonHandle.php | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/infrastructure/daemon/PhutilDaemonHandle.php b/src/infrastructure/daemon/PhutilDaemonHandle.php index 5030f5330c..25c517b8cf 100644 --- a/src/infrastructure/daemon/PhutilDaemonHandle.php +++ b/src/infrastructure/daemon/PhutilDaemonHandle.php @@ -327,13 +327,14 @@ final class PhutilDaemonHandle extends Phobject { private function annihilateProcessGroup() { $pid = $this->getPID(); - - $pgid = posix_getpgid($pid); - if ($pid && $pgid) { - posix_kill(-$pgid, SIGTERM); - sleep($this->getKillDelay()); - posix_kill(-$pgid, SIGKILL); - $this->pid = null; + if ($pid) { + $pgid = posix_getpgid($pid); + if ($pgid) { + posix_kill(-$pgid, SIGTERM); + sleep($this->getKillDelay()); + posix_kill(-$pgid, SIGKILL); + $this->pid = null; + } } } @@ -440,7 +441,10 @@ final class PhutilDaemonHandle extends Phobject { // naturally be restarted after it exits, as though it had exited after an // unhandled exception. - posix_kill($this->getPID(), SIGINT); + $pid = $this->getPID(); + if ($pid) { + posix_kill($pid, SIGINT); + } } public function didReceiveGracefulSignal($signo) { @@ -461,7 +465,10 @@ final class PhutilDaemonHandle extends Phobject { $this->logMessage('DONE', $sigmsg, $signo); - posix_kill($this->getPID(), SIGINT); + $pid = $this->getPID(); + if ($pid) { + posix_kill($pid, SIGINT); + } } public function didReceiveTerminateSignal($signo) { From 78d1b62bb8e9b5d90b005626a1635718c5273ce3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jul 2020 09:11:30 -0700 Subject: [PATCH 6/8] Streamline handling of Futures and PIDs in daemons Summary: Ref T13555. Currently, the daemon future may resolve into a failure state immediately inside "start()", and not have a valid PID when we read it. Instead, read PIDs from the current active future in all cases, using "hasPID()" to test for the presence of a valid PID. Since we don't query the PID immediately, we no longer need to explicitly start the future. Also fix an issue where the same future could be added to the overseer pool more than once if it threw on "resolve()". In general: - Before we "resolve()" a future, detach it from the DaemonHandle: we're always done with it. - Catch exceptions on resolution and treat them the same way as subprocess resolution errors. These aren't common, but are possible in the general case. - Have DaemonHandle add futures to the future pool directly when they're created. Test Plan: - Ran daemons with intentional subprocess creation failures, saw clean recovery. - Ran daemons with intentional resolution exceptions, saw clean recovery. Maniphest Tasks: T13555 Differential Revision: https://secure.phabricator.com/D21425 --- .../daemon/PhutilDaemonHandle.php | 67 +++++++++++++------ .../daemon/PhutilDaemonOverseer.php | 9 +-- .../daemon/PhutilDaemonPool.php | 12 ---- 3 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/infrastructure/daemon/PhutilDaemonHandle.php b/src/infrastructure/daemon/PhutilDaemonHandle.php index 25c517b8cf..428a64a056 100644 --- a/src/infrastructure/daemon/PhutilDaemonHandle.php +++ b/src/infrastructure/daemon/PhutilDaemonHandle.php @@ -16,7 +16,6 @@ final class PhutilDaemonHandle extends Phobject { private $restartAt; private $busyEpoch; - private $pid; private $daemonID; private $deadline; private $heartbeat; @@ -104,7 +103,7 @@ final class PhutilDaemonHandle extends Phobject { } public function isRunning() { - return (bool)$this->future; + return (bool)$this->getFuture(); } public function isHibernating() { @@ -134,10 +133,6 @@ final class PhutilDaemonHandle extends Phobject { return (!$this->shouldRestart && !$this->isRunning()); } - public function getFuture() { - return $this->future; - } - public function update() { if (!$this->isRunning()) { if (!$this->shouldRestart) { @@ -152,11 +147,19 @@ final class PhutilDaemonHandle extends Phobject { $this->startDaemonProcess(); } - $future = $this->future; + $future = $this->getFuture(); $result = null; - if ($future->isReady()) { - $result = $future->resolve(); + $caught = null; + if ($future->canResolve()) { + $this->future = null; + try { + $result = $future->resolve(); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } } list($stdout, $stderr) = $future->read(); @@ -173,16 +176,22 @@ final class PhutilDaemonHandle extends Phobject { } } - if ($result !== null) { - list($err) = $result; + if ($result !== null || $caught !== null) { - if ($err) { - $this->logMessage('FAIL', pht('Process exited with error %s.', $err)); + if ($caught) { + $message = pht( + 'Process failed with exception: %s', + $caught->getMessage()); + $this->logMessage('FAIL', $message); } else { - $this->logMessage('DONE', pht('Process exited normally.')); - } + list($err) = $result; - $this->future = null; + if ($err) { + $this->logMessage('FAIL', pht('Process exited with error %s.', $err)); + } else { + $this->logMessage('DONE', pht('Process exited normally.')); + } + } if ($this->shouldShutdown) { $this->restartAt = null; @@ -244,8 +253,22 @@ final class PhutilDaemonHandle extends Phobject { return $this->daemonID; } - public function getPID() { - return $this->pid; + private function getFuture() { + return $this->future; + } + + private function getPID() { + $future = $this->getFuture(); + + if (!$future) { + return null; + } + + if (!$future->hasPID()) { + return null; + } + + return $future->getPID(); } private function getCaptureBufferSize() { @@ -346,10 +369,12 @@ final class PhutilDaemonHandle extends Phobject { $this->stdoutBuffer = ''; $this->hibernating = false; - $this->future = $this->newExecFuture(); - $this->future->start(); + $future = $this->newExecFuture(); + $this->future = $future; - $this->pid = $this->future->getPID(); + $pool = $this->getDaemonPool(); + $overseer = $pool->getOverseer(); + $overseer->addFutureToPool($future); } private function didReadStdout($data) { diff --git a/src/infrastructure/daemon/PhutilDaemonOverseer.php b/src/infrastructure/daemon/PhutilDaemonOverseer.php index 77f6f6a20a..974a9cf3ed 100644 --- a/src/infrastructure/daemon/PhutilDaemonOverseer.php +++ b/src/infrastructure/daemon/PhutilDaemonOverseer.php @@ -181,10 +181,6 @@ EOHELP } } - foreach ($pool->getFutures() as $future) { - $future_pool->addFuture($future); - } - if ($pool->getDaemons()) { $running_pools = true; } @@ -210,6 +206,11 @@ EOHELP exit($this->err); } + public function addFutureToPool(Future $future) { + $this->getFuturePool()->addFuture($future); + return $this; + } + private function getFuturePool() { if (!$this->futurePool) { $pool = new FuturePool(); diff --git a/src/infrastructure/daemon/PhutilDaemonPool.php b/src/infrastructure/daemon/PhutilDaemonPool.php index 50b22289a8..9cdc15228e 100644 --- a/src/infrastructure/daemon/PhutilDaemonPool.php +++ b/src/infrastructure/daemon/PhutilDaemonPool.php @@ -111,18 +111,6 @@ final class PhutilDaemonPool extends Phobject { return $this->daemons; } - public function getFutures() { - $futures = array(); - foreach ($this->getDaemons() as $daemon) { - $future = $daemon->getFuture(); - if ($future) { - $futures[] = $future; - } - } - - return $futures; - } - public function didReceiveSignal($signal, $signo) { switch ($signal) { case PhutilDaemonOverseer::SIGNAL_GRACEFUL: From a27c83757d9fd73b1d9f79b0ae4268a636f35b6f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jul 2020 12:17:56 -0700 Subject: [PATCH 7/8] Remove ancient "phd.trace" and "phd.verbose" configuration options Summary: Ref T13556. These options are very old and effectively obsoleted by "bin/phd debug [--trace]". I haven't used either option diagnostically in many years, and they aren't mentioned in the documentation. Remove them to simplify configuration, and because "phd.trace" doesn't work anyway and likely hasn't for a long time -- it has specific issues with TTY detection (see T13556). Test Plan: Grepped for "phd.trace" and "phd.verbose". Ran "bin/phd debug [--trace]" and saw verbose/trace output. Maniphest Tasks: T13556 Differential Revision: https://secure.phabricator.com/D21426 --- .../check/PhabricatorDaemonsSetupCheck.php | 21 ++++++++---- .../PhabricatorExtraConfigSetupCheck.php | 6 ++++ .../option/PhabricatorPHDConfigOptions.php | 32 ------------------- .../PhabricatorDaemonManagementWorkflow.php | 4 +-- .../workers/PhabricatorTaskmasterDaemon.php | 2 +- 5 files changed, 23 insertions(+), 42 deletions(-) diff --git a/src/applications/config/check/PhabricatorDaemonsSetupCheck.php b/src/applications/config/check/PhabricatorDaemonsSetupCheck.php index 331a4f0908..7ef44dc384 100644 --- a/src/applications/config/check/PhabricatorDaemonsSetupCheck.php +++ b/src/applications/config/check/PhabricatorDaemonsSetupCheck.php @@ -8,14 +8,21 @@ final class PhabricatorDaemonsSetupCheck extends PhabricatorSetupCheck { protected function executeChecks() { - $task_daemon = id(new PhabricatorDaemonLogQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE) - ->withDaemonClasses(array('PhabricatorTaskmasterDaemon')) - ->setLimit(1) - ->execute(); + try { + $task_daemons = id(new PhabricatorDaemonLogQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE) + ->withDaemonClasses(array('PhabricatorTaskmasterDaemon')) + ->setLimit(1) + ->execute(); - if (!$task_daemon) { + $no_daemons = !$task_daemons; + } catch (Exception $ex) { + // Just skip this warning if the query fails for some reason. + $no_daemons = false; + } + + if ($no_daemons) { $doc_href = PhabricatorEnv::getDoclink('Managing Daemons with phd'); $summary = pht( diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index d863c928b7..1de39f3468 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -322,6 +322,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'directly supported. Prefixes and other strings may be customized with '. '"translation.override".'); + $phd_reason = pht( + 'Use "bin/phd debug ..." to get a detailed daemon execution log.'); + $ancient_config += array( 'phid.external-loaders' => pht( @@ -539,6 +542,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'phd.pid-directory' => pht( 'Phabricator daemons no longer use PID files.'), + + 'phd.trace' => $phd_reason, + 'phd.verbose' => $phd_reason, ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorPHDConfigOptions.php b/src/applications/config/option/PhabricatorPHDConfigOptions.php index 7a1d39e617..259660d562 100644 --- a/src/applications/config/option/PhabricatorPHDConfigOptions.php +++ b/src/applications/config/option/PhabricatorPHDConfigOptions.php @@ -43,22 +43,6 @@ final class PhabricatorPHDConfigOptions "configuration changes are picked up by the daemons ". "automatically, but pool sizes can not be changed without a ". "restart.")), - $this->newOption('phd.verbose', 'bool', false) - ->setLocked(true) - ->setBoolOptions( - array( - pht('Verbose mode'), - pht('Normal mode'), - )) - ->setSummary(pht("Launch daemons in 'verbose' mode by default.")) - ->setDescription( - pht( - "Launch daemons in 'verbose' mode by default. This creates a lot ". - "of output, but can help debug issues. Daemons launched in debug ". - "mode with '%s' are always launched in verbose mode. ". - "See also '%s'.", - 'phd debug', - 'phd.trace')), $this->newOption('phd.user', 'string', null) ->setLocked(true) ->setSummary(pht('System user to run daemons as.')) @@ -68,22 +52,6 @@ final class PhabricatorPHDConfigOptions 'user will own the working copies of any repositories that '. 'Phabricator imports or manages. This option is new and '. 'experimental.')), - $this->newOption('phd.trace', 'bool', false) - ->setLocked(true) - ->setBoolOptions( - array( - pht('Trace mode'), - pht('Normal mode'), - )) - ->setSummary(pht("Launch daemons in 'trace' mode by default.")) - ->setDescription( - pht( - "Launch daemons in 'trace' mode by default. This creates an ". - "ENORMOUS amount of output, but can help debug issues. Daemons ". - "launched in debug mode with '%s' are always launched in ". - "trace mode. See also '%s'.", - 'phd debug', - 'phd.verbose')), $this->newOption('phd.garbage-collection', 'wild', array()) ->setLocked(true) ->setLockedMessage( diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php index 05d94e218d..b9645323c2 100644 --- a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php +++ b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php @@ -116,11 +116,11 @@ abstract class PhabricatorDaemonManagementWorkflow $trace = PhutilArgumentParser::isTraceModeEnabled(); $flags = array(); - if ($trace || PhabricatorEnv::getEnvConfig('phd.trace')) { + if ($trace) { $flags[] = '--trace'; } - if ($debug || PhabricatorEnv::getEnvConfig('phd.verbose')) { + if ($debug) { $flags[] = '--verbose'; } diff --git a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php index c2276843db..12b06131d8 100644 --- a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php +++ b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php @@ -24,7 +24,7 @@ final class PhabricatorTaskmasterDaemon extends PhabricatorDaemon { if ($ex) { if ($ex instanceof PhabricatorWorkerPermanentFailureException) { // NOTE: Make sure these reach the daemon log, even when not - // running in "phd.verbose" mode. See T12803 for discussion. + // running in verbose mode. See T12803 for discussion. $log_exception = new PhutilProxyException( pht( 'Task "%s" encountered a permanent failure and was '. From 017ef1927c3e1172ea9ab21ba31f6593418db4b7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Jul 2020 13:35:12 -0700 Subject: [PATCH 8/8] Revert use of "user-select: all" to modify tab selection behavior Summary: Reverts D21419. See PHI1814. Previously, I used "user-select: all" to group sequences of spaces for selection. However, this has a side effect: the sequence is now selected with a single click. I didn't read the docuementation on the CSS property thoroughly and missed this in testing, since I was focused on drag-selection behavior. This behavior is enough of a net negative that I think we're in a worse state overall; revert it. Test Plan: Straight revert. Differential Revision: https://secure.phabricator.com/D21429 --- resources/celerity/map.php | 12 ++++++------ .../parser/DifferentialChangesetParser.php | 7 ------- .../__tests__/DifferentialTabReplacementTestCase.php | 4 ++-- webroot/rsrc/css/core/syntax.css | 5 ----- 4 files changed, 8 insertions(+), 20 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 77242a1384..d10f117847 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '2e175364', + 'core.pkg.css' => 'da792a0f', 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', @@ -115,7 +115,7 @@ return array( 'rsrc/css/application/uiexample/example.css' => 'b4795059', 'rsrc/css/core/core.css' => '1b29ed61', 'rsrc/css/core/remarkup.css' => '7d3ebc86', - 'rsrc/css/core/syntax.css' => '98fdb17e', + 'rsrc/css/core/syntax.css' => '548567f6', 'rsrc/css/core/z-index.css' => 'ac3bfcd4', 'rsrc/css/diviner/diviner-shared.css' => '4bd263b0', 'rsrc/css/font/font-awesome.css' => '3883938a', @@ -909,7 +909,7 @@ return array( 'sprite-login-css' => '18b368a6', 'sprite-tokens-css' => 'f1896dc5', 'syntax-default-css' => '055fc231', - 'syntax-highlighting-css' => '98fdb17e', + 'syntax-highlighting-css' => '548567f6', 'tokens-css' => 'ce5a50bd', 'trigger-rule' => '41b7b4f6', 'trigger-rule-control' => '5faf27b9', @@ -1422,6 +1422,9 @@ return array( 'phuix-autocomplete', 'javelin-mask', ), + '548567f6' => array( + 'syntax-default-css', + ), '55a24e84' => array( 'javelin-install', 'javelin-dom', @@ -1800,9 +1803,6 @@ return array( 'javelin-request', 'javelin-util', ), - '98fdb17e' => array( - 'syntax-default-css', - ), '995f5102' => array( 'javelin-install', 'javelin-util', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 1cc1c3ad9d..3652d18a9a 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1604,13 +1604,6 @@ final class DifferentialChangesetParser extends Phobject { 'span', array( 'data-copy-text' => "\t", - - // See PHI1814. Mark this as a single logical tab for the purposes - // of text selection behavior: when the user drags their mouse over - // the character sequence, we'd like the whole thing to select as - // a single unit. - - 'class' => 'logical-tab', ), str_repeat(' ', $ii)); $tag = phutil_string_cast($tag); diff --git a/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php b/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php index 494170c668..d63978cb61 100644 --- a/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php +++ b/src/applications/differential/parser/__tests__/DifferentialTabReplacementTestCase.php @@ -4,8 +4,8 @@ final class DifferentialTabReplacementTestCase extends PhabricatorTestCase { public function testTabReplacement() { - $tab1 = " "; - $tab2 = " "; + $tab1 = " "; + $tab2 = " "; $cat = "\xF0\x9F\x90\xB1"; diff --git a/webroot/rsrc/css/core/syntax.css b/webroot/rsrc/css/core/syntax.css index 822e1777d6..78aa83dbdc 100644 --- a/webroot/rsrc/css/core/syntax.css +++ b/webroot/rsrc/css/core/syntax.css @@ -39,8 +39,3 @@ span.crossreference-item { color: #ffffff; cursor: default; } - -.logical-tab { - user-select: all; - -webkit-user-select: all; -}