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

MyDeveloper Day via llvm-dev llvm-dev at lists.llvm.org
Tue Jul 28 12:46:56 PDT 2020

Previous message: [llvm-dev] Phabricator down for maintenance tonight
Next message: [llvm-dev] Phabricator down for maintenance tonight
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 (!$is_synthetic);

 if ($can_reply) {

 $action_buttons[] = id(new PHUIButtonView())

```
   ->setTag('a')
```
```
   ->setIcon('fa-reply')
```
```
   ->setTooltip(pht('Reply'))
```

   ->addSigil('differential-inline-reply')

```
   ->setMustCapture(true)
```
```
   ->setAuralLabel(pht('Reply'));
```

 $action_buttons[] = id(new PHUIButtonView())

```
   ->setTag('a')
```
```
   ->setIcon('fa-times')
```
```
   ->setTooltip(pht('Collapse'))
```

   ->addSigil('differential-inline-collapse')

```
   ->setMustCapture(true)
```
```
   ->setAuralLabel(pht('Collapse'));
```

  $menu_items[] = 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(is_ref); 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/f3fc6813/attachment.html>

Previous message: [llvm-dev] Phabricator down for maintenance tonight
Next message: [llvm-dev] Phabricator down for maintenance tonight
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the llvm-dev mailing list