Maintainer workflow #567

grokys · 2016-09-28T12:09:15Z

This adds a PR details view to the GitHub pane, and allows checking out PR branches both from the same repository and from forks.

Known issues:

Navigation in the GitHub pane is broken - we know this but didn't want to make this PR any bigger than it already is
Some of the messages are not very useful - this will be addressed in a separate UI polish PR
No open file/diff file support yet - that will also come in a separate PR

cc @haacked to notify you of the known issues above

When PR clicked in PR list.

Added avatar and clickable PR# button that takes user to GitHub.

@donokuda

According to mockup by @donokuda. Data for the view is present in the designer view model but not yet in the real view model.

Open/Closed display not yet functional however, and the positioning of the "View more on GitHub" link is broken.

And provide a tooltip in case the source branch name doesn't fit in the pane.

And go straight to ApiClient. I don't know if we'll be caching this, but for now it makes things easier to hack on - we can re-add to the caching layer if needed later.

To display the files changed section. Also update sample data with change count.

WPF's ScrollViewer is broken, so hack around it. We already had this hack in GitHubConnectContent, so moved the hack to a central place that can be used wherever it's needed.

And fix bug in Account where avatar changes weren't getting notified.

It's ugly but it's there.

And removed a duplicate property.

I really want all caps for totally subjective aesthetical reasons, but it looks like we don't get that out of the box 😭 https://stackoverflow.com/questions/1762485/how-to-make-all-text-upper-case-capital

shana · 2016-10-17T12:10:07Z

src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs

+            // TODO: Catch errors.
+            Observable.CombineLatest(
+                    repositoryHost.ApiClient.GetPullRequest(repository.Owner, repository.CloneUrl.RepositoryName, prNumber),
+                    repositoryHost.ApiClient.GetPullRequestFiles(repository.Owner, repository.CloneUrl.RepositoryName, prNumber).ToList(),


The ModelService is the one that should be loading the data from ApiClient, optionally caching it, and then building a IPullRequestModel with all the information that we need to show on the view model.

Yes, I did originally start writing it using ModelService but then I remembered how we ended up essentially bypassing the caching for the PR list, so I went straight to the API. The question I wanted to discuss with you was do our reasons for bypassing the cache for the PR list apply here?

In the case of the PR list, we're not bypassing the cache, we're aggressively refreshing. If the network goes down or the connection is slow, it can still load data from the cache while it tries to refresh. But regardless of whether we bypass the cache completely or partially, the model service's job is to take data and create models from it, caching is just an intermediate step that it can also do. We should not be using Octokit types anywhere if we can avoid it, we already have enough types to mock :P

Ok, that makes sense. I did assume that I would need to use it in the end, but for development it was easier just to bypass and not have to maintain yet another class and interface. Will make it use the model service.

shana · 2016-10-17T12:25:37Z

src/GitHub.App/ViewModels/PullRequestFileViewModel.cs

+
+namespace GitHub.ViewModels
+{
+    public class PullRequestFileViewModel : IPullRequestFileViewModel


Shouldn't this be a model and not a viewmodel?

It's specifically tailored to the needs of the view, so that makes it a view model to me. In particular it wouldn't be a tree if it weren't for the fact that we can display as a tree in the view and later we might want to add things like a custom icon depending on file type etc, which makes it a VM in my mind.

I don't see anything that's specific to this view in particular. It's just a representation of a file change in a pull request. It has no logic, only data, usable by any view or viewmodel to do stuff. Screams of model to me 😄

Well to me the model is the model you get from the backend (PullRequestFile currently). This is transformed into a tree at the viewmodel layer for view purposes, making it a view model :)

shana · 2016-10-17T12:26:09Z

src/GitHub.App/ViewModels/PullRequestDirectoryViewModel.cs

+
+namespace GitHub.ViewModels
+{
+    public class PullRequestDirectoryViewModel : IPullRequestDirectoryViewModel


Shouldn't this be a model?

Actually, this is only used internally in the PullRequestDetailViewModel, so it probably shouldn't be exposed publicly, it's an implementation detail. We can just have this class in the PullRequestDetailViewModel as a helper class. I would definitely still not call it a ViewModel - the only thing it does is represent data, it has no logic for a view to interact with data, it's just a handy model (or in this case, a handy helper class)

shana · 2016-10-17T12:29:46Z

src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs

+            Number = pullRequest.Number;
+            Author = new Models.Account(pullRequest.User, avatarProvider.GetAvatar(new AccountCacheItem(pullRequest.User)));
+            CreatedAt = pullRequest.CreatedAt;
+            Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : "*No description provided.*";


Instead of copying all of this to the view model, how about using the IPullRequestModel which is already set up for most of this information (adding any missing bits) and exposing a PullRequest property that the view can get the information from?

Yes, I did consider this but then we'd lose out on change notifications, as models shouldn't be implementing INotifyPropertyChanged. I could however use the IPullRequestModel fields as the backing instead of having them as fields in the VM itself.

Not yet caching it yet though.

And bind to that where we don't need to do any transformation for the view.

- Hide Open/Diff menu for now - Use correct property for view switching menu item header - Display changed files count

Use Node rather than ViewModel.

haacked

Wow, this is really cool stuff. I found one serious problem with a NullReferenceException. Other than that, mostly cosmetic changes.

When you fix that problem, I'd like to take another look. Let me know if there's any specific areas you'd like me to pay close attention to when reviewing.

haacked · 2016-10-19T15:23:57Z

src/GitHub.App/Models/PullRequestModel.cs

-            get { return isOpen; }
-            set { isOpen = value; this.RaisePropertyChange(); }
+            get { return status; }
+            set { status = value; this.RaisePropertyChange(); this.RaisePropertyChange(nameof(IsOpen)); }


Maybe a comment here to explain that the second RaisePropertyChange event will go away when this is merged to master? Also, shouldn't this.RaisePropertyChange(nameof(Merged)) also be called here?

Good catch! Yes Merged should also be notified.

haacked · 2016-10-19T15:28:23Z

src/GitHub.App/Models/PullRequestModel.cs

        public DateTimeOffset CreatedAt { get; set; }
        public DateTimeOffset UpdatedAt { get; set; }
        public IAccount Author { get; set; }
+        public IList<IPullRequestFileModel> ChangedFiles { get; set; } = new IPullRequestFileModel[0];


In general, I'm wary of collection properties that can be read/write like this because it creates two ways of updating the collection, setting it and mutating it. It's probably a good idea to pick one approach or the other to avoid confusion. For example, you could make this an immutable IReadOnlyCollection<IPullRequestFileModel> and always set it when it changes.

Or you could make this a List<IPullRequestFileModel> (or some form of observable collection) and mutate the list when the collection changes (even if it means calling Clear and AddRange every time).

Yes, again a very good point - I always forget about ~~Dre~~ IReadOnlyCollection.

haacked · 2016-10-19T20:05:33Z

src/GitHub.App/Services/ModelService.cs

+                return new PullRequestCacheItem(pr, new PullRequestFile[0]);
+            }
+
+            public static PullRequestCacheItem Create(PullRequest pr, IList<PullRequestFile> files)


A nitpick here, but could this be IReadOnlyList<PullRequestFile>? I really like it when our parameters communicate more about what's needed and how they will be used. If you accept an IList I wonder if you're going to modify that list and whether I need to copy it first. Etc.

haacked · 2016-10-19T20:17:37Z

src/GitHub.App/Services/PullRequestService.cs

+            }
+        }
+
+        static string GetSafeBranchName(string name)


This implementation would replace / in a branch name, but that's totally valid. We have a good implementation of this in GitHub Desktop for Windows you can steal. I'll post the code here:

// See https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html // ASCII Control chars and space, DEL, ~ ^ : ? * [ \ are not valid. // | " < and > are technically valid in a refname as far as Git is concerned, but not valid on Windows // Also invalid: the magic sequence @{, consecutive dots, leading and trailing dot, ref ending in .lock static readonly Regex invalidCharsRegex = new Regex(@"[\x00-\x20\x7F~^:?*\[\\|""<>]|@{|\.\.+|^\.|\.$|\.lock$", RegexOptions.ECMAScript); /// <summary> /// Given a ref name, returns a safe version with invalid characters replaced with dashes. /// </summary> /// <param name="refName"></param> /// <returns></returns> public static string GetSafeReferenceName(string refName) { Ensure.ArgumentNotNull(refName, "refName"); var groups = refName.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries) .Select(x => invalidCharsRegex.Replace(x, "-")); return String.Join("/", groups).TrimStart('-'); }

The branch name that is created here is based on the PR title, so I think that carrying that through into the branch name wouldn't be desired. For example, if I create a PR called "Fix foo/bar errors" then it doesn't make much sense for the PR to be called pr/123-fix foo/bar errors especially as Visual Studio will display it in the "Manage Branches" window as:

pr

123-fix foo

bar errors

Ah, I see. Ok, carry on. Is it possible for a PR title to only contain invalid characters? Then what would be the branch name? Just a single -?

Hmm I guess someone could create a PR using only punctuation and no letters or digits, though I'm not sure how likely it would be! And yeah, in that case the branch name would be PR/123--.

Thinking about this a little over the weekend, I think the current implementation could cause problems with non-Latin character sets. So I guess that if the title has no Latin characters we should fall back to simply pr/123.

haacked · 2016-10-19T20:21:37Z

src/GitHub.App/Services/PullRequestService.cs

            return ret;
        }

+        void EnsurePullRefSpecExists(IRepository repo)


This method could be static.

haacked · 2016-10-19T20:32:59Z

src/GitHub.App/Services/PullRequestService.cs

+
+        public bool IsPullRequestFromFork(ILocalRepositoryModel repository, IPullRequestModel pullRequest)
+        {
+            return pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl() != repository.CloneUrl.ToRepositoryUrl();


So the Repository object returned by the API has a fork boolean you could test first. It's not on the IPullRequestModel instance, so you'd need to plum it through. But it'd ballow you to do this:

return pullRequest.IsFork && pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl() != repository.CloneUrl.ToRepositoryUrl();

That lets you short circuit the URL comparison if it's not a fork.

Ah, I didn't think of looking there. So if I check that do I still need to compare the clone URLs?

Ah, I didn't think of looking there. So if I check that do I still need to compare the clone URLs?

Yeah, I suppose you still do because it's possible that the PR target is the fork and not upstream.

haacked · 2016-10-19T20:34:21Z

src/GitHub.App/Services/PullRequestService.cs

+            return Observable.Defer(async () =>
+            {
+                var repo = gitService.GetRepository(repository.LocalPath);
+                var branchName = GetLocalBranchesInternal(repository, repo, pullRequest).First();


So we're absolutely sure GetLocalBranchesInternal will never return an empty collection? Maybe assert that in debug?

haacked · 2016-10-20T21:13:17Z

src/GitHub.App/Services/ModelService.cs

                Title = pr.Title;
                Number = pr.Number;
+                Base = new GitReferenceModel { Label = pr.Base.Label, Ref = pr.Base.Ref, RepositoryCloneUrl = pr.Base.Repository.CloneUrl };
+                Head = new GitReferenceModel { Label = pr.Head.Label, Ref = pr.Head.Ref, RepositoryCloneUrl = pr.Head.Repository.CloneUrl };


It's possible for the pr.Head.Repository property to be null causing a NullReferenceException here. For example, check out this PR: scientistproject/Scientist.net#20 After it was merged, the user deleted the repository.

Very good catch - I didn't think of that! Added code to deal with that case and a unit test. In addition, I've made GitReferenceModel immutable and made it check for non-null values in its fields as an additional sanity check.

haacked · 2016-10-20T21:16:58Z

src/GitHub.App/ViewModels/PullRequestFileNode.cs

+        }
+
+        public string FileName { get; }
+        public string Path { get; }


I tend to find property names like Path confusing. Path to what? How about DirectoryName or DirectoryPath? Bonus, that'll let you use Path.GetFileName rather than the fully qualified calls. 😄

Thing is that this is either a path to a file or a directory, as the property comes from the IPullRequestChangeNode which is implemented by PullRequestDirectoryNode as well as PullRequestFileNode. As such, I'm struggling to come up with a better name.

Ah! I didn't catch that. Carry on.

And added comment indicating that it will be 🔥 soon.

To try and minimize the chances of null exceptions.

If `PullRequestService.SwitchToBranch` is called with no local branch, handle it but assert.

When `CheckoutMode == Push` and has uncommitted changes.

grokys and others added 30 commits September 16, 2016 13:23

Display basic PR details.

1a17ec4

When PR clicked in PR list.

Allow PR details to be opened more than once.

b281ce0

Clicking PR number opens PR on GitHub.

e71726c

Add more stuff to PR details.

13df498

Added avatar and clickable PR# button that takes user to GitHub.

Merge branch 'master' into grokys/pr-detail

e82016a

Filled out PR details form XAML.

d48f422

According to mockup by @donokuda. Data for the view is present in the designer view model but not yet in the real view model.

Display more PR detail information.

c4ba3f0

Open/Closed display not yet functional however, and the positioning of the "View more on GitHub" link is broken.

Rejigged PR detail view layout.

2bbdd88

Implement PR Open/Closed label.

5d1d177

Shorten branch name if not from a fork.

8ac5503

And provide a tooltip in case the source branch name doesn't fit in the pane.

Added body right margin.

7669160

Fix separator overflow.

7ad0d4b

Skip ModelService for PR details.

524c3e2

And go straight to ApiClient. I don't know if we'll be caching this, but for now it makes things easier to hack on - we can re-add to the caching layer if needed later.

Started adding changes tree to PR details.

7d52f9c

Use TeamExplorer's SectionControl.

ff38e49

To display the files changed section. Also update sample data with change count.

Fix mouse wheel scrolling in PR details.

4efb5f6

WPF's ScrollViewer is broken, so hack around it. We already had this hack in GitHubConnectContent, so moved the hack to a central place that can be used wherever it's needed.

Merge branch 'master' into grokys/pr-detail

45060f6

Display spinner in PR details view.

9654986

Display avatar.

0b46eba

And fix bug in Account where avatar changes weren't getting notified.

Make "view on github" link work.

898b73f

Preparation for switchable changed files view.

25ead9e

Allow switching changes to list in PR details.

27807d4

It's ugly but it's there.

Make changes list look a bit nicer.

0fac3a0

Show options in dropdown menu in PR details.

2cd7f1c

Implement toggling file open action in PR detail.

3e137fe

Added some documentation.

04fa640

And removed a duplicate property.

Tweaks for dark theme.

6870a39

Adjust font size of PR title

d0d6f14

Update PR status text treatment

028b60d

I really want all caps for totally subjective aesthetical reasons, but it looks like we don't get that out of the box 😭 https://stackoverflow.com/questions/1762485/how-to-make-all-text-upper-case-capital

Vertical align center pr status

9908f33

grokys force-pushed the grokys/maintainer-workflow branch from 3358472 to f862a3d Compare October 14, 2016 15:07

shana reviewed Oct 17, 2016

View reviewed changes

Read pull request details view ModelService.

e5aecc9

Not yet caching it yet though.

grokys assigned haacked Oct 18, 2016

grokys added 7 commits October 18, 2016 19:34

Enable the cache for PRs in ModelService.

c996266

Expose the PR model from view model.

a397f25

And bind to that where we don't need to do any transformation for the view.

Tweaks to PR details view

c350de0

- Hide Open/Diff menu for now - Use correct property for view switching menu item header - Display changed files count

Renamed PR file/directory node classes.

6587ddb

Use Node rather than ViewModel.

Fix failing tests.

fa6f59c

Merge branch 'master' into grokys/maintainer-workflow

73dd7e5

Account for DPI scaling when positioning menu.

6fc5ff9

grokys added Ready for Review and removed WIP labels Oct 19, 2016

grokys changed the title ~~WIP: Maintainer workflow~~ Maintainer workflow Oct 19, 2016

haacked suggested changes Oct 20, 2016

View reviewed changes

grokys added 7 commits October 21, 2016 11:22

Raised missing notification of dependent value.

e22daa8

And added comment indicating that it will be 🔥 soon.

Use readonly collections where appropriate.

d28c340

Make method static.

47ee8e0

PullRequest.Head can be null.

4526845

Make GitReferenceModel immutable.

b890f32

To try and minimize the chances of null exceptions.

Added an assert and handle something going wrong.

059a8db

If `PullRequestService.SwitchToBranch` is called with no local branch, handle it but assert.

Fix assertion error.

4711503

When `CheckoutMode == Push` and has uncommitted changes.

haacked approved these changes Oct 24, 2016

View reviewed changes

Merge branch 'master' into grokys/maintainer-workflow

5ad4d53

haacked merged commit 865bc2b into master Oct 24, 2016

grokys mentioned this pull request Oct 24, 2016

Maintainer Workflow: double clicking changed file should open a diff #626

Closed

grokys deleted the grokys/maintainer-workflow branch November 29, 2016 08:36

Maintainer workflow #567

Maintainer workflow #567

Uh oh!

Conversation

grokys commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

haacked 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!

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!

Reviewers

Assignees

grokys commented Sep 28, 2016 •

edited

Loading