[llvm-dev] Phabricator down for maintenance tonight (original) (raw)

Mehdi AMINI via llvm-dev llvm-dev at lists.llvm.org
Tue Jul 28 15:58:05 PDT 2020

+Tom Stellard <tstellar at redhat.com> : can we get a fork of https://github.com/phacility/phabricator in the LLVM project org on GitHub in order to host our patched version and start collaboratively maintaining it?

-- Mehdi

On Tue, Jul 28, 2020 at 12:49 PM MyDeveloper Day <mydeveloperday at gmail.com> wrote:

Could we ever consider adding https://github.com/r4nt/phabricator/tree/llvm-production as a new read/only observe Diffusion repository in reviews.llvm.org? I'd be happy to do code reviews? MyDeveloperDay On Tue, Jul 28, 2020 at 8:46 PM MyDeveloper Day <mydeveloperday at gmail.com> wrote:

Awesome, thanks for sharing

Here is a patch (based off yours) but this adds the "Collapse" button back in, remember to rerun phabricator/bin/celerity map MyDeveloperDay diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index fdaa99a..9b18031 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -214,6 +214,23 @@ final class PHUIDiffInlineCommentDetailView (!$issynthetic); if ($canreply) { + $actionbuttons[] = id(new PHUIButtonView()) + ->setTag('a') + ->setIcon('fa-reply') + ->setTooltip(pht('Reply')) + ->addSigil('differential-inline-reply') + ->setMustCapture(true) + ->setAuralLabel(pht('Reply')); + + $actionbuttons[] = id(new PHUIButtonView()) + ->setTag('a') + ->setIcon('fa-times') + ->setTooltip(pht('Collapse')) + ->addSigil('differential-inline-collapse') + ->setMustCapture(true) + ->setAuralLabel(pht('Collapse')); + + $menuitems[] = array( 'label' => pht('Reply to Comment'), 'icon' => 'fa-reply', diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 7293f89..bb84997 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -2402,6 +2402,19 @@ JX.install('DiffChangesetList', { ['differential-inline-comment', 'inline-action-dropdown'], onmenu); + // restore button for replying to comments + var onreply = JX.bind(this, this.onInlineEvent, 'reply'); + JX.Stratcom.listen( + 'click', + ['differential-inline-comment', 'differential-inline-reply'], + onreply); + var oncollapse = JX.bind(this, this.onInlineEvent, 'collapse'); + JX.Stratcom.listen( + 'click', + ['differential-inline-comment', 'differential-inline-collapse'], + oncollapse); + // END + var ondraft = JX.bind(this, this.onInlineEvent, 'draft'); JX.Stratcom.listen( 'keydown', @@ -2491,6 +2504,12 @@ JX.install('DiffChangesetList', { case 'delete': inline.delete(isref); break; + case 'reply': + inline.reply(); + break; + case 'collapse': + inline.setCollapsed(!inline.isCollapsed()); + break; case 'draft': inline.triggerDraft(); break; On Tue, Jul 28, 2020 at 8:02 PM Mehdi AMINI <joker.eph at gmail.com> wrote:

On Tue, Jul 28, 2020 at 11:04 AM MyDeveloper Day <_ _mydeveloperday at gmail.com> wrote: Out of interest are you keeping the local modifications in a fork of the Phabricator source (llvm-phabricator)? Firstly we should be keeping any changes we make in source control but also it's good to review those changes. I didn't innovate, Manuel set it up originally and I just reused the flow there: https://github.com/r4nt/phabricator/tree/llvm-production That said our "pre-production" setup isn't super streamlined, I'd like to improve this in the future. Let me know if you have suggestions :) I have found over the years that I've had to redo some of my local modifications as @evan changes the underlying infrastructure, nothing major but sometimes can cause a little downtime when doing an upgrade. Yeah we had some merge conflicts as well, I reduced some of our diff with upstream in the process, I think we can reduce a bit more. What I did for this upgrade is that 10 days ago I: - duplicate the entire VM and the database on a new machine. - merged the latest stable version and went through the conflicts - upgraded the database (had to manually craft some SQL queries as some duplicated records were breaking newly added keys). - tested manually this cloned instance by opening fake reviews. Had to fix the mail config since they changed the way it is configured. - Had to patch some broken code in phab in the SendGrid interaction with respect to attachments: https://discourse.phabricator-community.org/t/sendgrid-mailer-metamta-differential-attach-patches-true-errors-with-the-attachment-type-cannot-contain-or-crlf-characters/4098 Then when everything was working, I took down the production instance and repeated the migration steps (except fixing the merge conflicts and the patching that I had kept in git). -- Mehdi

MyDeveloperDay On Tue, Jul 28, 2020 at 6:17 PM Mehdi AMINI via llvm-dev <_ _llvm-dev at lists.llvm.org> wrote: On Tue, Jul 28, 2020 at 4:25 AM James Henderson <_ _jh7370.2008 at my.bristol.ac.uk> wrote: Thanks for the work too! Not specifically a regression, but since the upgrade, I find it annoying now that when I want to do something in relation to an inline comment (collapse it, reply to it etc), I now have to click on a drop-down menu in the comment header bar to select the option, whereas before the icons were inline there before. Is this something that can be easily addressed? Seems like https://secure.phabricator.com/D21244 <https://secure.phabricator.com/D21244>I patched back the reply button, collapse seems a bit more intrusive. That said the keyboard shortcuts are pretty nice: you can click on a comment to select it, n for selecting the next one, p the previous one, q to collapse, r to reply. -- Mehdi

James On Tue, 28 Jul 2020 at 05:56, David Blaikie via llvm-dev <_ _llvm-dev at lists.llvm.org> wrote: Thanks for the work! On Mon, Jul 27, 2020 at 9:53 PM Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Hi, > > Phab is upgraded now. It was ~2 years since the last upgrade so we got a few features along the way. > > We may have regressed some aspects as well, I only tested the basic functionalities. > Let me know if you see anything behaving unexpectedly! > > -- > Mehdi > > > On Mon, Jul 27, 2020 at 7:53 PM Mehdi AMINI <joker.eph at gmail.com> wrote: >> >> Hi, >> >> FYI, I'm taking Phabricator down for an upgrade right now. >> >> -- >> Mehdi >> _> ________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

LLVM Developers mailing list llvm-dev at lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

LLVM Developers mailing list llvm-dev at lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200728/52cd00a0/attachment.html>

More information about the llvm-dev mailing list