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

Should return to PR list after creating a PR #805

Closed
grokys opened this Issue Jan 27, 2017 · 3 comments

Comments

Projects
3 participants
@grokys
Contributor

grokys commented Jan 27, 2017

After creating a PR from the GitHub pane, the view should switch to the PR list. Instead it remains on the "Create Pull Request" view.

@paladique

This comment has been minimized.

Show comment
Hide comment
@paladique

paladique Jan 31, 2017

Collaborator

This seems most likely to happen when pushing a new branch when making a PR

Collaborator

paladique commented Jan 31, 2017

This seems most likely to happen when pushing a new branch when making a PR

@paladique

This comment has been minimized.

Show comment
Hide comment
@paladique

paladique Jan 31, 2017

Collaborator

Thought that ⬆️ was the reason, but it actually happens any time you switch branches. I put some logging in place to try and find some common patterns while debugging. Noticed Start() in UIController is getting called twice. The call stack led to the RepoChanged method:

protected override void RepoChanged(bool changed)
{

Stays on PR Creation View

Start (54397957)
Firing Next from None (54397957)
Firing Next from None for flow PullRequestCreation (54397957)
Where::CredentialManager
   credential manager found at 'C:\Program Files\some\directory`
Start (55444801)
Firing Next from None (55444801)
Firing Next from None for flow PullRequestCreation (55444801)
VM: PR Creation Done at 1/31/2017 4:22:26 PM
VIEW: PR Creation Notified Done at 1/31/2017 4:22:26 PM
Firing Next from PRCreation (55444801)
Firing Next from PRCreation for flow PullRequestCreation (55444801)
Firing Next from End (55444801)
Firing Next from End for flow PullRequestCreation (55444801)
Disposing (55444801)
NAV: Controller Pop at 1/31/2017 4:22:26 PM
NAV: Controller reload at 1/31/2017 4:22:26 PM Busy?False
Firing Reload from PRCreation (54397957)

Goes to PR List View

Start (29498151)
Firing Next from None (29498151)
Firing Next from None for flow PullRequestCreation (29498151)
VM: PR Creation Done at 1/31/2017 4:20:36 PM
VIEW: PR Creation Notified Done at 1/31/2017 4:20:36 PM
Firing Next from PRCreation (29498151)
Firing Next from PRCreation for flow PullRequestCreation (29498151)
Firing Next from End (29498151)
Firing Next from End for flow PullRequestCreation (29498151)
Disposing (29498151)
NAV: Controller Pop at 1/31/2017 4:20:36 PM
NAV: Controller reload at 1/31/2017 4:20:36 PM Busy?False
Firing Reload from PRList (16458760)
Collaborator

paladique commented Jan 31, 2017

Thought that ⬆️ was the reason, but it actually happens any time you switch branches. I put some logging in place to try and find some common patterns while debugging. Noticed Start() in UIController is getting called twice. The call stack led to the RepoChanged method:

protected override void RepoChanged(bool changed)
{

Stays on PR Creation View

Start (54397957)
Firing Next from None (54397957)
Firing Next from None for flow PullRequestCreation (54397957)
Where::CredentialManager
   credential manager found at 'C:\Program Files\some\directory`
Start (55444801)
Firing Next from None (55444801)
Firing Next from None for flow PullRequestCreation (55444801)
VM: PR Creation Done at 1/31/2017 4:22:26 PM
VIEW: PR Creation Notified Done at 1/31/2017 4:22:26 PM
Firing Next from PRCreation (55444801)
Firing Next from PRCreation for flow PullRequestCreation (55444801)
Firing Next from End (55444801)
Firing Next from End for flow PullRequestCreation (55444801)
Disposing (55444801)
NAV: Controller Pop at 1/31/2017 4:22:26 PM
NAV: Controller reload at 1/31/2017 4:22:26 PM Busy?False
Firing Reload from PRCreation (54397957)

Goes to PR List View

Start (29498151)
Firing Next from None (29498151)
Firing Next from None for flow PullRequestCreation (29498151)
VM: PR Creation Done at 1/31/2017 4:20:36 PM
VIEW: PR Creation Notified Done at 1/31/2017 4:20:36 PM
Firing Next from PRCreation (29498151)
Firing Next from PRCreation for flow PullRequestCreation (29498151)
Firing Next from End (29498151)
Firing Next from End for flow PullRequestCreation (29498151)
Disposing (29498151)
NAV: Controller Pop at 1/31/2017 4:20:36 PM
NAV: Controller reload at 1/31/2017 4:20:36 PM Busy?False
Firing Reload from PRList (16458760)
@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 1, 2017

Collaborator

Ah, so it is the problem where there's two views of the same type being pushed to the navigation history. When it goes "back", it goes back to the previous creation view that was pushed. Legit bug! This is likely easy to reproduce by just clicking "Refresh" when in the creation view, and then hitting back - it's likely that it will just go back to the "previous" creation view instead of the PR list.

The NavigationController that's loading things into the history has a Reload method that refreshes views in place instead of loading a new one. When GitHubPaneViewModel asks for the PR list to be loaded, and it's already the current one, the NavigationController knows how to handle that and just reloads it in place (https://github.com/github/VisualStudio/blob/master/src/GitHub.App/Controllers/NavigationController.cs#L73).

The creation view always causes a new view to be pushed, however: https://github.com/github/VisualStudio/blob/master/src/GitHub.App/Controllers/NavigationController.cs#L69.

It's fine to create a new view in this case and just throw the current view data away, but if the new view is of the same type as the current view, then we should not be adding it twice to the history, we should be popping the existing one and pushing the new one.

Collaborator

shana commented Feb 1, 2017

Ah, so it is the problem where there's two views of the same type being pushed to the navigation history. When it goes "back", it goes back to the previous creation view that was pushed. Legit bug! This is likely easy to reproduce by just clicking "Refresh" when in the creation view, and then hitting back - it's likely that it will just go back to the "previous" creation view instead of the PR list.

The NavigationController that's loading things into the history has a Reload method that refreshes views in place instead of loading a new one. When GitHubPaneViewModel asks for the PR list to be loaded, and it's already the current one, the NavigationController knows how to handle that and just reloads it in place (https://github.com/github/VisualStudio/blob/master/src/GitHub.App/Controllers/NavigationController.cs#L73).

The creation view always causes a new view to be pushed, however: https://github.com/github/VisualStudio/blob/master/src/GitHub.App/Controllers/NavigationController.cs#L69.

It's fine to create a new view in this case and just throw the current view data away, but if the new view is of the same type as the current view, then we should not be adding it twice to the history, we should be popping the existing one and pushing the new one.

@grokys grokys added this to Not Started in 2.2.0.8 Feb 2, 2017

@paladique paladique moved this from Not Started to In Progress in 2.2.0.8 Feb 8, 2017

@grokys grokys closed this in #810 Feb 10, 2017

@paladique paladique moved this from In Progress to Done in 2.2.0.8 Feb 10, 2017

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