Skip to content
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

Add Assembly column to search results #1690

Merged
merged 3 commits into from
Sep 20, 2019

Conversation

miloush
Copy link
Contributor

@miloush miloush commented Sep 5, 2019

Implements #1544

image

@sailro
Copy link
Contributor

sailro commented Sep 12, 2019

In the case #1694 is merged as well, what do you think we should do regarding this new column? When searching for resources, the "location" is the assembly.

We can keep Location for the shared assembly name (without version, etc.) and have this new column for the full assembly name. Would that be ok ?

@miloush
Copy link
Contributor Author

miloush commented Sep 13, 2019

Right, well the way I implemented resource search it looks for files inside .resources files too which then goes into the Location column:

image

It's trickier with assembly search, for which I just put the path in the Location column:

image

Any of that sounds reasonable?

@sailro
Copy link
Contributor

sailro commented Sep 13, 2019

Ah the funny thing is that we implemented both the same "search for resources" feature two days apart:

Yours: miloush@8f89457
Mine: https://github.com/sailro/ILSpy/commit/ee839b571348854757ea54f756bd00246e66dcac

@miloush
Copy link
Contributor Author

miloush commented Sep 13, 2019

Indeed. Interesting that you decided to split the search results and switch on their type while I decided to split the search strategy and take advantage of the switch in JumpToReference since nothing was actually reading the SearchResult.Entity strongly-typed. It made assembly search possible by just adding one extra strategy.

I don't mind what you go for, I just have almost 6000 assemblies loaded in the list and needed to search for a baml component mentioned in a source code, hence searching inside .resources files was a big help for me.

@siegfriedpammer
Copy link
Member

So... which one should I merge first? :)

@miloush
Copy link
Contributor Author

miloush commented Sep 20, 2019

As far as this PR goes I believe we agreed with @sailro that the Assembly column shows full assembly name, and hence this can be merged irrelevant of the other discussions.

Since resource search strategy either splits the strategy or the result, it might be easier to merge this one first as it precedes such changes in both.

@siegfriedpammer
Copy link
Member

@miloush are we going to see a PR for miloush@2fa0262 as well?

@miloush
Copy link
Contributor Author

miloush commented Sep 20, 2019

Sure happy to create one but the implementation will differ depending on the result vs. strategy split. Is there a preference which way to go?

@siegfriedpammer siegfriedpammer merged commit 1b1d779 into icsharpcode:master Sep 20, 2019
@siegfriedpammer
Copy link
Member

Thank you for the contribution!

I think we can continue the discussion about these features on the other pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants