Page MenuHomePhabricator
Create Task
Maniphest T12153

'Show Hidden Comments' bubble does not fit in margin next to line numbers >1000
Closed, ResolvedPublic
Actions

Description

pasted_image_at_2017_01_24_01_23_pm.png (138×681 px, 50 KB)

(Apologies for the super nit picky bug report, but it seemed borderline worth filing, it becomes more noticeable the more neighboring comments you have to hide)

Steps to reproduce: Create an inline comment on a revision on a line that has a number >1000, hide the comment. The column where the 'Show Hidden Comments' bubble appears can't fit both the line number and the bubble without line wrapping, which also pushes what looks like an empty line into the diff.

Version info:

* commit ab4355cde0f8913b9e12be345f813f5ba7e0e468
  Author: epriestley <git@epriestley.com>
  Date:   Sat Jan 21 08:11:19 2017
  
      (stable) Move Favorites and User menus to MenuBarExtensions

It looks like the line number column has a fixed width of 45 pixels.

Revisions and Commits

rP Phabricator
D17861rP4fd4ec3d275d Hide inlines one-by-one, instead of in a big group

Related Objects

Event Timeline

chris.siden created this task.Jan 24 2017, 11:29 PM
epriestley added a subscriber: epriestley.Jan 25 2017, 3:52 PM

What possible use case could anyone ever have for putting a THOUSAND lines of source code in a single file?!!

(This reproduces for me.)

Screen Shot 2017-01-25 at 7.51.18 AM.png (243×338 px, 15 KB)

epriestley triaged this task as Low priority.Jan 25 2017, 3:52 PM
epriestley added a project: Differential.
Herald added a subscriber: eadler. · View Herald TranscriptJan 25 2017, 3:52 PM
epriestley moved this task from Backlog to Inline Infra on the Differential board.Jan 25 2017, 3:52 PM
cspeckmim added a subscriber: cspeckmim.Jan 25 2017, 11:24 PM

My first internship I was tasked with working on an application as sole developer. It started at 8.6k lines in a Main.cs file and at the end of the summer I left with it at about 15.5k. Luckily we didn't do code reviews.

Mnkras added a subscriber: Mnkras.Jan 26 2017, 1:58 AM

@epriestley you don't want to know what schools teach/accept now-adays... is very scary.

epriestley added a project: Inline Comments.Apr 21 2017, 12:20 PM
epriestley moved this task from Backlog to Glitches on the Inline Comments board.Apr 21 2017, 12:22 PM
epriestley added a comment.May 2 2017, 6:48 PM

I'm thinking about resolving this by having three states instead:

  • Normal (as today).
  • Collapsed (replaces the current "hidden" state).
  • Hidden (new state, really hidden, tied to T8909).

The current "Hide" action would be replaced with "collapse", which would shrink the inline to a single row of lighter grey text with the current bubble-to-expand in the sidebar:

2302if (something) {
2303 do_magic();
alice: blah blah...
bailey: blah blah...
2304}

T8909 would introduce a new global "hide" state which would remove the rows completely, perhaps leaving a very lightweight indicator (like a background color in the numbers column) on the line to indicate that inlines exist, but not a full interactable "expand" element. To show them again, you'd change the setting to "show inlines".

I think this probably better aligns with use cases, too -- I use "hide" to mean "this is resolved to my personal satisfaction and I don't need to look at it anymore", but "collapse" is probably actually a better fit for that action. Likewise, global states from T8909 are probably better fits for "I just want to look at the code for now".

chad added a subscriber: chad.May 2 2017, 8:11 PM

56174329.jpg (508×914 px, 46 KB)

epriestley added a revision: D17861: Hide inlines one-by-one, instead of in a big group.May 9 2017, 9:01 PM
epriestley closed this task as Resolved by committing rP4fd4ec3d275d: Hide inlines one-by-one, instead of in a big group.May 16 2017, 1:20 PM
epriestley added a commit: rP4fd4ec3d275d: Hide inlines one-by-one, instead of in a big group.
Phabricator · Built by Phacility