The Wayback Machine - https://web.archive.org/web/20201215054520/https://github.com/github/VisualStudio/pull/732
Skip to content
Sign up
Sign in Sign up

/ VisualStudio

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

Jump to bottom

Pull Request Filtering #732

Closed
wants to merge 18 commits into from
Closed

Pull Request Filtering #732

wants to merge 18 commits into from

Conversation

@StanleyGoldman
Copy link
Contributor

@StanleyGoldman StanleyGoldman commented Dec 12, 2016 •
edited

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

Pull Request Filtering
@StanleyGoldman
StanleyGoldman reviewed Dec 12, 2016
View changes
src/GitHub.Exports.Reactive/ViewModels/IPullRequestListViewModel.cs Outdated
@@ -38,5 +38,7 @@ public interface IPullRequestListViewModel : IViewModel
IAccount SelectedAuthor { get; set; }
ObservableCollection<IAccount> Assignees { get; }
IAccount SelectedAssignee { get; set; }
string FilterText { get; set; }
bool IsLoaded { get; }

This comment has been minimized.

Sign in to view
@StanleyGoldman

StanleyGoldman Dec 12, 2016
Author Contributor

I get the feeling IsLoaded should be here.
@shana
Copy link
Collaborator

@shana shana commented Dec 12, 2016

This is pretty neat! 😄
@StanleyGoldman
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 12, 2016

It looks good. Just needs some 🏋️ from @donokuda
@grokys
Copy link
Contributor

@grokys grokys commented Dec 13, 2016 •
edited

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 "#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
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 13, 2016

Ah, testing against a repo with a lot of pull requests is something I need to start doing then.
@StanleyGoldman
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 13, 2016

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
grokys reviewed Dec 14, 2016
View changes
src/GitHub.VisualStudio/UI/Views/PullRequestListView.xaml Outdated
@@ -141,7 +141,7 @@
</Grid>

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

This comment has been minimized.

Sign in to view
@grokys

grokys Dec 14, 2016
Contributor

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

This comment has been minimized.

Sign in to view
@StanleyGoldman

StanleyGoldman Dec 14, 2016
Author Contributor

😨
@grokys
Copy link
Contributor

@grokys grokys commented Dec 14, 2016

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
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 14, 2016

Fixed up
@StanleyGoldman
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 14, 2016

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
Copy link
Contributor

@grokys grokys commented Dec 14, 2016

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?
@grokys grokys added this to the Maintainer Workflow: Beta milestone Dec 14, 2016
@StanleyGoldman StanleyGoldman mentioned this pull request Dec 14, 2016
Closed
@tocsoft
Copy link
Contributor

@tocsoft tocsoft commented Dec 14, 2016

could you not use both searching methods?
order exact matches first then partial matches below?
@grokys
Copy link
Contributor

@grokys grokys commented Dec 14, 2016

That's a really good idea @tocsoft !
@StanleyGoldman
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 14, 2016

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
Copy link
Member

@donokuda donokuda commented Dec 15, 2016

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
Copy link
Contributor

@grokys grokys commented Dec 15, 2016

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
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 15, 2016

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

@grokys grokys commented Dec 16, 2016

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.
@StanleyGoldman
Initial filtering functionality
b67cbb7
@StanleyGoldman
Treating the pull request id as a string leads to a better user exper…
123e392 
…ience

# Conflicts:
#	src/GitHub.App/ViewModels/PullRequestListViewModel.cs
@StanleyGoldman
Using FilterTextBox
0bc343b
@StanleyGoldman
Disabling filterText while !IsLoaded
040a7b5
@StanleyGoldman
Adding delay to the binding
d69e8eb
@StanleyGoldman
When Filter Text starts with '#' followed by digits, treat filter tex…
b633251 
…t as a pull request number filter
@StanleyGoldman
Putting the binding delay attribute in the right place
6f1a602
@StanleyGoldman
A modification of pull request filtering with exact PR number matching
Loading status checks…
aaf95ee
@StanleyGoldman
Copy link
Contributor Author

@StanleyGoldman StanleyGoldman commented Dec 16, 2016

Agreed. Exact matching is best.
@shana
Copy link
Collaborator

@shana shana commented Dec 16, 2016

Let's go with that and see how it goes 😄
@grokys grokys removed this from the Maintainer Workflow: Beta milestone Jan 4, 2017
@grokys grokys changed the base branch from master to release/2.3 Feb 13, 2017
@grokys
Merge branch 'release/2.3' into enhancement/pull-request-filtering
Loading status checks…
290c5ae
@grokys grokys added this to Not Started in 2.3.x Feb 28, 2017
@jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented Feb 28, 2017

This seems to work well. 😄

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

image

It looks like standard icons can be accessed via IVsImageService2. See:
https://github.com/madskristensen/ExtensibilityTools/blob/master/src/Misc/Commands/ImageMonikerDialog.xaml.cs
@grokys grokys moved this from Not Started to In Progress in 2.3.x Jun 29, 2017
grokys added 2 commits Aug 29, 2017
@grokys
Merge branch 'master' into pr/732-pull-request-filtering
Loading status checks…
956ae11 
 Conflicts:
	src/GitHub.App/SampleData/PullRequestListViewModelDesigner.cs
	src/GitHub.App/ViewModels/PullRequestListViewModel.cs
	src/GitHub.Exports.Reactive/ViewModels/IPullRequestListViewModel.cs
@grokys
Moved filter textbox above dropdowns.
Loading status checks…
50b9207
@grokys grokys changed the base branch from release/2.3 to master Sep 20, 2017
@grokys
Merge branch 'master' into pr/732-pull-request-filtering
Loading status checks…
4d5a72b 
 Conflicts:
	src/GitHub.App/ViewModels/PullRequestListViewModel.cs
@grokys
Copy link
Contributor

@grokys grokys commented Nov 8, 2017

@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...
grokys added 3 commits Nov 10, 2017
@grokys
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`.
@grokys
Use stretch placement for search box.
Loading status checks…
30cc14d
@grokys
Merge branch 'master' into pr/732-pull-request-filtering
Loading status checks…
4d93b2a
@grokys
Copy link
Contributor

@grokys grokys commented Nov 10, 2017

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

image
grokys added 3 commits Nov 10, 2017
@grokys
FIx/suppress CA errors.
Loading status checks…
2e2bfdd
@grokys
Merge branch 'master' into enhancement/pull-request-filtering
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8cb0f59
@grokys
Merge branch 'master' into enhancement/pull-request-filtering
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
985b18f
@grokys grokys mentioned this pull request Nov 13, 2017
Merged
@grokys
Copy link
Contributor

@grokys grokys commented Nov 13, 2017

Moved to #1312.
@grokys grokys closed this Nov 13, 2017
@grokys grokys moved this from In Progress to Done in 2.3.x Jan 16, 2018
@grokys grokys moved this from Done to Punted in 2.3.x Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grokys grokys

Assignees
No one assigned
Labels
None yet
Projects
2.3.x
Punted
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@StanleyGoldman @shana @grokys @tocsoft @donokuda @jcansdale
You can’t perform that action at this time.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.