[llvm-dev] Phabricator down for maintenance tonight
MyDeveloper Day via llvm-dev
llvm-dev at lists.llvm.org
Tue Jul 28 12:49:41 PDT 2020
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
> (!$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/7af65b4a/attachment-0001.html>
More information about the llvm-dev
mailing list