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

Use Atom opener to display file diff #1016

Merged
merged 126 commits into from Jul 31, 2017

Conversation

Projects
None yet
6 participants
@kuychaco
Member

kuychaco commented Jul 6, 2017

This PR includes a refactor of how we display diff views as pane items. It introduces an FilePatchPaneItem and registers an opener to Atom's built in file opener system. This refactor was driven by the following code and UX improvements:

  • straight-forward way of displaying and activating diff views based on a pre-existing system
    • built-in support of pending pane items
    • ability to view multiple diff views at once
    • next selected diff is automatically shown after staging/unstaging - fixes #645
  • reduced state and complexity in RootController
  • FilePatchController manages its own state and logic
  • Iterated on our system for bridging Atom's imperative API that synchronously opens items with the Github package's declarative system for rendering an eventually consistent view
  • Consistency across how we open and close pane items across our panels and diff views
  • Supports deserialization, fixes bug with multiple panel items
  • Simplifies tests

Fixes #854
Fixes #645
Addresses #864
Supersedes #970

  • tests
  • check amending
  • check discard and undo last discard
  • fix isChanged error after discarding line and then discarding again
  • deserialization breaks stuff
  • add command for showing file diff without focusing
  • handle multiple repos - update active repo when file diff is active pane item
  • update staging view selection when selecting FilePatchPaneItems in diff repos
  • handle when repo is removed from project (works fine already)
  • handle error when item with no corresponding changed file is selected Uncaught (in promise) Error: Unable to find item at path file1.txt with staging status staged
  • clicking anywhere cause hunks to re-render - issue?
  • scroll into view for view unstated changes does not work
  • close file diffs when committing
  • why are there multiple repo refreshes?

kuychaco added some commits Jul 5, 2017

Pass file patch object to `discardLines`
This is so that we can remove the filePatch state from RootController
Don't throw error if item cannot be found in quietlySelectItem
This happens when a non-pending diff view has no file patch associated
with it and we try to quietly select the associated non-existent file in
the StagedChanges view
Add onDidDestroy to FilePatchPaneItem
This is necessary so that the tab associated with the item disappears
when the item is destroyed
@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Jul 7, 2017

Member

Should double-clicking a pending diff view make it not-pending? Currently I think it's only possible by move it into a split view.

Otherwise, this feels great.

Edit: One more small thing. After staging a hunk with the keyboard (enter), the focus is lost and the arrow keys don't work anymore. You can press tab and focus moves to the <> code button and then the arrow keys work again.

focus

Member

simurai commented Jul 7, 2017

Should double-clicking a pending diff view make it not-pending? Currently I think it's only possible by move it into a split view.

Otherwise, this feels great.

Edit: One more small thing. After staging a hunk with the keyboard (enter), the focus is lost and the arrow keys don't work anymore. You can press tab and focus moves to the <> code button and then the arrow keys work again.

focus

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 7, 2017

Member

I've apm link'd this branch and I'll let you know what bugs I see. 😁

Here's something odd that I'm seeing now: somehow, I've put myself in a state where the FilePatchPaneItem is showing the "unstage" controls, even though the selected file is unstaged:

screen shot 2017-07-07 at 8 46 04 am

Interacting with the "unstage" controls gives me this stacktrace:

Error: Unknown stagingStatus: bootstrap
    at FilePatchController.stageOrUnstageLines (/Users/smashwilson/src/atom-github/file-patch-controller.js:261:13)
    at FilePatchController.attemptLineStageOperation (/Users/smashwilson/src/atom-github/file-patch-controller.js:276:10)
    at FilePatchView.didClickStageButtonForHunk (/Users/smashwilson/src/atom-github/file-patch-view.js:414:18)
    at FilePatchView.didConfirm (/Users/smashwilson/src/atom-github/file-patch-view.js:434:17)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (/Applications/Atom.app/Contents/Resources/app/src/command-registry.js:265:35)
    at /Applications/Atom.app/Contents/Resources/app/src/command-registry.js:3:65
    at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (/Applications/Atom.app/Contents/Resources/app/node_modules/atom-keymap/lib/keymap-manager.js:621:22)
    at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (/Applications/Atom.app/Contents/Resources/app/node_modules/atom-keymap/lib/keymap-manager.js:412:28)
    at WindowEventHandler.module.exports.WindowEventHandler.handleDocumentKeyEvent (/Applications/Atom.app/Contents/Resources/app/src/window-event-handler.js:100:42)
    at HTMLDocument.<anonymous> (/Applications/Atom.app/Contents/Resources/app/src/window-event-handler.js:3:65)

(Notice that the "Unknown stagingStatus" is the basename of the selected file; it changes to atom_io.rb if I interact with the other filepatch.) Possibly related: I was playing around with having multiple FilePatchViews open at once before I noticed this. Maybe I messed up some state that's still tracked as though there's only a single FilePatchView?

Member

smashwilson commented Jul 7, 2017

I've apm link'd this branch and I'll let you know what bugs I see. 😁

Here's something odd that I'm seeing now: somehow, I've put myself in a state where the FilePatchPaneItem is showing the "unstage" controls, even though the selected file is unstaged:

screen shot 2017-07-07 at 8 46 04 am

Interacting with the "unstage" controls gives me this stacktrace:

Error: Unknown stagingStatus: bootstrap
    at FilePatchController.stageOrUnstageLines (/Users/smashwilson/src/atom-github/file-patch-controller.js:261:13)
    at FilePatchController.attemptLineStageOperation (/Users/smashwilson/src/atom-github/file-patch-controller.js:276:10)
    at FilePatchView.didClickStageButtonForHunk (/Users/smashwilson/src/atom-github/file-patch-view.js:414:18)
    at FilePatchView.didConfirm (/Users/smashwilson/src/atom-github/file-patch-view.js:434:17)
    at CommandRegistry.module.exports.CommandRegistry.handleCommandEvent (/Applications/Atom.app/Contents/Resources/app/src/command-registry.js:265:35)
    at /Applications/Atom.app/Contents/Resources/app/src/command-registry.js:3:65
    at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (/Applications/Atom.app/Contents/Resources/app/node_modules/atom-keymap/lib/keymap-manager.js:621:22)
    at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (/Applications/Atom.app/Contents/Resources/app/node_modules/atom-keymap/lib/keymap-manager.js:412:28)
    at WindowEventHandler.module.exports.WindowEventHandler.handleDocumentKeyEvent (/Applications/Atom.app/Contents/Resources/app/src/window-event-handler.js:100:42)
    at HTMLDocument.<anonymous> (/Applications/Atom.app/Contents/Resources/app/src/window-event-handler.js:3:65)

(Notice that the "Unknown stagingStatus" is the basename of the selected file; it changes to atom_io.rb if I interact with the other filepatch.) Possibly related: I was playing around with having multiple FilePatchViews open at once before I noticed this. Maybe I messed up some state that's still tracked as though there's only a single FilePatchView?

@kuychaco

This comment has been minimized.

Show comment
Hide comment
@kuychaco

kuychaco Jul 7, 2017

Member

Thanks for all the feedback so far! One other thing to note - things are buggy when you keep diff views from multiple repos open. I think this primarily has to do with not updating the active repo to reflect the active pane item. Will handle this case shortly

Member

kuychaco commented Jul 7, 2017

Thanks for all the feedback so far! One other thing to note - things are buggy when you keep diff views from multiple repos open. I think this primarily has to do with not updating the active repo to reflect the active pane item. Will handle this case shortly

Make `stagingStatus` a query parameter
Previously file paths with `/` weren't being handled correctly
@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 7, 2017

Member

2323b97 fixed the issue I had this morning 🙇

Member

smashwilson commented Jul 7, 2017

2323b97 fixed the issue I had this morning 🙇

@jasonrudolph

This comment has been minimized.

Show comment
Hide comment
@jasonrudolph

jasonrudolph Jul 10, 2017

Member

I've apm link'd this branch and I'll let you know what bugs I see. 😁

Same here. 😸

So far, I've just noticed one regression: It seems that you have to click on a file in the staging view in order for the package to display the diff view:

demo

In the latest production release (0.3.5) of this package, the package immediately displays the diff view for the currently selected file in the staging view. As you use the keyboard to navigate up/down the list of files in the staging view, the package displays the diff view for the currently-selected file.

Member

jasonrudolph commented Jul 10, 2017

I've apm link'd this branch and I'll let you know what bugs I see. 😁

Same here. 😸

So far, I've just noticed one regression: It seems that you have to click on a file in the staging view in order for the package to display the diff view:

demo

In the latest production release (0.3.5) of this package, the package immediately displays the diff view for the currently selected file in the staging view. As you use the keyboard to navigate up/down the list of files in the staging view, the package displays the diff view for the currently-selected file.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Jul 20, 2017

with pending pane unchecked, I wonder if we should close the pane item if there's nothing left. Or alternatively have one item for Unstaged Changes and one for Stage Changes that get populated. If people are going through a lot of stuff they could have multiple windows open that have File has no content.

One other issue is if you stage or unstage hunk using the keyboard shortcut and that's the only hunk you lose the ability to navigate with the keyboard. If you just stay in the git panel, staging and unstaging works fine without issues with keyboard.

ungb commented Jul 20, 2017

with pending pane unchecked, I wonder if we should close the pane item if there's nothing left. Or alternatively have one item for Unstaged Changes and one for Stage Changes that get populated. If people are going through a lot of stuff they could have multiple windows open that have File has no content.

One other issue is if you stage or unstage hunk using the keyboard shortcut and that's the only hunk you lose the ability to navigate with the keyboard. If you just stay in the git panel, staging and unstaging works fine without issues with keyboard.

@ungb

This comment has been minimized.

Show comment
Hide comment

ungb commented Jul 20, 2017

image

kuychaco added some commits Jul 20, 2017

@kuychaco kuychaco merged commit 6719293 into master Jul 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kuychaco kuychaco deleted the ku-next-diff-please branch Jul 31, 2017

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Jul 31, 2017

Member
Member

BinaryMuse commented Jul 31, 2017

@jasonrudolph

This comment has been minimized.

Show comment
Hide comment
@jasonrudolph

jasonrudolph Aug 1, 2017

Member

@kuychaco: 👏⚡️ Would you be up for releasing a new version of the github package with this change and updating atom/atom to use the new version?

Member

jasonrudolph commented Aug 1, 2017

@kuychaco: 👏⚡️ Would you be up for releasing a new version of the github package with this change and updating atom/atom to use the new version?

@BinaryMuse BinaryMuse referenced this pull request Aug 1, 2017

Merged

⬆ github@0.3.9-0 #15137

@kuychaco kuychaco changed the title from [WIP] Use Atom opener to display file diff to Use Atom opener to display file diff Aug 1, 2017

@BinaryMuse BinaryMuse referenced this pull request Aug 1, 2017

Closed

Hotfix from v0.3.3 #1073

9 of 25 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment