Open diff from editor #879

kuychaco · 2017-05-26T15:50:43Z

Closes #888
Closes #46

This PR adds commands for easily viewing the diff associated with the active editor:

github:view-unstaged-changes-for-current-file
github:view-staged-changes-for-current-file

The RootController class

grabs the active editor
determines the cursor location
selects the associated item in the list view
opens the staged or unstaged file diff associated with the file
selects the closest added hunk line based on cursor position and scrolls it into view if necessary

As part of this the FilePatchSelection API was extended to include a goToDiffLine(lineNumber) method which selects the closest added line based on the passed in line number. This can be used for a github:go-to-diff-line command similar to Atom's go-to-line functionality in editors. This is outside of the scope of the current PR, and may be implemented separately at a later point.

simurai · 2017-05-27T05:21:45Z

Great addition. 😍

Should it open in a split pane?

Also, I wonder if this replaces the "stage from editor" #46? For very small changes, it's ok to "blindly" stage without seeing a diff. But otherwise it's nice be able to take a quick look what you're going to stage.

I think eventually, we should still offer some kind of "quick commit" option, but that's for another PR.

This makes more sense architecturally since viewing the (un)staged changes for files is not a concern of the git tab

kuychaco · 2017-05-30T14:29:32Z

Should it open in a split pane?

Good idea @simurai. Added a config option (similar to what we have for find-and-replace results) that defaults to opening in the current pane, but gives users the option of splitting right or down

Also, I wonder if this replaces the "stage from editor" #46?

Yeah, I think this is a good alternative/replacement to staging directly from the editor. You think it's cool if I close out that issue with this PR?

smashwilson

One typo and a bunch of idle thoughts about further work from me. 👍

smashwilson · 2017-05-30T15:29:30Z

lib/controllers/git-tab-controller.js

@@ -364,7 +364,7 @@ export default class GitTabController {

  @autobind
  quietlySelectItem(filePath, stagingStatus) {
-    return this.refs.gitTab.quitelySelectItem(filePath, stagingStatus);
+    return this.refs.gitTab.quietlySelectItem(filePath, stagingStatus);


smashwilson · 2017-05-30T15:30:29Z

lib/controllers/root-controller.js

@@ -43,7 +43,7 @@ export default class RootController extends React.Component {
  static propTypes = {
    workspace: PropTypes.object.isRequired,
    commandRegistry: PropTypes.object.isRequired,
-    notificationManager: PropTypes.object.isRequired,
+    notificationManager: PropTypes.object.isReuired,


☝️ It looks like this one was spelled right the first time? Caught a stray backspace keypress maybe?

Nice catch!

smashwilson · 2017-05-30T15:40:41Z

lib/controllers/root-controller.js

+        amending: stagingStatus === 'staged' && this.state.isAmending,
+      });
+    } else {
+      throw new Error(`${absFilePath} does not belong to repo ${repoPath}`);


Thinking this through: because the active text editor sets the active repository context, the only time that this could happen is if it's triggered in the window between opening an editor on a file not in the active repository and the render context change completing. Performance captures indicate that window is only in the tens of milliseconds range, so it's unlikely that anyone could hit this manually.

Eventually we might want to add logic to the effect of "wait on a Promise that resolves when the active context is what we expect it to be, then trigger diveIntoFilePatchForPath." Not worth holding up an initial merge for though I think.

Just adding my two cents — my gut reaction is to feel slightly uncomfortable with assuming that the active repository is the one we want to use for all operations. For example, I've wanted the ability to "pin" a repo so that it's always active for a while now. On a related note, I think that the diff views should render irrespective of the active repository; that is, they set and maintain their own context outside of the "active" one.

smashwilson · 2017-05-30T15:42:06Z

lib/controllers/root-controller.js

+
+  @autobind
+  viewUnstagedChangesForCurrentFile() {
+    this.viewChangesForCurrentFile('unstaged');


I'd ❤️ to turn 'unstaged' and 'staged' into Symbols or another more enum-ish, non-String type.

Great suggestion. I'll take a look into refactoring in a separate PR :)

smashwilson · 2017-05-30T15:43:23Z

lib/views/file-patch-selection.js

+      }
+    }
+
+    return this.selectLine(closestLine);


👍 This is pretty cool. 😄

smashwilson · 2017-05-30T15:44:15Z

lib/controllers/root-controller.js

+  @autobind
+  goToDiffLine(lineNumber) {
+    return this.filePatchController.goToDiffLine(lineNumber);
+  }


Passing actions down the component hierarchy like this really feels like a "we should start looking at introducing Redux" smell to me 😆

Yeah this felt icky to me. IIRC @BinaryMuse looked into Redux and dismissed using it for some reason. Can't remember what

The way we manage model data doesn't suit itself well to Redux in my opinion. I also wanted to live with explicit props for a while to ensure we didn't switch to something a little more magical unless we really felt the pain and needed to — the abstraction, like any other, has a cost, and already the more meta-programmy parts of the package are the hardest to understand.

I do think we could use the other half of Redux — actions on the context — to good effect in the package. For example, anything that lives on the AtomEnvironment global could be placed on the context and components that need them could be wrapped in an HoC that accesses them (but allows them to be passed explicitly as props in tests). And other methods that perform actions that do not directly modify state and that do not affect rendering could be passed on context, as long as we're careful and selective about it.

smashwilson · 2017-05-30T15:45:06Z

lib/views/git-tab-view.js

@@ -182,7 +182,7 @@ export default class GitTabView {
  }

  @autobind
-  quitelySelectItem(filePath, stagingStatus) {
+  quietlySelectItem(filePath, stagingStatus) {


Haha oh wow. How many times have my eyes just glossed riiight over this one 😆

smashwilson · 2017-05-30T15:46:48Z

menus/git.cson

+    {
+      'label': 'View Staged Changes',
+      'command': 'github:view-staged-changes-for-current-file'
+    }


Any opinions on keybindings to roll into #790 for these events?

…ditor

smashwilson · 2017-05-30T18:22:49Z

Fixes #889 instead of #888 😁

kuychaco added 2 commits May 29, 2017 13:13

Not quitely, but "quietly"

18b813b

Add commands to view changes for current file

cbef837

kuychaco force-pushed the ku-open-diff-from-editor branch from 5cf52fb to 6f1aba3 Compare May 29, 2017 13:30

Go to selected line in diff

a1541b2

kuychaco force-pushed the ku-open-diff-from-editor branch from 6f1aba3 to a1541b2 Compare May 29, 2017 14:58

kuychaco added 5 commits May 30, 2017 13:31

Pull up logic from GitTabController to RootController

5f24906

This makes more sense architecturally since viewing the (un)staged changes for files is not a concern of the git tab

Add integration test for viewing diffs from editor

fd1151c

👕

585eac3

Add test for FilePatchSelection#goToDiffLine(lineNumber)

c83daf2

Add context menu items

3dc10db

This was referenced May 30, 2017

go-to-diff-line for jumping directly to a specified hunk line #887

Open

Work with lot of files #845

Closed

kuychaco added 2 commits May 30, 2017 15:46

Scroll selected line into view in HunkView#componentDidUpdate

88974c2

Add config for opening diff in split pane and some 🎨

db32f08

kuychaco force-pushed the ku-open-diff-from-editor branch 2 times, most recently from d25e493 to ab16969 Compare May 30, 2017 13:57

Focus file patch view by using diveIntoFilePatchForPath

378cf85

kuychaco force-pushed the ku-open-diff-from-editor branch from e3007d4 to 378cf85 Compare May 30, 2017 14:21

Merge branch 'master' into ku-open-diff-from-editor

64c5eb0

smashwilson suggested changes May 30, 2017

View reviewed changes

Oops typo

e0f7235

smashwilson approved these changes May 30, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into ku-open-diff-from-e…

b33444a

…ditor

kuychaco merged commit 906a7c1 into master May 30, 2017

kuychaco deleted the ku-open-diff-from-editor branch May 30, 2017 16:31

smashwilson mentioned this pull request May 30, 2017

View staged/unstaged changes for current file #889

Closed

Open diff from editor #879

Open diff from editor #879

Uh oh!

Conversation

kuychaco commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simurai commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuychaco commented May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smashwilson commented May 30, 2017

Uh oh!

Uh oh!

kuychaco commented May 26, 2017 •

edited

Loading

simurai commented May 27, 2017 •

edited

Loading

kuychaco commented May 30, 2017 •

edited

Loading