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

Master search bar and misc GUI clean-up #3041

Merged
merged 1 commit into from
May 15, 2020

Conversation

HebaruSan
Copy link
Member

Motivation

Currently we have three search boxes, one for name, one for author, and one for description. It's simultaneously cumbersome and limiting, in that it takes up a lot of horizontal space and yet there's no easy place to add more filters.

The Main form hosts the mod list grid view directly, which leads to spaghetti code in the absence of encapsulation. See #2966 for a previous round of clean-up for the other tabs.

There are some old workarounds and awkward things in GUI that don't need to be that way anymore.

Changes

  • Install errors are now handled in PostInstall instead of just before InstallMods exits
  • The MainModListGUI class is eliminated because it only existed to implement workarounds for very old versions of Mono
  • The Manage Mods tab is moved into its own file and UserControl so Main can focus on the outer/top level application logic
  • MainModList is renamed ModList because it has nothing to do with Main
    • And it is moved into the Model folder because it's a data object
  • Main.ModList is renamed ModGrid to distinguish it from the ModList class
  • The search fields are moved into their own file and UserControl
    • Fields for searching by localization and relationships are added
    • A combined master search textbox is added
    • By default the master box is visible and the others are hidden
    • An expand/collapse button is added to show/hide the fields other than the master box
    • The master box supports multiple search terms via string prefixes like GMail
      • Editing the master box updates the detailed boxes and vice versa
  • The search fields are now on the bottom of the grid instead of the top

This results in a simpler, yet more powerful, search UI.

image

image

Fixes #2736.
Fixes #2113.
Fixes #1494.

@HebaruSan HebaruSan added Enhancement New features or functionality GUI Issues affecting the interactive GUI Pull request Tests Issues affecting the internal tests labels Apr 29, 2020
@HebaruSan HebaruSan requested a review from DasSkelett April 29, 2020 02:30
@HebaruSan HebaruSan force-pushed the feature/master-search branch 2 times, most recently from 62c1984 to 6d897c7 Compare May 5, 2020 20:17
@DasSkelett
Copy link
Member

Looks like FailWaitDialog is not correctly called after download errors.
The Retry button is inactive, and the Cancel button has no effect:
download error

In this case it was an artificially induced WebException:

System.Net.WebException: Error: ConnectFailure (The requested address is not valid in this context)
 ---> System.Net.Sockets.SocketException: The requested address is not valid in this context
  at System.Net.Sockets.SocketAsyncResult.CheckIfThrowDelayedException () [0x00014] in <4e15bbae9d7043d8afd6cfd50bd9bd5a>:0 
  at System.Net.Sockets.Socket.EndConnect (System.IAsyncResult asyncResult) [0x0002c] in <4e15bbae9d7043d8afd6cfd50bd9bd5a>:0 
  at System.Net.WebConnection+<>c.<Connect>b__16_1 (System.IAsyncResult asyncResult) [0x00006] in <4e15bbae9d7043d8afd6cfd50bd9bd5a>:0 
  at System.Threading.Tasks.TaskFactory`1[TResult].FromAsyncCoreLogic (System.IAsyncResult iar, System.Func`2[T,TResult] endFunction, System.Action`1[T] endAction, System.Threading.Tasks.Task`1[TResult] promise, System.Boolean requiresSynchronization) [0x00019] in <a17fa1457c5d44f2885ac746c1764ea5>:0 

@HebaruSan
Copy link
Member Author

Good catch, I got the code flow structure wrong, the null case needs to happen unconditionally. Should be fixed in latest commit.

GUI/Main/MainLabels.cs Outdated Show resolved Hide resolved
GUI/Main/MainLabels.cs Outdated Show resolved Hide resolved
GUI/Main/MainLabels.cs Outdated Show resolved Hide resolved
@DasSkelett
Copy link
Member

Good catch, I got the code flow structure wrong, the null case needs to happen unconditionally. Should be fixed in latest commit.

The retry button is still inactive :/
I checked with my IDE what's happening there, and apparently there is an exception thrown in this line, cuasing PostInstallMods() to abort early:
https://github.com/HebaruSan/CKAN/blob/504b5ffce5012e6d97d46a3b92ddf3f68ed283db/GUI/Main/MainInstall.cs#L377
.NET seems to hide this exception though, I guess that's special handling for BackgroundWorker.OnWorkerCompleted()

The exception:

System.Reflection.TargetInvocationException: An exception occurred during the operation, making the result invalid.  Check InnerException for exception details. 
---> CKAN.ModuleDownloadErrorsKraken: Exception of type 'CKAN.ModuleDownloadErrorsKraken' was thrown.
  <Inner exception stack trace, not important here>
   --- End of inner exception stack trace ---
  at System.ComponentModel.AsyncCompletedEventArgs.RaiseExceptionIfNecessary () [0x0001d] in <4e15bbae9d7043d8afd6cfd50bd9bd5a>:0 
  at System.ComponentModel.RunWorkerCompletedEventArgs.get_Result () [0x00000] in <4e15bbae9d7043d8afd6cfd50bd9bd5a>:0 
  at CKAN.Main.PostInstallMods (System.Object sender, System.ComponentModel.RunWorkerCompletedEventArgs e) [0x0039e] in /home/kilian/git/CKAN/GUI/Main/MainInstall.cs:377 
  at System.ComponentModel.BackgroundWorker.OnRunWorkerCompleted (System.ComponentModel.RunWorkerCompletedEventArgs e) [0x0000a] in <4e15bbae9d7043d8afd6cfd50bd9bd5a>:0 
  at System.ComponentModel.BackgroundWorker.AsyncOperationCompleted (System.Object arg) [0x0000e] in <4e15bbae9d7043d8afd6cfd50bd9bd5a>:0 

So .NET denies accessing e.Result if an exception is thrown within the background worker, and throws an exception again if someone tries to do it.

I've got a fix for this that works, I'm going to submit it as a PR to your branch, I think that's easiest.

@HebaruSan
Copy link
Member Author

Yeah. Wow!

https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.runworkercompletedeventargs.result?view=netcore-3.1

Your RunWorkerCompleted event handler should always check the Error and Cancelled properties before accessing the Result property. If an exception was raised or if the operation was canceled, accessing the Result property raises an exception.

@HebaruSan
Copy link
Member Author

Rearranged PostInstallMods again now that I know e.Result can throw.

@HebaruSan
Copy link
Member Author

@cku-github, we're designing a search syntax and trying to decide whether to localize the prefix strings. GMail as a case study apparently does not:

gmail

I found a post about GMail's search syntax here by someone with the same first and last name as you, and I'm hoping this was you:

https://gmail.googleblog.com/2012/11/search-for-emails-by-size-and-more-in.html

If that's you, do you recall why GMail expects German users to search for "subject:blah" rather than "betreff:blah" or whatever? Did you decide that was a better user experience, or was it due to technical limitations or just a low priority? In our case it would be pretty easy to translate these strings, so we can go entirely by what would be a good UX. Thanks for any insight (and sorry for bothering you if you never had anything to do with GMail)!

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Thank you very very much for this feature, I love it! I've used the enhanced search a lot during the last two weeks, it's very handy!

@HebaruSan HebaruSan force-pushed the feature/master-search branch from 382b768 to d36da4d Compare May 15, 2020 17:08
@HebaruSan HebaruSan merged commit a742d5c into KSP-CKAN:master May 15, 2020
@HebaruSan HebaruSan deleted the feature/master-search branch May 15, 2020 17:10
@yalov
Copy link
Contributor

yalov commented May 15, 2020

Since the content are English (things, that user search in CKAN), so English prefix are more anticipated? It's even more true for the non-latin languages.

Generally, even if the content are multilingual, if you have strictly determined syntax, almost like html tags, user probably still expect English, but if you are going to Wolfram Alpha area, where search engine try to understand you, then localized prefix are expected.

@yalov
Copy link
Contributor

yalov commented May 15, 2020

though, I made localized prefixes in the KVASS, probably because vessels names often are written by players in their languages, but any language still was supporting also the English default prefixes

This was referenced Oct 11, 2020
@DasSkelett DasSkelett added the i18n Issues regarding the internationalization of CKAN and PRs that need translating label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New features or functionality GUI Issues affecting the interactive GUI i18n Issues regarding the internationalization of CKAN and PRs that need translating Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Master Search box [Feature] Mods Language Filter Search on Relationships
3 participants