Convert the FilePatchController hierarchy to React #617

smashwilson · 2017-03-21T15:41:16Z

Continuing the path in #381. I'm also starting to convert the selection models to be immutable along the way to avoid a bunch of this.selection.selectItem(); this.forceUpdate(); awkwardness.

Stuff I don't want to forget to do, in this PR or follow-on ones:

🔥 the registerHunkView props; Enzyme selectors work much more cleanly.
PaneItem Portal stuff
- Unit test that fails when the PaneItem doesn't render and mount properly in the workspace
- Introduce a Portal.getView() method that returns a Proxy that (a) returns the root DOM node of the portal'd content for ViewRegistry purposes, and (b) acts like the root component instance for all other methods to support getTitle(), onDidDestroy, and so on.

Deal with this warning:

warning.js:36 Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're accessing the property `metaKey` on a released/nullified synthetic event. This is set to null. If you must keep the original synthetic event around, use event.persist(). See https://fb.me/react-event-pooling for more information.

smashwilson · 2017-03-29T15:00:40Z

No test failures here, move along...

BinaryMuse

Mostly just scanned through the tests portion of this, but they looked good. Some comments below. Great job on this! 🎉

@kuychaco reviewed with me so this can count for both of us ;)

BinaryMuse · 2017-03-30T22:54:00Z

lib/controllers/file-patch-controller.js

            hunks={hunks}
            filePath={filePath}
            stagingStatus={this.props.stagingStatus}
            isPartiallyStaged={this.props.isPartiallyStaged}
-            registerHunkView={this.props.registerHunkView}


Very happy to see we can drop this 🙇

BinaryMuse · 2017-03-30T23:04:32Z

lib/helpers.js

+
+export function forceUpdatePromise(component) {
+  return new Promise(resolve => component.forceUpdate(resolve));
+}


Do we still need this?

Apparently not! 🔥

BinaryMuse · 2017-03-30T23:27:59Z

lib/views/file-patch-view.js

-export default class FilePatchView {
-  constructor(props) {
-    this.props = props;
-    this.selection = new FilePatchSelection(this.props.hunks);
    this.mouseSelectionInProgress = false;

    window.addEventListener('mouseup', this.mouseup);
    this.disposables = new CompositeDisposable();
    this.disposables.add(new Disposable(() => window.removeEventListener('mouseup', this.mouseup)));


We should probably move the event handler stuff to componentDidMount - not sure why this was listed above etch.initialize(this) in the old version.

👍 Yeah, that's better; done.

BinaryMuse · 2017-03-30T23:35:52Z

lib/views/file-patch-view.js

+      });
+    } else if (mode === 'line' && !this.state.selection.getSelectedLines().has(line)) {
+      await setStatePromise(prevState => {
+        return {selection: prevState.selection.selectLine(line, event.shiftKey)};


Do you have an idea of the performance characteristics of the immutable data models compared to the mutable version for large diffs?

According to the React docs this should move us toward being able to use PureComponent which will prevent unnecessary rendering when the selection hasn't changed. I haven't, like, measured it, though.

I was cautious to keep ListSelection.copy() from doing an O(n) copy of the items Array. Right now it is linear with respect to the number of non-coalesced selections, but once ListSelection is immutable, too, it should be able to do a shallow-copy of that as well and be nice and snappy in that regard.

BinaryMuse · 2017-03-30T23:37:25Z

lib/views/file-patch-view.js

-          this.selection.selectLine(hunkLines[hunkLines.length - 1], true);
+
+    this.mouseSelectionInProgress = true;
+    event.persist && event.persist();


Does this event need to be persisted? I don't see any async operations in which it's used.

It does! this.setState can be asynchronous, although it never is in enzyme. I only knew about this because it was throwing warnings in the console telling me to do this 😉

BinaryMuse · 2017-03-30T23:44:39Z

lib/views/file-patch-view.js

-  stageOrUnstageAll() {
-    this.selectAll();
+  async stageOrUnstageAll() {
+    await this.selectAll();


Does this need to be async?

It does because the setState call in selectAll can be asynchronous, which means that the this.didConfirm() call would fire before the selection is updated.

Buuut it turns out that selectAll() wasn't returning a Promise any more, so I should probably fix that 😉

BinaryMuse · 2017-03-30T23:50:44Z

lib/views/pane-item.js

@@ -65,7 +65,14 @@ export default class PaneItem extends React.Component {
  componentWillUnmount() {
    this.subscriptions && this.subscriptions.dispose();
    if (this.paneItem && !this.didCloseItem) {
-      this.paneItem.destroy();
+      if (this.paneItem.destroy) {
+        this.paneItem.destroy && this.paneItem.destroy();


There's a duplicate check for destroy here

BinaryMuse · 2017-03-30T23:51:35Z

lib/views/pane-item.js

+      } else {
+        const activePane = this.props.workspace.getActivePane();
+        if (activePane) {
+          activePane.destroyItem(this.paneItem);


Hmmm, am I wrong in my understanding that we can call destroyItem here and the pane item will have its destroy method called?

Also, can we assume that getting the active pane will be correct? Or should we look for the item in all panes?

Hah. This is actually me not understanding how Pane.addItem() works and forgetting to implement onDidDestroy on FilePatchController. Oops.

BinaryMuse · 2017-03-30T23:55:11Z

lib/views/portal.js

@@ -76,4 +76,31 @@ export default class Portal extends React.Component {
  getElement() {
    return this.node;
  }
+
+  getView() {


I was wondering what you'd think about this one 😁

Also, I figured I'd make the React-compatible getItem version the default, since we're moving everything to React.

BinaryMuse · 2017-03-31T00:18:17Z

test/views/file-patch-selection.test.js

-          new HunkLine('line-7', 'added', -1, 8),
-          new HunkLine('line-8', 'added', -1, 9),
+          new HunkLine('line-7', 'added', -1, 8), //
+          new HunkLine('line-8', 'added', -1, 9), //


You forgot your //

Changes made. Thanks 💯 for the review :-D

maxbrunsfeld · 2017-04-04T00:18:53Z

It looks like this caused an issue with the file patch view's height; I can't scroll vertically in the file patch view anymore.

Make FilePatchSelection immutable

6b1aaa3

smashwilson added the work-in-progress label Mar 21, 2017

smashwilson added 20 commits March 23, 2017 09:31

Port the HunkView and its tests to React

8688aa7

FilePatchView 👉 React

dbd794f

Centralize the "resolve this promise the next time X occurs" pattern.

e94187d

Ensure getXyzPromise() and resolveXyzPromise() methods align

50815ea

Support repeated events

13e1596

Handle duplicate events correctly.

c47e837

Boolean logic is hard, okay

c3bd8b8

FilePatchController tests, so far

261f00a

FilePatchController tests are ✅

40766ed

Oh hey I _did_ use that function

61fc620

RootController tests are ✅

4e8efd7

🔥 registerHunkView

bba25e4

:burn: usage of setStatePromise

b345897

RootController integration test to exercise actual PaneItem mounting

0235eb8

Portal.getView() to construct a view-compatible item

a2bdf8d

Default PaneItem's item to a view

7210d26

Use the default PaneItem getter

8fb4b0f

Judicious use of event.persist

a1068df

Correct FilePatchController focus

aac603c

Always destroy PaneItems on unmount.

6efa684

smashwilson requested a review from BinaryMuse March 28, 2017 19:53

smashwilson removed the work-in-progress label Mar 28, 2017

Work with Enzyme events

2649a99

smashwilson requested a review from kuychaco March 28, 2017 19:56

smashwilson added 2 commits March 29, 2017 10:16

Mock the FilePatch pane

89ae013

Use either item.destroy() or activePane.destroyItem()

de03a5e

BinaryMuse previously requested changes Mar 31, 2017

View reviewed changes

smashwilson mentioned this pull request Apr 1, 2017

Rework GithubPackage model and promise management #633

Merged

12 tasks

smashwilson added 6 commits April 1, 2017 12:03

🔥 setStatePromise

b9ee6d5

Event subscriptions in componentDidMount

5f5800d

selectAll needs to return a Promise

48acdb8

My // were showing

60f2f1b

🔥 duplicate .destroy() check

028d16b

Implement onDidDestroy and destroy again

f8704ac

smashwilson removed the request for review from kuychaco April 1, 2017 16:41

Merge branch 'master' into aw-file-patch-to-react

e5c871d

smashwilson merged commit a2a1c61 into master Apr 1, 2017

smashwilson deleted the aw-file-patch-to-react branch April 1, 2017 16:51

smashwilson mentioned this pull request Apr 4, 2017

Unable to vertically scroll the FilePatchView #639

Closed

smashwilson mentioned this pull request May 19, 2017

File patch selection and context menu interaction #853

Merged

Convert the FilePatchController hierarchy to React #617

Convert the FilePatchController hierarchy to React #617

Uh oh!

Conversation

smashwilson commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smashwilson commented Mar 29, 2017

Uh oh!

BinaryMuse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

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!

maxbrunsfeld commented Apr 4, 2017

Uh oh!

Uh oh!

smashwilson commented Mar 21, 2017 •

edited

Loading

BinaryMuse left a comment •

edited

Loading

BinaryMuse Mar 30, 2017 •

edited

Loading