Re: PSA: Staying in control of Phabricator comments, and avoiding a footgun

2020-08-18 Thread Karl Tomlinson
Karl Tomlinson writes:

> "Show Older Inlines" "Disabled" sounds like something to avoid
> like the plague.  I wonder whether global configuration to remove
> that setting is available ...

My comment was rash.

The setting can actually be very useful (at least temporarily)
because, when disabled, the comments are still in the history but
always link to the code where the comment was made (instead of
using heuristics to move the comment).  However, some other
mechanism is required to keep track of which comments are
addressed.

"When a revision is updated, Phabricator attempts to bring inline
comments on the older version forward to the new changes. You can
disable this behavior if you prefer comments stay anchored in one
place."

My personal preference would probably be that the setting applied
only to the history, making it orthogonal to the "Hide Older
Inlines" option in the transient banner.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


Re: PSA: Staying in control of Phabricator comments, and avoiding a footgun

2020-08-18 Thread Karl Tomlinson
Thank you for highlighting those, Jonathan.

"Show Older Inlines" "Disabled" sounds like something to avoid
like the plague.  I wonder whether global configuration to remove
that setting is available ...

I find the "Collapse" operation on inline comments useful to track
remaining relevant comments, especially given the reviewer has
little control over the "Done" flag.
"Collapse" state is maintained over page loads.
I assume it is tracked per login.
(It is unset and not available when not logged in, at least.)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


PSA: Staying in control of Phabricator comments, and avoiding a footgun

2020-08-18 Thread Jonathan Watt
In talking to some other devs recently, it seems there's poor awareness of the
options to make your life easier when dealing with Phab Revisions that
accumulate a lot of comments.

First, the footgun: setting `Settings → Diff Preferences → Show Older inlines →
Disabled` will hide comments made PRIOR TO THE LAST UPDATE of the Revision. But
beware - this setting will hide comments EVEN IF THEY ARE NOT MARKED "Done". I
only just figured out that the reason that a couple of the devs that I do
reviews for sometimes appear to ignore some of my (sometimes conditional r+)
comments is likely because they have this setting set. This seems pretty bad so
I wouldn't recommend using it.

The other option I'm aware of[1] is somewhat hidden in the `position:fixed`
header banner that appears at the top of the viewport once you scroll down to
the diff part of a Revision. The hamburger menu in that banner has some other
options for hiding inline comments, including 'Hide "Done" inlines'. As long as
the Revision author does a good job of marking comments "Done" as they address
them, this option appears to work well. It's obviously annoying that it needs to
be selected every time you load the page for a Revision, but at least its
behavior is sane and useful. (Who are our reps with Phacility? Can we request
this option also be added to `Settings → Diff Preferences`?)

Hope someone finds the above useful.

Jonathan

[1] Unfortunately not mentioned in:
https://secure.phabricator.com/book/phabricator/article/differential_inlines/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform