Skip to content

Conversation

@seanders
Copy link

@seanders seanders commented Mar 22, 2019

Description

This PR fixes #2843.

The source of the issue seems to have been the asynchronous setting of the component's state. Prior to this change, the search input's state was being updated by a debounced updateKeyword function. Pushing these updates to the event queue was causing keystrokes to be lost. The image below should make it more clear:

Before this code change:
image

After this code change:
image

GIFs

Here is a gif of me trying to type bacon as fast as possible

Before the fix.

Characters dropped bug

After the fix.

Characters dropped bug FIXED

Issue fixed

#2843

Type of changes

  • 🔘 Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • ⚪ Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

* The issue seemed to stem from the debounced state updates.
  Two keystrokes close together would cause the first one to be dropped.
  Say Keystroke A and Keystroke B happen in the same debounce interval.
  Only Keystroke B would be 'saved' to the state.
@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Mar 22, 2019
@ZeroX-DG ZeroX-DG requested a review from Rokt33r May 4, 2019 14:19
@AWolf81
Copy link
Contributor

AWolf81 commented May 8, 2019

The key dropping with fast typing seems already fixed on master. Can you please double check?

I think there is another issue on master. If you're backspace deleting the text it happens sometimes that the search field is losing focus and the markdown editor text area will be focused instead.

@e-volusian
Copy link

e-volusian commented May 22, 2019

Has this change been incorporated into the latest release (0.11.16) ? It appears not. Can somebody make it happen? This problem basically makes the search function useless for me.

The problem as I experience it is:

  1. Select search field
  2. Type something. In my screenshot below, you can see I typed sqlite
  3. The first part of the search term stays in the search field. The latter part of search term winds up in first line of first result and somehow out of order

2019-05-22_08-47-40


If nothing else, can the developers maybe consider adding an option to NOT auto-focus the first search result? At least then I could fix the dropped keys without having to switch to the mouse and reselect the search field.

@fmomin
Copy link

fmomin commented May 23, 2019

Has this change been incorporated into the latest release (0.11.16) ? It appears not. Can somebody make it happen? This problem basically makes the search function useless for me.

The problem as I experience it is:

  1. Select search field
  2. Type something. In my screenshot below, you can see I typed sqlite
  3. The first part of the search term stays in the search field. The latter part of search term winds up in first line of first result and somehow out of order
2019-05-22_08-47-40

If nothing else, can the developers maybe consider adding an option to NOT auto-focus the first search result? At least then I could fix the dropped keys without having to switch to the mouse and reselect the search field.

seanders already did the hard work :)
If you are looking to implement this yourself till this is published in new release then:

  1. Close boostnote if it is already running
  2. Goto "Boostnote\resources\app\compiled"
  3. Make a copy of main.js
  4. Search for "handleSearchChange" (For me it was line 71755 in version 0.11.15)
  5. Add the 3 lines he has added to make the section look like below
	  }, {
	    key: 'handleSearchChange',
	    value: function handleSearchChange(e) {
	      if (this.state.isAlphabet || this.state.isConfirmTranslation) {
					var keyword = this.refs.searchInput.value;
					
					this.setState({
						search: keyword
					})

	        this.updateKeyword(keyword);
	      } else {
	        e.preventDefault();
	      }
	    }
	  }, {
  1. Save the file and start Boostnote again
  2. Test the change with Developer Tools enabled to ensure it has not broken something else

@e-volusian
Copy link

@fmomin thanks for the guidance. confirming this fixes the problem for me

@seanders
Copy link
Author

seanders commented Jun 4, 2019

@Rokt33r @AWolf81 Fwiw, there are still reports of this issue: #2843 (comment)
Seems like its not yet fixed?

@Rokt33r
Copy link
Member

Rokt33r commented Jul 22, 2019

AFAIK, #3037 fixed this problem. Let me know if it is still broken in master branch. And the fix will be shipped in 2~3 days.

@Rokt33r Rokt33r closed this Jul 22, 2019
@seanders
Copy link
Author

seanders commented Aug 4, 2019

@Rokt33r Fair enough. I just wanted to leave a piece of feedback.

I offered up this PR 2 months before the fix in #3037. Personally, I can't help but to admit I find that frustrating. It feels like a lot of wasted time/effort.

This really disinclines me from wanting to make future contributions. I would imagine that this would frustrate future would-be contributors, too.

@Rokt33r
Copy link
Member

Rokt33r commented Aug 5, 2019

This really disinclines me from wanting to make future contributions. I would imagine that this would frustrate future would-be contributors, too.

Understood. The problem is I don't have enough time to review prs.. lots of people are using the app though. The good news is I've just started actively preparing the new Boostnote app from this month. But it would take 2~3 months to reveal the new thing from now. I hope you could find us providing better app around end of this October.... I'll do my best. Sorry for disappointing you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review ❇️ Pull request is awaiting a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Search box misses quickly typed characters

6 participants