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

Pm searchbar improvements #14553

Merged
merged 14 commits into from
Nov 10, 2023
Merged

Conversation

dnenov
Copy link
Collaborator

@dnenov dnenov commented Nov 1, 2023

Purpose

PR addressing outstanding issues with the Search of the Package Manager.

  • search should no-longer be able during loading screen
  • now only performs RefreshAndSearchAsync() once when loading the ViewModel for the first time. This makes subsequent views loads instantaneous
  • removed double binding through test_propertyChanged event and direct binding to property; now only triggers the text_propertyChanged
  • added value difference check for the SearchText property to stop the possibility of reapplying the same text over and over
  • simplified the DispatcherTimer code
  • now waits for 100ms after each keystroke before sending a search query
  • this may decrease the 'live' feel of the search, but will prevent starting multiple searches in quick succession

This PR cherry-picks and updates on some of the issues in #14553. The old PR will be split into separate PRs for clarity.

Loading search results

search box improve

Removing RequestShowFileDialog -= OnRequestShowFileDialog;

Removing nonHostFilter.ForEach(f => f.PropertyChanged -= filter_PropertyChanged);

The nonHostFilter are only initialized once and are part of the ViewModel. Since we are not distroying the ViewModel manually, we should not be removing these handlers when the View closes, we want them to remain.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

  • disables the search bar functionality while the results are being loaded
  • removed the double-binding through textchanged AND property binding
  • simplified code for timed trigger of search via the DispatcherTimer
  • added value difference check to stop reapplying the same one over and over
  • removed unnecessary un-subscriptions

Reviewers

@mjkkirschner
@reddyashish

FYIs

@Amoursol

- disables the search bar functionality while the results are being loaded
- removed the double-binding through textchanged AND property binding
- simplified code for timed trigger of search via the DispatcherTimer
- added value difference check to stop reapplying the same one over and over
- removed unnecessary un-subscriptions (to explain in PR)
- now only does the initial (slow) search when loading the model for the first time
- since we are not destroying the model when closing the window, we should not have to refreshandsearch again and again. We are using the cached results we have loaded the first time
- timed-out timer fix
- removed reset events on windows close as no longer needed
@dnenov dnenov marked this pull request as ready for review November 1, 2023 22:24
@reddyashish reddyashish added this to the 3.0 milestone Nov 1, 2023
@dnenov dnenov requested a review from reddyashish November 2, 2023 13:09
@dnenov dnenov mentioned this pull request Nov 2, 2023
9 tasks
- RefreshAndSearchAsync will be repeated every time you open the PM window
- replaced delay timer with wpf native delay property (thank you!)
- removed old code
{
TimedOut = false; // reset the timedout screen
InitialResultsLoaded = false; // reset the loading screen settings
RequestShowFileDialog -= OnRequestShowFileDialog;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still unsubscribed somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, no. This line never made sense in a way, because the RequestShowFileDialog is null here.

We are subscribing each individual PackageManagerSearchElementViewModel to OnRequestShowFileDialog here

. Speaking with @reddyashish these should automatically unsubscribe when the collection is destroyed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@QilongTang OnRequestShowFileDialog is subscribed to the elements in the packages list which should be disposed when the window is closed. So want to confirm if we still need to unsubscribe it.

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM with a few questions

{
TimedOut = false; // reset the timedout screen
InitialResultsLoaded = false; // reset the loading screen settings
RequestShowFileDialog -= OnRequestShowFileDialog;
Copy link
Contributor

Choose a reason for hiding this comment

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

@QilongTang OnRequestShowFileDialog is subscribed to the elements in the packages list which should be disposed when the window is closed. So want to confirm if we still need to unsubscribe it.

@reddyashish reddyashish merged commit ce4c9b3 into DynamoDS:master Nov 10, 2023
17 checks passed
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.

6 participants