increase search query delay

[sertonix]

This increases the delayin wich a web request is send when searching for packages.

This should solve issues like this.

[confused-Techie]

This looks great to me! I'll still need to test before providing approval, but seems like it should get the job done

[DeeDeeG]

https://github.com/pulsar-edit/text-buffer/blob/9ef7defe5bb7dd06959c4116ee6b9cd553eeaef5/src/helpers.js#L9-L26

(^ debounce() function definition in text-buffer package)

Wow, this looks crazy. (Edit: I mean the text-buffer debounce() code, not necessarily this PR, although honestly probably both.)

Well, if this works (hopefully I might have time to test?) Then I don't see why not.

[DeeDeeG]

It works! Thanks for implementing this.

[DeeDeeG]

I'd personally like if we would write out this debounce() function directly in the file, rather than importing, and perhaps use a simpler debounce function?

This https://www.freecodecamp.org/news/javascript-debounce-example/#debounce-in-javascript is working for me, and it's only 7 lines of code instead of 16, and easier to read.

And also, it makes it so you can develop and test settings-view in its own dir with ppm link, as an alternative to having to either re-build/re-package Pulsar, or build the whole repo and run it from source.

cc @pulsar-edit/core, does anyone agree with this suggestion, or should we just merge it as-is?

[sertonix]

I now included the debounce function.
I made the changes in the browser so the should still be tested.

[DeeDeeG]

I tested this, and I think 1.5 seconds (1500ms) is rather long. (500ms is almost long enough, but I found it can still be too short and cause another search to happen on each character if you type rather slowly.) So I suggest 660 ms? This length of timeout (660 ms) never timed out on me in the middle of typing stuff.

(I would be willing to do this in a follow-up PR if there is no consensus on this, but this is my suggestion, based on testing and my personal opinion.)

[sertonix]

I now changed the delay to 700ms.
It would be nice if somebody implements that the search also happens if you press enter.
I made it more compact so that it only takes 3 additional lines

[DeeDeeG]

Thanks, this looks better already, IMO.

[DeeDeeG]

Just a quick summary in case that was a lot to read:

Why not do this to simplify (easier to read the code this way)?

diff --git a/lib/install-panel.js b/lib/install-panel.js
index 87fb739..ba47e75 100644
--- a/lib/install-panel.js
+++ b/lib/install-panel.js
@@ -20,6 +20,7 @@ export default class InstallPanel {
     this.disposables = new CompositeDisposable()
     this.client = this.packageManager.getClient()
     this.atomIoURL = 'https://web.pulsar-edit.dev/'
+    this.timer = 0
 
     etch.initialize(this)
 
@@ -45,10 +46,10 @@ export default class InstallPanel {
       })
     )
     const searchBuffer = this.refs.searchEditor.getBuffer();
-    searchBuffer.debouncedEmitDidStopChangingEvent = (timer => () => {
-      clearTimeout(timer);
-      timer = setTimeout(searchBuffer.emitDidStopChangingEvent.bind(searchBuffer), 700);
-    })();
+    searchBuffer.debouncedEmitDidStopChangingEvent = () => {
+      clearTimeout(this.timer);
+      this.timer = setTimeout(searchBuffer.emitDidStopChangingEvent.bind(searchBuffer), 7000);
+    };
     // TODO remove hack to extend stop changing delay
     this.disposables.add(
       this.refs.searchEditor.onDidStopChanging(() => {

I am willing to merge the PR with or without this suggested change, wanted a second opinion before doing it that way. Thanks for everything. I think this would be the last feedback I have, and I am willing to merge once this suggestion is looked at -- or if concensus is to not adopt this suggestion, or if there is no consensus on adopting this suggestion after some time, then I would be comfortable merging as-is.

@sertonix if you agree with this suggestion, I will happily push it to this branch for you if you want, just ask and I will do it. And if you think there is a reason not do do this suggestion, please let me know, I am open to hearing it. And either way, thanks for this PR.

[sertonix]

This is a bad fix and requires a good api for a good solution.
I want to make sure that no code could use any of this to prevent any potenital breaks if this is removed again.
I know that this causes code that is not easy to read but exposing more hacky apis would be worse.

[DeeDeeG]

Yeah, I can agree with that.

If some package somehow relies on timer existing (despite only being there for the sake of this hack), it would be messier having to break a community package than to have a slightly harder to read temporary hack.

I assume this hack is more or less optimal, so we may not replace it quickly. And doing anything with the timer variable should be unsupported by us, IMO. I can't see what it would be good for, since we overwrite it super frequently ourselves. But it still changes the properties of the class, so that's probably not worth it.

Okay, so I accept why the arrow functions are a little funky. It is a hack after all. But a good one for now.

Thanks again, I will approve and merge.

[DeeDeeG]

Approving because: Looks good, all feedback was addressed, and this PR is helpful to limit traffic on our backend API server.

Also: Approving on the basis of my testing that it works, plus @confused-Techie's positive emoji reactions add to an at least light degree of apparent consensus.

LGTM. Gonna go ahead and merge now. Thanks again @sertonix!