Pull Request Filtering #732

StanleyGoldman · 2016-12-12T15:53:29Z

As requested in #722, functionality to filter pull requests by title or number.

StanleyGoldman · 2016-12-12T17:14:10Z

src/GitHub.Exports.Reactive/ViewModels/IPullRequestListViewModel.cs

        ObservableCollection<IAccount> Assignees { get; }
        IAccount SelectedAssignee { get; set; }
+        string FilterText { get; set; }
+        bool IsLoaded { get; }


I get the feeling IsLoaded should be here.

shana · 2016-12-12T17:36:38Z

This is pretty neat! 😄

StanleyGoldman · 2016-12-12T22:35:37Z

It looks good. Just needs some 🏋️ from @donokuda

grokys · 2016-12-13T14:18:43Z

This looks great! A few things:

While the list is loading, the filter text box is enabled but typing into it does nothing. We probably need to fix this to make filtering etc work too while loading (all the PRs for e.g. aspnet/Mvc take a while to load) but for the moment we should probably disable the textbox while loading like we do with the sort dropdowns
I tried to search for a PR by number by typing "Switch submodule links to https #3" but PR 3 was not found - I'd expect that I'd search for PRs by number by using the "#" prefix
Should probably put a delay on the search text box binding, as currently typing lags when a project has a lot of PRs (again as in aspnet/Mvc)

StanleyGoldman · 2016-12-13T19:58:39Z

Ah, testing against a repo with a lot of pull requests is something I need to start doing then.

StanleyGoldman · 2016-12-13T21:13:17Z

Updated the gif above.

Let me know if you find the "#" functionality odd..
It wont assume you are performing a pull request number search until and while you follow the "#" with digits. So the initial "#" starts to search for pull requests with "#" in the title.

grokys · 2016-12-14T09:55:01Z

src/GitHub.VisualStudio/UI/Views/PullRequestListView.xaml


    <ui:FilterTextBox x:Name="filterText"
-            IsEnabled="{Binding IsLoaded}"
+            IsEnabled="{Binding IsLoaded, Delay=500}"


This delay should be on the Text property, not IsEnabled ;)

grokys · 2016-12-14T09:58:08Z

No, I think the "#" works as you've implemented it. I just wonder whether it should be a full string search on the PR number instead of a substring search. For example in aspnet/Mvc if I search "#2" I'm probably looking for PR #2, but instead I get all of the PRs which start with the number 2 (which is a lot in this case!)

StanleyGoldman · 2016-12-14T13:10:05Z

Fixed up

StanleyGoldman · 2016-12-14T13:20:21Z

Yea, I tried it with exact number matching, I can push a branch if you wanna try it out.
It just felt kind of... empty... until you got to the number you were typing, lol.

grokys · 2016-12-14T13:48:50Z

Hmm yeah, I see what you're saying. One thing I tried (to see if it'd work) was adding a space after the number to see if that forced exact matching - I'm not sure how many people would think to try that though?

tocsoft · 2016-12-14T14:51:05Z

could you not use both searching methods?
order exact matches first then partial matches below?

grokys · 2016-12-14T15:41:42Z

That's a really good idea @tocsoft !

StanleyGoldman · 2016-12-14T16:22:14Z

I think that's actually a sort not a filter. We would have to land the sorting pull request to see how to integrate the logic.

I'm willing to check it out, but I'm afraid a user wouldn't be able to ascertain what it's doing.

donokuda · 2016-12-15T17:05:22Z

I'm assuming that if someone is searching for a PR by its number, then they would know exactly which number they're looking for. If that's the case, then I'm not convinced of the value spent trying to refine filtering for PR numbers beyond how it currently works.

I also can't remember the last time when I went "Oh, I need to search for PR #732." Rather, I'm probably going to be searching for keywords like "pull request filtering" instead. That said, I would love to hear your experiences when searching for a specific pull request number is more useful and quicker (perhaps in tandem with an issue tracker 😄)

However, if matching worked on something like branch names (which could be considered out of scope for this pull request), then searching for a number would be more useful when I know a branch is fixing a specific issue (fixes/479-clear-pr-creation-form).

grokys · 2016-12-15T18:09:45Z

I also can't remember the last time when I went "Oh, I need to search for PR #732." Rather, I'm probably going to be searching for keywords like "pull request filtering" instead.

I do this all the time: for example if someone mentions a PR by number in slack. In addition if I'm looking at a PR on github.com and want to find that PR in Visual Studio I'll likely want to search by number,

StanleyGoldman · 2016-12-15T20:52:26Z

So I think we are agreeing?
Summary: Partial number matching of any kind is misleading and exact number matching is best.

grokys · 2016-12-16T10:02:03Z

Yeah, I've just tried out the PR with the exact matching, and while I can see your problem with it, I think the behavior is more useful. So personally I think exact number matching is best, though I can't speak for anyone else.

…ience # Conflicts: # src/GitHub.App/ViewModels/PullRequestListViewModel.cs

…t as a pull request number filter

StanleyGoldman · 2016-12-16T13:29:19Z

Agreed. Exact matching is best.

shana · 2016-12-16T14:43:40Z

Let's go with that and see how it goes 😄

jcansdale · 2017-02-28T15:47:33Z

This seems to work well. 😄

For consistency, it would be good to use the magnifying glass icon:

It looks like standard icons can be accessed via IVsImageService2. See:
https://github.com/madskristensen/ExtensibilityTools/blob/master/src/Misc/Commands/ImageMonikerDialog.xaml.cs

Conflicts: src/GitHub.App/SampleData/PullRequestListViewModelDesigner.cs src/GitHub.App/ViewModels/PullRequestListViewModel.cs src/GitHub.Exports.Reactive/ViewModels/IPullRequestListViewModel.cs

Conflicts: src/GitHub.App/ViewModels/PullRequestListViewModel.cs

grokys · 2017-11-08T15:40:02Z

@jcansdale yes that would be nice, to properly fit in with other VS search boxes I think we need a completely different control (as the one we're using is styled for our dialog which uses a different color scheme etc). This also has the downside that the search box is white on a dark theme. I think that would be another PR tbh...

Instead of displaying our own search box in the PR list, use the built-in search box provided by `ToolWindowPane`.

grokys · 2017-11-10T15:20:36Z

I've updated the PR to use the built-in VS search functionality, which works around the styling issues. Now looks like this:

grokys · 2017-11-13T08:15:31Z

Moved to #1312.

StanleyGoldman commented Dec 12, 2016

View reviewed changes

grokys reviewed Dec 14, 2016

View reviewed changes

grokys added this to the Maintainer Workflow: Beta milestone Dec 14, 2016

StanleyGoldman mentioned this pull request Dec 14, 2016

Pull Request Filtering w/ Exact PR Number Match #740

Closed

StanleyGoldman added 8 commits December 16, 2016 08:21

Initial filtering functionality

b67cbb7

Treating the pull request id as a string leads to a better user exper…

123e392

…ience # Conflicts: # src/GitHub.App/ViewModels/PullRequestListViewModel.cs

Using FilterTextBox

0bc343b

Disabling filterText while !IsLoaded

040a7b5

Adding delay to the binding

d69e8eb

When Filter Text starts with '#' followed by digits, treat filter tex…

b633251

…t as a pull request number filter

Putting the binding delay attribute in the right place

6f1a602

A modification of pull request filtering with exact PR number matching

aaf95ee

grokys removed this from the Maintainer Workflow: Beta milestone Jan 4, 2017

grokys changed the base branch from master to release/2.3 February 13, 2017 14:56

Merge branch 'release/2.3' into enhancement/pull-request-filtering

290c5ae

grokys added 2 commits August 29, 2017 11:22

Merge branch 'master' into pr/732-pull-request-filtering

956ae11

Conflicts: src/GitHub.App/SampleData/PullRequestListViewModelDesigner.cs src/GitHub.App/ViewModels/PullRequestListViewModel.cs src/GitHub.Exports.Reactive/ViewModels/IPullRequestListViewModel.cs

Moved filter textbox above dropdowns.

50b9207

grokys changed the base branch from release/2.3 to master September 20, 2017 22:29

Merge branch 'master' into pr/732-pull-request-filtering

4d5a72b

Conflicts: src/GitHub.App/ViewModels/PullRequestListViewModel.cs

grokys added 3 commits November 10, 2017 14:38

Use built-in VS search box.

d201359

Instead of displaying our own search box in the PR list, use the built-in search box provided by `ToolWindowPane`.

Use stretch placement for search box.

30cc14d

Merge branch 'master' into pr/732-pull-request-filtering

4d93b2a

grokys and others added 3 commits November 10, 2017 16:36

FIx/suppress CA errors.

2e2bfdd

Merge branch 'master' into enhancement/pull-request-filtering

8cb0f59

Merge branch 'master' into enhancement/pull-request-filtering

985b18f

grokys mentioned this pull request Nov 13, 2017

Pull Request Filtering #1312

Merged

grokys closed this Nov 13, 2017

Pull Request Filtering #732

Pull Request Filtering #732

Uh oh!

Conversation

StanleyGoldman commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StanleyGoldman Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

shana commented Dec 12, 2016

Uh oh!

StanleyGoldman commented Dec 12, 2016

Uh oh!

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

Uh oh!

Uh oh!

StanleyGoldman commented Dec 13, 2016

Uh oh!

StanleyGoldman commented Dec 13, 2016

Uh oh!

grokys Dec 14, 2016

Choose a reason for hiding this comment

Uh oh!

StanleyGoldman Dec 14, 2016

Choose a reason for hiding this comment

Uh oh!

grokys commented Dec 14, 2016

Uh oh!

StanleyGoldman commented Dec 14, 2016

Uh oh!

StanleyGoldman commented Dec 14, 2016

Uh oh!

grokys commented Dec 14, 2016

Uh oh!

tocsoft commented Dec 14, 2016

Uh oh!

grokys commented Dec 14, 2016

Uh oh!

StanleyGoldman commented Dec 14, 2016

Uh oh!

donokuda commented Dec 15, 2016

Uh oh!

grokys commented Dec 15, 2016

Uh oh!

StanleyGoldman commented Dec 15, 2016

Uh oh!

grokys commented Dec 16, 2016

Uh oh!

StanleyGoldman commented Dec 16, 2016

Uh oh!

shana commented Dec 16, 2016

Uh oh!

jcansdale commented Feb 28, 2017

Uh oh!

grokys commented Nov 8, 2017

Uh oh!

grokys commented Nov 10, 2017

Uh oh!

grokys commented Nov 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

StanleyGoldman commented Dec 12, 2016 •

edited

Loading

grokys commented Dec 13, 2016 •

edited

Loading