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

Commit Preview #1767

Merged
merged 416 commits into from Nov 20, 2018

Conversation

@smashwilson
Member

smashwilson commented Oct 30, 2018

🌐 Collaborative pull request to implement the first chunk of behavior described in #1753.

Demo gif

commit-preview

  • bug - funky rendering as shown in #1767 (comment)
  • bug - unstaging last line in CommitView does not select the next last line, instead the selection jumps to another file above (see comment with gif in multi-file-patch.js)
  • bug - cmd-right from a ChangedFileItem throws an error. Desired behavior should be to focus appropriate file in StagingView
  • bug - cmd-right from CommitPreview throws an error. Desired behavior is that "Hide All Staged Changes" button is focused
  • Remove repetitive "Staged Changes" for Commit Preview (keep in ChangedFileItem)
  • Make file name bold (ex: /path/to/ file-name.md)
  • Make symlink and mode change text look less funky (the font looks large)
  • Let's have a MultiFilePatch model that includes an ordered list of FilePatch objects.
  • We'll need to extend the parser to understand multi-file git diff output. There may be some tricky work in here in relation to the deleted-symlink-and-created-file and deleted-file-and-created-symlink edge cases. @smashwilson
  • Add a method to our git repository plumbing to get the multi-diff from all staged changes, or, preferably, find a way to get this information using existing git repository methods. @smashwilson
  • Let's have a CommitPreview component hierarchy for the React side of things: CommitPreviewItem for the workspace item stuff, CommitPreviewContainer for the loading and error logic.
  • For the rest of the components, we have a decision to make. We can either: (a) generalize the existing FilePatch classes (controller and view) to be usable in both situations, or (b) create new components that are specialized for the multi-file case. I'm leaning a little toward (a) but that might change when we get down into the code. (@annthurium and @kuychaco)
    • make it so that users can only highlight hunks or lines from one file at a time.
      • fix initial header highlighting which is real weird rn
    • Fix the styling so that we're not hard coding the height of each file.
  • do we want to support tabbing, or some other kind of keyboard nav, across hunks or files?
  • Finally, we'll need to do the addition of the "preview" button within the CommitView and the corresponding wiring for the preview action in CommitController.
    • If the preview is already showing, the button doesn't do anything. Should the button toggle the preview?
    • Toggle commit preview open/closed with button
      • update button when pane item is destroyed (eg. commit preview is closed by hitting 'x' on the tab)
  • if the "Preview Commit" button is selected, hitting "enter" doesn't actually open the preview. Seems like we should fix that.
    • Ensure that cursor is placed in editor for easy keyboard navigation
  • Rework to use a single TextEditor for all FilePatches (@smashwilson - working in a separate branch until we verify performance improvements)
    • Modify patch builder to use a single TextBuffer and MarkerLayer set for all patches in a MultiFilePatch
    • Modify FilePatchController to accept a MultiFilePatch as a prop, and require a FilePatch as an argument in its action methods
    • Modify FilePatchView to render all FilePatches in a MultiFilePatch
    • Verify improved performance
    • Catch up the tests
  • Replace getChangedFilePatch hack and find better way of getting multi file diff in ChangedFilesItem
  • clean up PropTypes
  • clicking the <> in a file header to open in file takes you to where the selection is, even if it’s not the file corresponding to the header

Metrics

  • toggling the “preview” button

Tests

Unit tests:
For each of these get the test suite to be green and check for missing coverage based on UX testing, application logic, and using coveralls.
Arranged according to component hierarchy. Not in order of what makes sense to tackle first

  • RootController
    • ChangedFileItem
      • ChangedFileContainer
    • CommitPreviewItem
      • CommitPreviewContainer
        • MultiFilePatchController
        • MultiFilePatchView
  • MultiFilePatch
    • FilePatch
      • Patch
  • Full test suite check
    • Repository
    • buildFilePatch
  • add test for "Open in File", testing multiple files

Test coverage:

  • Coveralls reports no new uncovered lines
  • Coveralls reports no uncovered existing lines

Manual test plan:

  • Make sure commit preview updates when you stage and unstage files/hunks/lines
  • When several are open (each corresponding to a different project repo
    • check commit preview functionality
    • check all file patch view behavior
  • Open in file
    • <> button for file that isn't selected
  • Discarding/undoing discard
    • Discard button in hunk header for hunk that is not selected
  • Jumping to mirrored file
  • view and unstage symlink changes from the commit preview
  • view and unstage file mode changes from the commit preview

Documentation

  • Update the component atlas

User Experience Research (Optional)

  • Verify change to button wording for discoverability.
@coveralls

This comment has been minimized.

coveralls commented Oct 30, 2018

Coverage Status

Coverage decreased (-0.08%) to 84.851% when pulling cca6f09 on commit-preview into 6d18d72 on master.

@coveralls

This comment has been minimized.

coveralls commented Oct 30, 2018

Coverage Status

Coverage increased (+0.6%) to 85.572% when pulling 8e5d9ee on commit-preview into f16266d on master.

Show resolved Hide resolved styles/commit-preview-view.less Outdated
Show resolved Hide resolved styles/commit-preview-view.less Outdated
Show resolved Hide resolved lib/views/file-patch-view.js Outdated
@simurai

Some questions that might already came up in Slack:

  • Should it be a pending pane? It feels like the Commit Preview is something more temporary. Or do people want to keep it open all the time, maybe in a split view?
  • Similar and maybe depends on the above, should the Commit Preview pane get automatically closed after committing? Maybe not if there are more unstaged changes since you might want to make a 2nd commit next.
  • Should the Commit Preview button be a 3-way toggle? Similar how the Git and GitHub status bar buttons work. If the Commit Preview pane is already open, but not visible (the active tab), users might just want to focus it again, and not close it altogether.
  • Should we remove the blinking cursor? And use the default mouse cursor? To not make it look like the diff is editable? Here some options: #1720 (comment)
@annthurium

This comment has been minimized.

Contributor

annthurium commented Nov 1, 2018

addressing @simurai's questions:

Should it be a pending pane? It feels like the Commit Preview is something more temporary. Or do people want to keep it open all the time, maybe in a split view?

@kuychaco brought this up too. I'm not really sure what the difference is between a pending pane and a non pending pane, tbh. I'll learn about that and come back with a more informed opinion.

Similar and maybe depends on the above, should the Commit Preview pane get automatically closed after committing? Maybe not if there are more unstaged changes since you might want to make a 2nd commit next.

Yeah, I think we should not close the preview automatically, for that reason.

Should the Commit Preview button be a 3-way toggle? Similar how the Git and GitHub status bar buttons work. If the Commit Preview pane is already open, but not visible (the active tab), users might just want to focus it again, and not close it altogether.

Is your question prompted by performance concerns? I think the API we're using hides the pane, rather than destroying it.

Should we remove the blinking cursor? And use the default mouse cursor? To not make it look like the diff is editable? Here some options: #1720 (comment)

It depends on when editable diffs are on the roadmap. If we're going to do that soon (like before the end of the year), it might not make sense to implement this. Otherwise yeah, it's confusing if the diff looks like it's editable and it's not.

@simurai

This comment has been minimized.

Member

simurai commented Nov 2, 2018

I'm not really sure what the difference is between a pending pane and a non pending pane

Sorry, here a gif that tries to show the issue:

pending

Notice that when clicking on individual files, Atom automatically closes the tab of the previous file. But after opening the "Commit Preview" and clicking on a single file, Atom opens a new tab for that file and keeps the "Commit Preview" tab permanently opened. It's not necessarily a "bug", some people might prefer this behaviour. Ohh.. btw. "pending pane" is a core setting, that can be turned off/on:

screen shot 2018-11-02 at 11 45 59 am


Is your question prompted by performance concerns?

More a UX concern. Actually, it now works like a 3-way toggle. 👍👍👍 Just that the label of the button "Close Commit Preview" could be misleading. Example:

  1. The "Commit preview" tab is open, but not the active tab
  2. When clicking on "Close Commit Preview", it doesn't close the "Commit preview" tab, it makes it the active tab. Which is the right behaviour, just wrong button label.

toggle

Maybe "Close Commit Preview" could be renamed to "Toggle Commit Preview". Then the button label doesn't need to be updated when the "Commit Preview" tab isn't the active tab.


Re. button label. I asked about it in our design review. Some felt it's confusing that it says "Commit Preview", because it feels more like a "Show all staged changes". Curious if more people feel that way too.


Otherwise yeah, it's confusing if the diff looks like it's editable and it's not.

Ok, I might play around a bit to see if it can be made more "read-only" looking.


Anyways, since this PR is our first exploration into multi-file diffs, we don't have to iron out every little detail. 😅 It might be easier to make certain decisions after using this feature for a while.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Nov 2, 2018

Re. button label. I asked about it in our design review. Some felt it's confusing that it says "Commit Preview", because it feels more like a "Show all staged changes". Curious if more people feel that way too.

Hmm, interesting. I can see your point about "commit preview" being unclear... but I feel like "show all staged changes" doesn't do enough to convey why you want to do that. It's also a bit wordy.

Maybe its proximity to the commit editor and commit button will be enough to signal the "why"?

smashwilson and others added some commits Nov 7, 2018

smashwilson and others added some commits Nov 7, 2018

Merge remote-tracking branch 'origin' into pr-1767/atom/commit-preview
Co-Authored-By: Katrina Uychaco <katrina@github.com>
@kuychaco

This comment has been minimized.

Member

kuychaco commented Nov 17, 2018

[Edit]: I fixed the unstaging issue. Now the only thing that is left is the funky rendering.

Found another bug. It appears when the Commit Preview has a mode-change patch with no hunks, and then another patch below it with changed lines:

commit_preview_ ___src_repo-symlinks

Unstaging lines throws an error:

git-shell-out-strategy.js? [sm]:33 Uncaught (in promise) GitError: git apply --cached - in /Users/kuychaco/src/test-repo exited with code 128
stdout: 
stderr: error: patch with only garbage at line 4

    at new GitError (/Users/kuychaco/github/github/lib/git-shell-out-strategy.js:35:18)
    at Promise (/Users/kuychaco/github/github/lib/git-shell-out-strategy.js:329:23)
    at <anonymous>

Here's the diagnostic output:

commit_preview_ ___src_repo-symlinks

Edit: Note that this issue goes away if the mode-change patch is not empty, that is, if it has hunk lines then the alignment and staging is just fine...

And the rendering funkiness seems to correlate with the tiling that the atom text editor does:

tiling

kuychaco and others added some commits Nov 17, 2018

fix integration tests for file patch
test needed to be updated to reflext new getNextSelectionRange logic 
added in 1685484

Co-Authored-By: Katrina Uychaco <katrina@github.com>
add test for `surfaceToCommitPreviewButton` in `RootController`
Co-Authored-By: Katrina Uychaco <katrina@github.com>
test that getFileMode returns the correct file mode for symlinks
Co-Authored-By: Katrina Uychaco <katrina@github.com>
test file mode for executable files in git-strategies test
Co-Authored-By: Katrina Uychaco <katrina@github.com>
test that repository.updateCommitMessageAfterFileSystemChange handles…
… events with no path property

Co-Authored-By: Katrina Uychaco <katrina@github.com>
maybe make windows filemode happy?
Co-Authored-By: Katrina Uychaco <katrina@github.com>
istanbul ignore test-only code
Co-Authored-By: Katrina Uychaco <katrina@github.com>
fix misaligned line numbers in file mode changes
Co-Authored-By: Katrina Uychaco <katrina@github.com>
Co-Authored-By: simurai <simurai@users.noreply.github.com>
Check that `getFileMode` works for untracked symlink files
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from In Progress 🔧 to QA Review 🔬 Nov 20, 2018

@kuychaco

I reviewed all of the application logic changes since my last review, everything looks good!

I also reviewed a bunch of the test files, but eventually decided there are waaaay too many for me to look at and it's probably fine to merge them without looking super closely. They're for insurance, and not as important as the application logic (which can actually contain bugs and regressions). The most important thing around tests is that we have coverage for all new lines, which we do (thanks coveralls!).

Thanks everyone for your fantastic work with this PR! GO TEAM!

@kuychaco kuychaco merged commit d403789 into master Nov 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 85.572%
Details

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from QA Review 🔬 to Merged ☑️ Nov 20, 2018

@kuychaco kuychaco deleted the commit-preview branch Nov 20, 2018

@simurai simurai referenced this pull request Nov 21, 2018

Open

Commit Preview tweaks #1805

@vanessayuenn vanessayuenn referenced this pull request Nov 21, 2018

Closed

v0.23.0-0 QA Review #1806

5 of 5 tasks complete

@simurai simurai referenced this pull request Nov 27, 2018

Open

Commit Preview #499

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