Fix logic for automatically opening diff views based on selection change #1311

kuychaco · 2018-02-15T20:57:04Z

Problem: We’re seeing surprised diff views popping up unexpectedly, such as in the following scenarios

Scenario 1: see steps in #1287
Scenario 2: have two repos open in project. have pending diff in pane 2 corresponding to repo 2. in pane 1 focus a file from repo 1, then focus a file from repo 2. note that new diff view opens.
Scenario 3: only have one repo. have pending diff in pane 2. focus pane 1. focus staging view (be sure not to click on a file). use arrow key to change selection (up or down). note that new diff view pops up in pane 1. The only time we want to open a brand new pending diff view in a pane is if the user explicitly expresses intent, such as by clicking the file (see 1 below).

These point to bugs in our logic for automatically opening diff views when the selection changes. We want to do this in two cases:

when navigating up/down and there is a pending diff view open in the active pane, update the diff view to reflect the new selection. This enables users to quickly glance at the change they've made
when there is a pending diff view open and the file associated with it no longer has changes (such as when changes are staged or deleted), then display the next diff view in its place according to what is selected in the staging view. This was a highly requested feature so that one could quickly stage using the keyboard by hitting enter multiple times. Or hit the “Stage File” button in a pending diff view in rapid sequence.

So let’s get clear on the circumstances under which we do want a pending diff item to be opened.

In order to get this behavior right, we need to get more specific around when/what/how exactly we want to automatically show the next file.

If there are stale pending pane items corresponding to a file that no
longer exists in the changed files list, update them to reflect the new
selection.

When? The following criteria must be satisfied

A staging action has taken place, the repo updates, the staged file no longer appears in the staged changes list, and a new file is selected
There is at least one pending diff view open corresponding to the file that has just been staged. Multiple pending diff views can be open across multiple panes, must check all
- since we can’t easily know which file has been staged, we need to deduce this somehow
  - diff view no longer has staging view file associated with it
  - diff view belongs to active repo (check to discern between staging action and repo change)

What? What parts of the workspace should be affected?

Any pane that has a pending diff view that meets the criteria above should be updated such that its pending item corresponds to the new selection in the staging view
- This behavior is consistent with the fact that staging lines/hunks in a diff view affects all diff views for that file open in other panes

How? Implementation details for updating pending items according to what is outlined above

Check to see when there is a change in the selected file in the staging view
Identify the panes that have pending diff views that no longer have staging view files associated with them and belong to the active repo
For each pane, call workspace.open with the newly selected file and pass the pane as an option, as well as pending: true. This will cause the old diff view to be replaced

TODO:

maintain selection across repo changes.
repro steps for bug: open diff view for repo 1 that is not the first one listed. make repo 2 active. click diff view for repo 1 and note that it changes to be first file listed.

If there are stale pending pane items corresponding to a file that no longer exists in the changed files list, update them to reflect the new selection. If the selection in the changed file list has changed due to keyboard nav and there is a pending diff view in the active pane, update the diff view to reflect the new selection.

kuychaco · 2018-02-15T23:12:39Z

lib/views/staging-view.js

+      } else {
+        const panesWithStaleItemsToUpdate = this.getPanesWithStalePendingFilePatchItem();
+        if (panesWithStaleItemsToUpdate.length > 0) {
+          // Staging event caused selection to change, so we need to update all old diff views to reflect new selection


Not just when staging. also when deleting files and stashing

kuychaco · 2018-02-15T23:25:45Z

lib/views/staging-view.js

      }
    }
  }

-  async showFilePatchItem(filePath, stagingStatus, {activate} = {activate: false}) {
+  getPanesWithStalePendingFilePatchItem() {
+    // "stale" meaning there is no longer a changed file associated with it due to contents being fully staged/unstaged


again, not just when staging/unstaging

kuychaco · 2018-02-15T23:57:13Z

test/views/staging-view.test.js

        await view.selectNext();
        assert.isFalse(showFilePatchItem.called);

-        getPendingFilePatchItem.returns(true);
+        getPanesWithStalePendingFilePatchItem.returns(['pending-file-patch-item']);


have multiple array items to improve this and test the code that runs showFilePatchItem for multiple panes

kuychaco · 2018-02-16T00:07:41Z

lib/views/staging-view.js

+      const pendingItem = pane.getPendingItem();
+      if (!pendingItem) { return false; }
+      const isDiffViewItem = pendingItem.getRealItem && pendingItem.getRealItem() instanceof FilePatchController;
+      const isInActiveRepo = pendingItem.getWorkingDirectory() === this.props.workingDirectoryPath;


add comment Diff views from repositories other than the active one do not count as stale. Ex: ...

smashwilson

I've found one thing that looks like a typo which should probably be changed. Other than that, I'd say the rest of this PR is ready to ✨

smashwilson · 2018-02-24T04:38:21Z

lib/views/staging-view.js

+      this.quietlySelectItem(this.activeFilePatch.getFilePath(), this.activeFilePatch.getStagingStatus());
+    } else {
+      const isRepoSame = oldProps.workingDirectoryPath === this.props.workingDirectoryPath;
+      const selectionsPresent = previouslySelectedItems.size > 0 && currentlySelectedItems > 0;


🚨 It looks like currentlySelectedItems is an Array. Did you mean currentlySelectedItems.length > 0?

Also: the test suite is ✅ even though this is here. Does that mean we're missing a test case, or does it mean that the currentlySelectedItems isn't actually needed?

smashwilson · 2018-02-24T04:39:41Z

lib/views/staging-view.js

-        const activate = pendingItem === this.props.workspace.getActivePaneItem();
-        await this.showFilePatchItem(selectedItem.filePath, this.selection.getActiveListKey(), {activate});
+      if (openNew) {
+        // User explicitly asked to view diff, such as via click


👍

I like explicitly separating the logic between "the user explicitly triggered this" and "we're inferring that this should happen."

smashwilson · 2018-02-24T04:41:49Z

lib/views/staging-view.js

+          const activePane = this.props.workspace.getCenter().getActivePane();
+          const activePendingItem = activePane.getPendingItem();
+          const activePaneHasPendingFilePatchItem = activePendingItem && activePendingItem.getRealItem &&
+            activePendingItem.getRealItem() instanceof FilePatchController;


instanceof checks always make me feel vaguely uncomfortable. I think they raise my "OO code smell" instincts 😄

(No useful alternatives to suggest here, though; we don't have much choice when we're dealing with arbitrary PaneItems.)

smashwilson · 2018-02-24T04:46:22Z

lib/views/staging-view.js

    const encodedFilePath = encodeURIComponent(filePath);
    const encodedWorkdir = encodeURIComponent(this.props.workingDirectoryPath);
    const filePatchItem = await this.props.workspace.open(
      `atom-github://file-patch/${encodedFilePath}?workdir=${encodedWorkdir}&stagingStatus=${stagingStatus}`,
-      {pending: true, activatePane: activate, activateItem: activate},
+      {pending: true, activatePane: activate, activateItem: activate, pane},


Ah, it'll be good to control the Pane here.

... I still want to do that change in core to be able to specify "categories" of pending pane, so that there's only one pending pane of each category at once and we can keep ours independent from tree-view's.

smashwilson · 2018-02-24T04:48:32Z

test/views/staging-view.test.js

        await view.selectPrevious();
+        assert.isTrue(showFilePatchItem.calledTwice);


Since this is .calledTwice, shouldn't there be two assert.isTrue(showFilePatchItem.calledWith(...)) calls below it to assert that they're both correct? Or are the other calls too annoying to assert specifically? 🤔

Same for the next stanza there 👇

smashwilson · 2018-02-24T04:49:29Z

test/views/staging-view.test.js

        await view.selectPrevious();
        await assert.async.isTrue(showFilePatchItem.calledWith(filePatches[0].filePath));
+        assert.isTrue(showFilePatchItem.calledThrice);


Hehe. Times you get to use the word "thrice" in source code++

…pdate

smashwilson · 2018-03-05T13:07:43Z

Nice ✨

kuychaco commented Feb 15, 2018

View reviewed changes

kuychaco commented Feb 16, 2018

View reviewed changes

kuychaco added 5 commits February 15, 2018 16:24

WIP update selected item when focusing pane item

807b5bd

🎨 logic for selecting corresponding file when diff view is active item

6968ecb

🎨 comments

e0627d2

Test for multiple stale pending items

9a7b2b4

👕

dda105a

kuychaco requested a review from smashwilson February 19, 2018 01:25

smashwilson suggested changes Feb 24, 2018

View reviewed changes

kuychaco and others added 5 commits March 4, 2018 22:38

Test automatically updating selection due to repo changes

0810dfb

Fix bug in check to see if selections were present before and after u…

8010d8a

…pdate

More thoroughly check for arguments to spy

4ecc5f3

Add comment about items being stale only if they belong to active repo

09b7822

Merge branch 'master' into ku-no-more-suprise-diffs-please

7a34f57

smashwilson approved these changes Mar 5, 2018

View reviewed changes

smashwilson merged commit 2fa2492 into master Mar 5, 2018

smashwilson deleted the ku-no-more-suprise-diffs-please branch March 5, 2018 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix logic for automatically opening diff views based on selection change #1311

Fix logic for automatically opening diff views based on selection change #1311

kuychaco commented Feb 15, 2018 •

edited

Loading

kuychaco Feb 15, 2018 •

edited

Loading

kuychaco Feb 15, 2018

kuychaco Feb 15, 2018

kuychaco Feb 16, 2018

smashwilson left a comment

smashwilson Feb 24, 2018

smashwilson Feb 24, 2018

smashwilson Feb 24, 2018

smashwilson Feb 24, 2018

smashwilson Feb 24, 2018

smashwilson Feb 24, 2018

smashwilson commented Mar 5, 2018

		await view.selectPrevious();
		assert.isTrue(showFilePatchItem.calledTwice);

Fix logic for automatically opening diff views based on selection change #1311

Fix logic for automatically opening diff views based on selection change #1311

Conversation

kuychaco commented Feb 15, 2018 • edited Loading

kuychaco Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smashwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smashwilson commented Mar 5, 2018

kuychaco commented Feb 15, 2018 •

edited

Loading

kuychaco Feb 15, 2018 •

edited

Loading