Slightly improve the chat log functionality by making it parse ticket numbers, links, etc. This is my first venture into patching phabricator, almost certainly not the most efficient way to achieve this result.
Details
Details
- Reviewers
epriestley btrahan - Group Reviewers
Blessed Reviewers
Visit a chat log, see that it's now parsed...
Diff Detail
Diff Detail
- Branch
- master
- Lint
Lint Errors Severity Location Code Message Error /usr/arc/libphutil/src/auth/PhutilAuthAdapterRADIUS.php:106 PHL1 Unknown Symbol Error /usr/arc/libphutil/src/auth/PhutilAuthAdapterRADIUS.php:107 PHL1 Unknown Symbol Error /usr/arc/libphutil/src/auth/PhutilAuthAdapterRADIUS.php:113 PHL1 Unknown Symbol Error /usr/arc/libphutil/src/auth/PhutilAuthAdapterRADIUS.php:117 PHL1 Unknown Symbol Error /usr/arc/libphutil/src/auth/PhutilAuthAdapterRADIUS.php:122 PHL1 Unknown Symbol Error /usr/arc/libphutil/src/auth/PhutilAuthAdapterRADIUS.php:134 PHL1 Unknown Symbol Error src/applications/auth/controller/PhabricatorEmailTokenController.php:54 PHL1 Unknown Symbol Error src/applications/calendar/view/AphrontCalendarMonthView.php:90 PHL1 Unknown Symbol Error src/applications/conpherence/controller/ConpherenceNotificationPanelController.php:75 PHL1 Unknown Symbol Error src/applications/differential/render/DifferentialChangesetHTMLRenderer.php:253 PHL1 Unknown Symbol Error src/applications/differential/view/DifferentialAddCommentView.php:189 PHL1 Unknown Symbol Error src/applications/differential/view/DifferentialInlineCommentView.php:233 PHL1 Unknown Symbol Error src/applications/differential/view/DifferentialLocalCommitsView.php:131 PHL1 Unknown Symbol Error src/applications/differential/view/DifferentialRevisionCommentView.php:90 PHL1 Unknown Symbol Error src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php:205 PHL1 Unknown Symbol Error src/applications/diffusion/controller/DiffusionCommitController.php:747 PHL1 Unknown Symbol Error src/applications/diffusion/view/DiffusionCommentView.php:142 PHL1 Unknown Symbol Error src/applications/feed/builder/PhabricatorFeedBuilder.php:51 PHL1 Unknown Symbol Error src/applications/feed/controller/PhabricatorFeedDetailController.php:30 PHL1 Unknown Symbol Error src/applications/feed/controller/PhabricatorFeedListController.php:35 PHL1 Unknown Symbol Error src/applications/feed/controller/PhabricatorFeedPublicStreamController.php:28 PHL1 Unknown Symbol Error src/applications/herald/controller/HeraldTranscriptController.php:371 PHL1 Unknown Symbol Error src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php:21 PHL1 Unknown Symbol Error src/applications/maniphest/controller/ManiphestTaskDetailController.php:352 PHL1 Unknown Symbol Error src/applications/notification/controller/PhabricatorNotificationListController.php:48 PHL1 Unknown Symbol - Unit
No Test Coverage
Event Timeline
| src/applications/chatlog/controller/PhabricatorChatLogChannelLogController.php | ||
|---|---|---|
| 132 | Is there a more efficient way to parse all messages in one go? | |
Comment Actions
I think the linting errors are all due to the fact that I'm running patched/forked versions of arc and libphutil
Comment Actions
Provided some inline feedback on getting more efficient here. Not sure if @epriestley had other thoughts about the diff tho.
| src/applications/chatlog/controller/PhabricatorChatLogChannelLogController.php | ||
|---|---|---|
| 132 | Yeah, as is this is very inefficient. To fix, put something like this above this foreach loop $engine = id(new PhabricatorMarkupEngine())
->setViewer($this->getUser());
$markup_objects = array();
foreach ($blocks as $index => $block) {
$markup_object = id(new PhabricatorMarkupOneOff())
->setPreserveLinebreaks(true)
->setContent($block);
$engine->addObject($markup_object, 'default');
$markup_objects[$index] = $markup_object;
}
$engine->process();and then where you do this renderOneObject thing, you'll need something like $markup_objects = $markup_objects[$index]; $engine->getOutput($markup_object, 'default'); You'll have to augment the foreach so $index is in scope here too. | |
Comment Actions
You can clear the phutil_tag_div() errors by updating libphutil. You can identify which copy of libphutil to update by examining the first few lines of any arc command with --trace (e.g., arc list --trace).
One quasi-problem with doing this is that stuff like macros will expand inline, which isn't necessarily intended, but I think the direction we're headed will probably see us interpret chat as remarkup with greater frequency. If you make the batch processing improvements @btrahan outlines, I think we can pull this safely.
It would be slightly more efficient to make the log events implement PhabricatorMarkupInterface, then batch them like @btrahan's inline. Since they're immutable, the key can be the log entry ID instead of requiring us to hash the text every time. This should be very minor compared to other performance considerations, however.
Comment Actions
Is there such a thing as parsing profiles/parameters for remarkup, which would allow to parse specific things (PHIDs) but avoid macros, which are likely to be noise, since they were never seen in the original chat conversation?
Comment Actions
There are, but we should avoid them for now since they're kind of built on the wrong dimension. The current profiles are per-application ("feed", "differential", "diffusion"), but these don't actually map well to use.
The right profiles are half-implemented, and are "html", "text", and probably "inline" (no images / huge block elements), which would be appropriate here. We can switch over to using that once that stuff is more robust. T2617 is sort of tracking this. We've gotten rid of most of the differences between the application-based profiles but haven't yet made the role-based profiles substantive.