Mehdi AMINI via llvm-dev
2020-Jul-28 19:01 UTC
[llvm-dev] Phabricator down for maintenance tonight
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/49216c10/attachment.html>
MyDeveloper Day via llvm-dev
2020-Jul-28 19:46 UTC
[llvm-dev] Phabricator down for maintenance tonight
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>
MyDeveloper Day via llvm-dev
2020-Jul-28 19:49 UTC
[llvm-dev] Phabricator down for maintenance tonight
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>