Page MenuHomePhabricator

Conpherence - change "A, B, C..." subtitle to "A: what most recent person said" when we can
ClosedPublic

Authored by btrahan on Apr 10 2015, 6:16 PM.

Details

Summary

For the price of loading transactions more consistently, we get a better subtitle. We do this in all cases EXCEPT for when we're grabbing handles, because that makes the handles pretty heavy weight and I could even feel the perf hit on my development machine and we don't use subtitle there anyway. We may want to cache the latest message on the conpherence thread object to improve performance here as well as consider falling back to "A, B, C..." more often. Code is written such that no transactions means an automagical fallback.

Fixes T7795. (Technically, there's still a note about handle code conversion work on T7795 but we'll get that generally later.)

Test Plan

played around with conpherence in both views and things seemed to work nicely.
made sure to try the original repro in T7795 and couldn't get that to go either
posted a long comment and verified that the CSS / string truncation both make it display nicely. Note that without the CSS the chosen glyph value can be too high to fit nicely at times.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

btrahan retitled this revision from to Conpherence - change "A, B, C..." subtitle to "A: what most recent person said" when we can.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

Got a warning in a unit test so give me a sec.

btrahan edited edge metadata.

make sure to check $transactions != ConpherenceThread::ATTACHABLE

epriestley edited edge metadata.

We could also specialize the transaction subquery to get the most recent comment at some point, but that'd be more code so it feels reasonable to wait for this to show up on a profile.

src/applications/conpherence/storage/ConpherenceThread.php
128

Maybe consider getTransactions() and getTransactionsIfAttached() or similar (or a hasAttachedTransactions() method) to make intent more clear.

This revision is now accepted and ready to land.Apr 10 2015, 6:51 PM
btrahan edited edge metadata.

change to hasAttachedTransactions to improve clarity of code

This revision was automatically updated to reflect the committed changes.