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

Removed Debouncing mechanism #3612

Merged
merged 3 commits into from
Dec 21, 2023
Merged

Removed Debouncing mechanism #3612

merged 3 commits into from
Dec 21, 2023

Conversation

gouri-panda
Copy link
Collaborator

Fixes #3608

It removes debounce mechanics in order to have a better user experince.

Copy link

codecov bot commented Dec 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bdbfe2f) 48.77% compared to head (3eda149) 48.75%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3612      +/-   ##
============================================
- Coverage     48.77%   48.75%   -0.02%     
+ Complexity     1093     1091       -2     
============================================
  Files           285      285              
  Lines         10567    10556      -11     
  Branches       1413     1413              
============================================
- Hits           5154     5147       -7     
+ Misses         4573     4569       -4     
  Partials        840      840              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kelson42
Copy link
Collaborator

@gouri-panda We are sure this does not reintroduce new crash scenario? As well, I see you have removed the whole test, does that mean we have no test anymore for the suggestions?

@gouri-panda
Copy link
Collaborator Author

@kelson42 The test was specifically for the testing of debounce test to make sure we're not searching after a certain period of time.

@MohitMaliFtechiz
Copy link
Collaborator

MohitMaliFtechiz commented Dec 18, 2023

@gouri-panda I have tested it and it searches results a little faster, I have tried with both codes with slow/fast typing, and without debouncing it is 500ms faster to fetch the results.

@gouri-panda We are sure this does not reintroduce a new crash scenario.

@kelson42 We should make sure about this, However, on my device, there is no crash with this new code. @gouri-panda had faced a crashing issue on his device. @gouri-panda have you faced the crashing issue on your devices after this fix?

Apart from this, @gouri-panda if we are going to remove the debouncing from our search functionality, we should remove all the code related to it.

@kelson42
Copy link
Collaborator

@MohitMaliFtechiz please remove addional code if needed

@kelson42
Copy link
Collaborator

@MohitMaliFtechiz what’s up here? This is prio PR to handle.

@MohitMaliFtechiz
Copy link
Collaborator

MohitMaliFtechiz commented Dec 20, 2023

@kelson42 We have removed unnecessary debouncing code from the search functionality, and modified the test cases accordingly.

@kelson42 We should make sure about this, However, on my device, there is no crash with this new code. @gouri-panda had faced a crashing issue on his device. @gouri-panda have you faced the crashing issue on your devices after this fix?

@gouri-panda your feedback is important here.

@gouri-panda
Copy link
Collaborator Author

@MohitMaliFtechiz @kelson42 Sorry for late reply! Yes, the changed code looks good to me! @MohitMaliFtechiz I can't see the new crash scenario. But if we have a chance to test this on the low-end devices. then that would be perfect. Also, we should write various test case scenarios to ensure they pass what we intended.

@MohitMaliFtechiz
Copy link
Collaborator

MohitMaliFtechiz commented Dec 20, 2023

@MohitMaliFtechiz @kelson42 Sorry for late reply! Yes, the changed code looks good to me! @MohitMaliFtechiz I can't see the new crash scenario. But if we have a chance to test this on the low-end devices. then that would be perfect. Also, we should write various test case scenarios to ensure they pass what we intended.

@gouri-panda Thanks for confirming that you have not been able to reproduce the crash scenario, i have tested it on the lower API level emulators and it did not crash. Writing the test cases is a good idea, we have a ticket for this #3596.

@kelson42 We should implement #3596 in this PR, or we should do it separately. Apart from the test cases, the PR code LGTM.

@kelson42 kelson42 merged commit 2a0d98d into kiwix:main Dec 21, 2023
9 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.

Display search results as you type delay
3 participants