<div dir="ltr"><br><div>Could we ever consider adding <a href="https://github.com/r4nt/phabricator/tree/llvm-production">https://github.com/r4nt/phabricator/tree/llvm-production</a> as a new read/only observe Diffusion repository in <a href="http://reviews.llvm.org">reviews.llvm.org</a>? I'd be happy to do code reviews?</div><div><br></div><div>MyDeveloperDay</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 28, 2020 at 8:46 PM MyDeveloper Day <<a href="mailto:mydeveloperday@gmail.com">mydeveloperday@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Awesome, thanks for sharing<br><div><br></div><div>Here is a patch (based off yours) but this adds the "Collapse" button back in, </div><div><br></div><div>remember to rerun phabricator/bin/celerity map </div><div><br></div><div>MyDeveloperDay</div><div><br></div><div>diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php<br>index fdaa99a..9b18031 100644<br>--- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php<br>+++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php<br>@@ -214,6 +214,23 @@ final class PHUIDiffInlineCommentDetailView<br> (!$is_synthetic);<br> <br> if ($can_reply) {<br>+ $action_buttons[] = id(new PHUIButtonView())<br>+ ->setTag('a')<br>+ ->setIcon('fa-reply')<br>+ ->setTooltip(pht('Reply'))<br>+ ->addSigil('differential-inline-reply')<br>+ ->setMustCapture(true)<br>+ ->setAuralLabel(pht('Reply'));<br>+<br>+ $action_buttons[] = id(new PHUIButtonView())<br>+ ->setTag('a')<br>+ ->setIcon('fa-times')<br>+ ->setTooltip(pht('Collapse'))<br>+ ->addSigil('differential-inline-collapse')<br>+ ->setMustCapture(true)<br>+ ->setAuralLabel(pht('Collapse'));<br>+<br>+<br> $menu_items[] = array(<br> 'label' => pht('Reply to Comment'),<br> 'icon' => 'fa-reply',<br>diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js<br>index 7293f89..bb84997 100644<br>--- a/webroot/rsrc/js/application/diff/DiffChangesetList.js<br>+++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js<br>@@ -2402,6 +2402,19 @@ JX.install('DiffChangesetList', {<br> ['differential-inline-comment', 'inline-action-dropdown'],<br> onmenu);<br> <br>+ // restore button for replying to comments<br>+ var onreply = JX.bind(this, this._onInlineEvent, 'reply');<br>+ JX.Stratcom.listen(<br>+ 'click',<br>+ ['differential-inline-comment', 'differential-inline-reply'],<br>+ onreply);<br>+ var oncollapse = JX.bind(this, this._onInlineEvent, 'collapse');<br>+ JX.Stratcom.listen(<br>+ 'click',<br>+ ['differential-inline-comment', 'differential-inline-collapse'],<br>+ oncollapse);<br>+ // END<br>+<br> var ondraft = JX.bind(this, this._onInlineEvent, 'draft');<br> JX.Stratcom.listen(<br> 'keydown',<br>@@ -2491,6 +2504,12 @@ JX.install('DiffChangesetList', {<br> case 'delete':<br> inline.delete(is_ref);<br> break;<br>+ case 'reply':<br>+ inline.reply();<br>+ break;<br>+ case 'collapse':<br>+ inline.setCollapsed(!inline.isCollapsed());<br>+ break;<br> case 'draft':<br> inline.triggerDraft();<br> break;<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 28, 2020 at 8:02 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 28, 2020 at 11:04 AM MyDeveloper Day <<a href="mailto:mydeveloperday@gmail.com" target="_blank">mydeveloperday@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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.</div></blockquote><div><br></div><div>I didn't innovate, Manuel set it up originally and I just reused the flow there: <a href="https://github.com/r4nt/phabricator/tree/llvm-production" target="_blank">https://github.com/r4nt/phabricator/tree/llvm-production</a></div><div><br></div><div>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 :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>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.<br></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>What I did for this upgrade is that 10 days ago I:</div><div><br></div><div>- duplicate the entire VM and the database on a new machine.</div><div>- merged the latest stable version and went through the conflicts</div><div>- upgraded the database (had to manually craft some SQL queries as some duplicated records were breaking newly added keys).</div><div>- tested manually this cloned instance by opening fake reviews. Had to fix the mail config since they changed the way it is configured.</div><div>- Had to patch some broken code in phab in the SendGrid interaction with respect to attachments: <a href="https://discourse.phabricator-community.org/t/sendgrid-mailer-metamta-differential-attach-patches-true-errors-with-the-attachment-type-cannot-contain-or-crlf-characters/4098" target="_blank">https://discourse.phabricator-community.org/t/sendgrid-mailer-metamta-differential-attach-patches-true-errors-with-the-attachment-type-cannot-contain-or-crlf-characters/4098</a></div><div><br></div><div>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).</div><div><br></div><div>-- </div><div>Mehdi</div><div><br></div><div><br></div><div><br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><br></div><div>MyDeveloperDay</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 28, 2020 at 6:17 PM Mehdi AMINI via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 28, 2020 at 4:25 AM James Henderson <<a href="mailto:jh7370.2008@my.bristol.ac.uk" target="_blank">jh7370.2008@my.bristol.ac.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Thanks for the work too!</div><div><br></div><div>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?</div></div></blockquote><div><br></div><div>Seems like <a href="https://secure.phabricator.com/D21244" target="_blank">https://secure.phabricator.com/D21244</a></div><div><br></div><div><a href="https://secure.phabricator.com/D21244" target="_blank"></a>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.</div><div><br></div><div>-- </div><div>Mehdi</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div>James<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 28 Jul 2020 at 05:56, David Blaikie via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Thanks for the work!<br>
<br>
On Mon, Jul 27, 2020 at 9:53 PM Mehdi AMINI via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
> Hi,<br>
><br>
> Phab is upgraded now. It was ~2 years since the last upgrade so we got a few features along the way.<br>
><br>
> We may have regressed some aspects as well, I only tested the basic functionalities.<br>
> Let me know if you see anything behaving unexpectedly!<br>
><br>
> --<br>
> Mehdi<br>
><br>
><br>
> On Mon, Jul 27, 2020 at 7:53 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br>
>><br>
>> Hi,<br>
>><br>
>> FYI, I'm taking Phabricator down for an upgrade right now.<br>
>><br>
>> --<br>
>> Mehdi<br>
>><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
</blockquote></div></div></div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
</blockquote></div></div></div></div>
</blockquote></div>
</blockquote></div>