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

Feature/multi search #16

Merged

Conversation

DasSkelett
Copy link

@DasSkelett DasSkelett commented Apr 20, 2021

Some fixes and stuff for KSP-CKAN#3323:

  • Changed the search control tab indices in a way that it should focus the first text box when hitting CTRL+F, and the following tab order: First textbox > Expand button > "add new mod search" button > second textbox > second expand button > third textbox...

  • Applying a search or filter (e.g. by changing filter from compatible to installed) does no longer force-show the "InstalledVersion", "InstallDate" and "AutoInstalled" columns if the user explicitly hid them. I hope I found all places where we need to do this.

  • Fixed a NullReferenceException in MatchesTags() when mod.Tags is null but tn/the tag name we're comparing with isn't.

  • Added and adjusted German localization where needed.

  • Adjusted other localizations to the best of my abilities, i.e. taking the translations from the Resources.MainFilter* strings and putting them into the new EditModSearchDetails.*Label strings.

  • Remove the empty EditModSearches.resx

  • Removed the Name property from SavedSearch, which I wasn't used anywhere, This in turn allowed to remove the FilterName() method and all the Resources.MainFilter* strings.

One question: the SavedSearch class has some attributes for XML serialization, however it is never serialized to XML. I suppose at one point you planned to use it in the GUIConfiguration, which is now just a simple List<string> (which SavedSearch is basically too). Do we still need these XML attributes, and what about moving it of GUIConfiguration since it doesn't really have anything to do with it?

@HebaruSan
Copy link
Owner

HebaruSan commented Apr 20, 2021

  • Removed the Name property from SavedSearch, which I wasn't used anywhere, This in turn allowed to remove the FilterName() method and all the Resources.MainFilter* strings.

One question: the SavedSearch class has some attributes for XML serialization, however it is never serialized to XML. I suppose at one point you planned to use it in the GUIConfiguration, which is now just a simple List<string> (which SavedSearch is basically too). Do we still need these XML attributes, and what about moving it of GUIConfiguration since it doesn't really have anything to do with it?

The idea behind SavedSearch was to allow the user to create a search using the UI currently in the PR, and then click a Save button somewhere, be prompted for a name, and then have that search added to the UI permanently (the filters dropdown would be the smallest change, but I was planning to replace that with a collapsible tree pane to the left listing various categories of searches). Then the whole list of saved searches would be saved to GUIConfig.xml and restored the next time the GUI ran. However, this would also require some way to let the user edit saved searches, rename them, delete them, etc., which requires much larger UI changes. So I scaled it back to what it is currently, with just the one active search being saved and restored.

I think I would like to return to that concept at some point in the future, at which point SavedSearch.Name and the XML attributes would have a purpose. Do you think it would be better to remove them now and add them back later, or leave them in without a function for a while?

@HebaruSan HebaruSan self-assigned this Apr 20, 2021
@DasSkelett
Copy link
Author

Ah, I see. Makes sense to keep the savedSearch.Name and XML attributes around.

The name would probably come from user input then instead of being generated from the active filter, I guess?
So we could still remove the Resources.MainFilter* strings and FilterName(), and replace it with some placeholder for now.
But if you say you would want to keep them as well, since we might use them as defaults if a user doesn't change it, and to not loose the translations, I'm happy with that, too.

@HebaruSan HebaruSan self-requested a review April 21, 2021 18:24
@HebaruSan
Copy link
Owner

But if you say you would want to keep them as well, since we might use them as defaults

I think I'd prefer that, yes. It will make it easier to fit them into a saved-search list in the future, should one be created.

Copy link
Owner

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, thanks for catching these.

@HebaruSan HebaruSan merged commit d39f8b1 into HebaruSan:feature/multi-search Apr 22, 2021
@DasSkelett DasSkelett deleted the feature/multi-search branch July 24, 2021 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants