New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render file patches with a decorated Editor #1512

Merged
merged 402 commits into from Oct 25, 2018

Conversation

8 participants
@smashwilson
Member

smashwilson commented Jun 6, 2018

Use decorations within an Atom TextEditor element to implement a file patch item. This should get us improved performance for large diffs, editable diff support, and open the path for multi-file diff views like a commit or pull request pane.

Just to set proper expectations, I'll likely need to pause and resume this a few times before it lands. Don't worry, I won't forget about it πŸ˜„

Remaining Work
  • Split FilePatchController out into a FilePatchItem, FilePatchContainer, FilePatchController, and FilePatchView, as per #1436.
  • Inherit basic item wiring (getTitle(), etc) onto FilePatchItem.
  • Render an <AtomTextEditor> within the FilePatchView.
  • Model: render a FilePatch to the unadorned text that should be displayed within the editor accompanied by the buffer line ranges that correspond to each hunk and line range
  • View: render each group of controls from the original FilePatchView within an appropriately placed Decoration
    • CSS tweaks to keep it looking right
  • Controller: port interactions over and wire 'em up to the right controls
    • Figure out how to preserve selection semantics
  • Update Marker and Decoration components to handle prop changes correctly
  • Correctly and efficiently update FilePatchView decorations when the FilePatch changes due to staging operations
  • Upstream in Atom atom/atom#17736
    • Support custom line numberings in the line number gutter
    • Support the creation of multiple line number gutters
    • Investigate and correct the weird doubled line number labels in custom line-number gutters
    • Write tests for the new gutter code in TextEditorElement's tests
  • Fix StagingView selection sync regression
  • "No changes to display" should be centered in the pane
  • Rework PresentedFilePatch model to unify it with the old model and prevent us from keeping two copies of the diff around
    • Store all patch text in a single String owned by Patch
    • Use an IndexedRowRange to unify an Atom Range with a pair of string offsets
    • Eliminate HunkLine and replace it with IndexedRowRanges within each Hunk that delimit addition and deletion regions
    • Rework partial and complete stage and unstage patch generation
    • Use null objects for missing Files or Patches
    • Unify new model objects within a FilePatch
    • Use a nullFilePatch to signal a missing patch
    • Efficiently determine the Hunk owning a given buffer row
    • Generate old and new line numbers given a buffer row within the patch
    • Use buffer rows to represent lines within a FilePatchSelection
  • Adjust the FilePatchView to use the new model instead of a PresentedFilePatch
  • An empty FilePatch should have navigation controls
  • Restore commands and hotkeys
  • Synchronize the FilePatchSelection and the TextEditor selections
    • Derive the selected patch rows from the current TextEditor selection ranges
    • Handle ctrl- and cmd-clicks and drags on the gutters by adjusting the TextEditor selections
    • Fix patch application
    • Heuristically preserve selection rows when a new FilePatch is generated
    • Suppress setState while the AtomTextEditor is being re-rendered with new content
    • Delete the now-unused FilePatchSelection and its tests
  • Stretch hunk and change region ranges to the end of the last line
  • Catch test coverage up to the speculative work. Get us βœ… again.
  • Store the patch text in a TextBuffer in the Patch model instead of a String
    • Allow an AtomTextEditor to be initialized with an externally owned TextBuffer.
    • Store markers within Regions and Hunks instead of IndexedRowRanges.
    • Organize the markers within a given patch into layers.
    • Create markers instead of IndexedRowRanges in buildFilePatch().
    • Roll a new FilePatch into a previous FilePatch's buffer.
    • Move getHunkAt() into Patch and FilePatch.
    • Allow Markers and MarkerLayers to inherit externally managed resources by ID
    • Decorate Patch-owned MarkerLayers and Markers in FilePatchView.
    • Preserve the buffer when a new FilePatch arrives.
    • Make the CommitView use an external text buffer for the commit message.
  • Re-introduce hunk selections
  • Create a prerelease tag on this branch so that we can dogfood this
v0.19.1-0
  • Fix discarding hunks and lines
  • Stage, unstage, or discard patches with a blank line
  • Correct exception when staging or unstaging the final hunk
v0.19.1-1
  • Correct breakage when decorating destroyed markers
  • Re-introduce the "toggle hunk" keybinding
  • Be less sensitive to stale MarkerLayers when rendering
  • Re-introduce hunk-mode navigation controls
  • Selection decorations only appear on the first row when a row is soft-wrapped
  • "TypeError: Cannot read property 'getMarker' of undefined"
  • Re-render on first click with soft-wrapped lines. Upstream fix: atom/atom#18087
  • Reword "(un)stage selections" buttons to mention lines
v0.19.1-2
  • Include a final empty row in the selected row set
  • TypeError: Cannot destructure property holder of 'undefined' or 'null' when conflicts are encountered
  • Empty diff view when staging and unstaging changes
  • Include keyboard shortcuts in (un)stage selection buttons
v0.19.1-3
  • "Discard changes" regressed again
  • Begin in hunk selection mode
  • βœ… Green tests again checkpoint βœ…
v0.19.1-5
  • Reintroduce + and - indicators in a gutter
  • Improve the context menu on diffs
  • Correct keyboard focus when opening FilePatchItem
  • Track down stalling and failing tests on CircleCI
  • Reintroduce "surfacing" focus to the Git tab with cmd/ctrl-right
v0.19.1-6
  • Register more missing commands
  • Port #1667 on top of the rewrite
v0.22.1-0
  • Integration tests for patch correctness
Concurrent with review
  • Ensure CI runs a full stable, beta, and dev matrix on all three platforms - #1663
  • Handle CI tests that run against versions of Atom older than the version specified in package.json - #1664
  • 🎨 ✨ @simurai design magic CSS pass 🎨 ✨
Checks
  • Ensure that the pane item works well
    • ... as a pending pane item
    • ... on serialize and deserialize
    • ... when changing between multiple repositories
  • Ensure that our keyboard shortcuts and commands all still work properly
  • Ensure that we don't regress our ability to handle rapid staging and unstaging
  • Make sure I delete the .old.js files 😬
  • Regression check: #1312
  • Regression check: #1241
Stretch goals
  • Make the editor editable. Alter the FilePatch accordingly. Filed as #1720.
  • Add syntax highlighting by setting the editor's grammar.
  • Support folding of the diff
  • Efficiently match up the rendered decorations by deriving a durable hunk-specific key.
  • Keep hunk headers visible as you scroll: #1642
Resources

A Guided Tour to that Giant "Files changed" Tab

Fixes #1502. ...Eventually πŸ•

@smashwilson smashwilson force-pushed the aw/file-patch-editor branch from d5aefd6 to 33595a8 Jun 10, 2018

@smashwilson

This comment was marked as outdated.

Member

smashwilson commented Jun 11, 2018

Getting there πŸ˜„

screen shot 2018-06-11 at 1 35 23 pm

@smashwilson smashwilson added this to In progress πŸ”§ in Feature Sprint: 18 June - 6 July 2018 Jun 22, 2018

@smashwilson smashwilson removed this from In progress πŸ”§ in Feature Sprint: 18 June - 6 July 2018 Jul 10, 2018

@smashwilson smashwilson referenced this pull request Jul 24, 2018

Merged

Multiple, custom line number gutters #17736

4 of 4 tasks complete

@smashwilson smashwilson referenced this pull request Aug 20, 2018

Open

Commit pane item #1655

@smashwilson

This comment was marked as resolved.

Member

smashwilson commented Aug 21, 2018

/cc @sguthals @simurai @kuychaco @annthurium

I'd like to hear your opinion about how selections should work here. Because we're using a proper Atom text editor for the patch body, we have two sets of selection ranges to manage:

  1. The editor's selections. You make these by clicking and dragging on the actual patch text. They're useful for copying from patches (and, eventually, cutting and pasting, multi-selections, and so on once we get to editable patches). Editor selections are pretty freeform: they can begin and end at any valid buffer position, in the middles of rows and so on, and you can have zero to many of them.
  2. The FilePatchSelection. These are a collection of hunks and buffer rows that are used to generate the actual patch we apply to implement partial staging and unstaging. The FilePatchSelection is a bit more restricted, as it can only contain changed lines, and must consist of entire rows. It's also modal and can be toggled between hunk and line mode.

At the moment, these are managed independently. The editor selections are manipulated with standard mouse and keyboard actions in the editor itself, while the FilePatchSelection is manipulated by mouse actions on the line number gutters or by clicking on the hunk headers. I've been thinking that this is a bit awkward and counter-intuitive, though.

What I'd like to implement is binding the two together. When the editor selections are changed, make the FilePatchSelection be the set of changed buffer rows that the editor selections currently span, and when the FilePatchSelection is changed directly, set the editor selection to be the corresponding rows.

Can you think of any reason to keep the two independent? Do you think that unifying them would feel natural?

@annthurium

This comment was marked as resolved.

Contributor

annthurium commented Aug 21, 2018

@smashwilson I support this change to unifying handling of editor selections and file patch selections.

I can't speak for other users, but I think I would find it confusing/frustrating to have file patch selections work differently.

We should look at how vscode handles this as well. I believe that you can edit diffs and select text the same way as you select text in the editor, so keeping things consistent here seems like a win.

@simurai

This comment was marked as resolved.

Member

simurai commented Aug 22, 2018

Do you think that unifying them would feel natural?

Would the unifying be that selections behave just like in current buffers, but with the addition that you can click the hunk header to select the whole hunk at once? I think that's a good idea. πŸ‘

When selecting only a single word and then clicking on "Stage Selection", it might feel unexpected that it staged the entire line/row. But should still be better than "forcing" people to only use the gutter to select individual lines for staging.

@smashwilson

This comment was marked as resolved.

Member

smashwilson commented Aug 22, 2018

Thanks for weighing in πŸ˜„ I've added a checklist entry to track this.

When selecting only a single word and then clicking on "Stage Selection", it might feel unexpected that it staged the entire line/row. But should still be better than "forcing" people to only use the gutter to select individual lines for staging.

We could change the button caption to "Stage Lines" or "Stage Selected Lines. We could also style the selected rows differently to highlight them as "the selected ones" rather than just using the gutter highlight, to emphasize that "the stuff that's this color is what will be applied if you click the thing." Then you'd select a single word and see the entire line change.

We're already using line background color to signal a few things though:

  • Addition, removal, nonewline
  • Cursor row
  • Text editor selection

It could be getting a bit overloaded. I'm not sure.

@smashwilson

This comment was marked as outdated.

Member

smashwilson commented Aug 22, 2018

Progress shots, running in Atom Nightly:

screen shot 2018-08-22 at 8 11 59 am

screen shot 2018-08-22 at 8 12 09 am

smashwilson added some commits Aug 29, 2018

smashwilson added some commits Oct 25, 2018

smashwilson added some commits Oct 25, 2018

@smashwilson smashwilson merged commit 32c39c2 into master Oct 25, 2018

7 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: snapshot Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+3.1%) to 84.854%
Details

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from QA Review πŸ”¬ to Merged β˜‘οΈ Oct 25, 2018

@smashwilson smashwilson deleted the aw/file-patch-editor branch Oct 25, 2018

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 25, 2018

power up

@simurai

This comment has been minimized.

Member

simurai commented Oct 25, 2018

I shall remember this PR as "The wall of checkboxes PR". πŸ˜‚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment