code-review icon indicating copy to clipboard operation
code-review copied to clipboard

Support marking items as seen

Open fuzzycode opened this issue 3 years ago • 3 comments

Use-case

Similar to how github allows you to mark a file as seen while performing a code review to not have to go back and look at it again it would be nice to be able to mark a file or even a hunk as seen to not have to review it again.

This would especially be useful when reviewing large PRs with many changes where the review has to be done in multiple sessions, possibly over several days.

Functionality

Similar to how Magit allows to stage either a hunk or a whole file depending on the position of point it would be possible to mark a hunk or a whole file as seen, depending on the position of point.

There would need to be a way to display to the user the status of each hunk or each file, possibly also hiding or collapsing them by default like github does when a file is seen. Not sure what would be the best alternative for this, a new face or an icon or text describing the state.

Not sure if this is technically possible but it would be a nice feature to have when working with large PRs.

fuzzycode avatar Dec 18 '21 11:12 fuzzycode

Great idea @fuzzycode ! I've been thinking in the following UX:

  1. Create a new session above Files changed called "Files seen"
  2. If you mark a hunk as "seen", we move the file with only that specific hunk to "Files seen" view
  3. If you mark a file as "seen", we move the whole file to "Files seen"
  4. Should be possible to move from Files seen to Files changed again

I believe this is possible to be done, not at all trivially. I would like to iterate on the UX of this feature before start coding anything. wdyt?

Following up the next feature request of "new since last review" we can add a new warning text at the right side of the file name in Files Seen section displaying a "New since last review". And perhaps even a command to filter the whole buffer with only things that changes since last review should be possible something like a "narrow to new changes".

wandersoncferreira avatar Dec 18 '21 17:12 wandersoncferreira

Yes all those ideas sounds good to me.

For "Mark as seen" I can think of two different solutions.

Alternative One:

Have 2 sections, something like "Seen" and "Not Seen" and it would work exactly like Staged/Unstaged works for Magit. You would move lines, hunks or whole files between the two groups using a single key and depending on what is selected it would do the right thing. Just like with staging a file that can be in both groups at the same time it could be in both the seen and unseen group if it has both seen and unseen hunks.

Alternative Two:

Keep one list but mark hunks as seen/not seen. For this it could use a different colour or text to mark things as seen and possible use a different colour for the file names to indicate the state. This would get very noisy if it should support individual lines.

I think I like the alternative one best, hoping that it would feel very native to how the rest of magit works.

For "New Changes" I like the idea of being able to "narrow to new stuff" to focus the effort of the review.

Having both should help a lot when doing large or complex reviews that take a lot of time to do.

fuzzycode avatar Dec 18 '21 19:12 fuzzycode

Definitely the Alternative 1 Magit-like behavior is very nice. However, perhaps this whole notion of "seen" and "not seen" should be in a different buffer? In Github we have these things in dedicated tabs.

Or only a key to toggle this view in the main buffer. So a new key to add the "seen" section otherwise everything is displayed as-is today.

wandersoncferreira avatar Dec 18 '21 19:12 wandersoncferreira