Page MenuHomePhabricator
Create Task
Maniphest T6323

Provide UI indication you've reached the end of a Diff's comments
Closed, WontfixPublic
Actions

Description

Steps to reproduce:

  1. Open a CR with multiple notes, e.g. https://secure.phabricator.com/D8685?vs=on&id=21431&whitespace=ignore-all#toc
  2. Go to the last note
  3. Press "Next"

Expected behavior: Phabricator somehow indicates to me that I've finished reviewing all the notes

Actual behavior: Phabricator silently wraps around to the top.

I guess this is not a big deal if all the comments are real comments, but if there are a lot of lint comments, they can be a bit repetitive, and it's easy to accidentally end up back up top without realizing it.

Related Objects

Event Timeline

ezyang created this task.Oct 16 2014, 5:39 AM
ezyang raised the priority of this task from to Needs Triage.
ezyang updated the task description. (Show Details)
ezyang added a project: Phabricator.
ezyang added a subscriber: ezyang.
chad closed this task as Wontfix.Oct 16 2014, 2:13 PM
chad claimed this task.

This feature works as I expect it. The behaviour is consistent with "Search & Find" interfaces on many applications like Chrome/Firefox and changing this default would likely generate a moderate level of outrage from other Phabricator users who expect it.

ezyang added a comment.Oct 16 2014, 6:51 PM

Most search & find interfaces give a visual cue when they wrap around, however. Would a patch along those lines be accepted?

chad reopened this task as Open.Oct 16 2014, 8:11 PM

Yeah, let's see where we end up.

chad renamed this task from "Next" link on code review comments should not wrap around to beginning to Provide UI indication you've reached the end of a Diff's comments.Oct 16 2014, 8:32 PM
chad removed chad as the assignee of this task.
chad triaged this task as Normal priority.
chad edited projects, added Differential, Design; removed Phabricator.
chad added a subscriber: chad.
chad moved this task from Backlog to v2 (UI/Mobile) on the Differential board.Mar 4 2015, 5:09 PM
epriestley added a subscriber: epriestley.Mar 8 2015, 6:40 PM

@chad, do you have thoughts on where you want to go with this? I'm definitely WONTFIX on it -- e.g., neither Chrome nor Safari nor TextMate give a specific visual indicator when wrapping under "⌘G", so this feels consistent to me. I can't recall any application giving a specific wrap indication except nano, which just says [Search Wrapped].

chad added a comment.Mar 8 2015, 7:28 PM

My only thought was maybe change the text to say:

Next (back to top)

Or something along those lines. But perhaps with T1460 this is solved by a done indicator (UI color changes)

epriestley closed this task as Wontfix.Mar 8 2015, 7:34 PM
epriestley claimed this task.
  • Value seems very low.
  • Behavior is generally consistent with "Find Next" in many programs, including browsers.
  • It's a pain to compute "Next (Back to Top)" because inlines can become visible or invisible in response to a bunch of events (e.g., adding and removing comments, showing and hiding files, new files loading).
  • There may be only one inline, which would produce something goofy like "Prev (Wrap to Bottom) * Next (Back to Top)" and probably need to be special cased, further raising the complexity of the feature.
  • Use case isn't very compelling: there shouldn't ever be a lot of lint comments. If linters are raising nontrivial levels of false positives, delete them. Lint has powerful tools for excluding files, adjusting severities, etc., and particularly after .arclint these are relatively easy to configure.
Phabricator · Built by Phacility