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.

Sign up for GitHub

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

Jump to bottom

Reworking mkDebounce #996

Closed
wants to merge 8 commits into from
Closed

Reworking mkDebounce #996

wants to merge 8 commits into from

Conversation

Vlix
Copy link
Contributor

@Vlix Vlix commented Jun 8, 2024 •
edited
Loading

Ok, so I've tried to rework the debouncing, because of issue #980, but it seems to fail on a test because the trigger MVar doesn't seem to be correctly filled unless I put a trace in a specific spot!?

Does anyone know how to force/evaluate (or whatever is happening here) the tryPutMVar without using trace?

  • auto-update: reworked 'mkDebounce', and it should work... but 'Leading' only works if I put a 'trace' in a specific spot???

Still TODO:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@Vlix Vlix requested a review from kazu-yamamoto June 8, 2024 21:21
@Vlix
Copy link
Contributor Author

Vlix commented Jun 9, 2024 •
edited
Loading

Hmmm, seeing the random failing in CI jobs, this is probably a timing issue? And the trace "..." printing to the screen just takes enough time with the GHC version I was developing with? 🤔

Now to figure out if the test should be adjusted, or if there is actually an error in the implementation...

@Vlix
Copy link
Contributor Author

Vlix commented Oct 27, 2024

I figured it out. It was the trigger MVar that should be emptied BEFORE forking.

I think this fixes the leaking of threads and adds two styles of debouncing that are also pretty common.

@Vlix
Copy link
Contributor Author

Vlix commented Oct 27, 2024 •
edited
Loading

Windows nightly fails with:
doctest > C:/Users/runneradmin/AppData/Local/Programs/stack/x86_64-windows/ghc-9.8.3/mingw/bin/strip.exe: error: Permission denied

... who's fault would this be? 🤔

EDIT: Ah... rerunning fixes it. That's dumb 🤷

@Vlix Vlix force-pushed the trying-to-rework-Control-Debounce branch from ddb4081 to a4ec84d Compare October 27, 2024 13:08
@Vlix Vlix mentioned this pull request Oct 29, 2024
Open
@Vlix
Squashed commit of the following:
64e2e6c 
commit ddb4081
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 14:03:24 2024 +0100

    add PR number and URL to Changelog

commit ffecec5
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 14:02:58 2024 +0100

    remove custom build directory

commit 1eeceb4
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 13:06:49 2024 +0100

    and fix the CI

commit 27c6ff9
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 12:50:20 2024 +0100

    upped version in cabal file

commit 4246ff5
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 12:49:41 2024 +0100

    documentation fix

commit 16a20ca
Merge: b9e7aad 23b8c4c
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 12:42:07 2024 +0100

    Merge branch 'master' into trying-to-rework-Control-Debounce

commit b9e7aad
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 12:26:54 2024 +0100

    added to Changelog

commit 73deb77
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 12:26:39 2024 +0100

    auto-update/test: adjusted tests for new 'DebounceEdge' tests

commit f0ba37b
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sun Oct 27 12:26:08 2024 +0100

    auto-update: fixed debounce to not leak threads and also added two new 'DebounceEdge' types. Also improved (hopefully) the documentation

commit a0f9e92
Author: Felix Paulusma <felix.paulusma@gmail.com>
Date:   Sat Jun 8 23:17:01 2024 +0200

    auto-update: reworked 'mkDebounce', and it should work... but i'Leading' only works if I put a 'trace' in a specific spot???
@Vlix
auto-update/test: added timing check for 'TrailingDelay' test
df0c20e
@Vlix
auto-update: added documentation to new settings fields and bumped ve…
f368fc4 
…rsion to 0.2.3
@Vlix Vlix force-pushed the trying-to-rework-Control-Debounce branch from b5cedbe to f368fc4 Compare November 2, 2024 10:13
@Vlix
Copy link
Contributor Author

Vlix commented Nov 2, 2024 •
edited
Loading

Oh god, data-default breaks literally every CI... why is this even still a thing!?

https://github.com/yesodweb/wai/actions/runs/11641886813/job/32420880212?pr=996

@kazu-yamamoto
Copy link
Contributor

@Vlix Is this PR ready for reviewing?

@Vlix
Copy link
Contributor Author

Vlix commented Nov 3, 2024

@kazu-yamamoto Yes, I hope with this last commit (and quic building again) that CI is back to working condition.

@Vlix
'quic' no longer builds on GHC < 9.6, so removed 'warp-quic' from CI …
1e362d3 
…on those GHC versions, and no longer supporting GHC 8.10.7. I think it's time to drop it; we're at GHC 9.12 almost
@Vlix
Copy link
Contributor Author

Vlix commented Nov 3, 2024 •
edited
Loading

Apparently my new auto-update test is a bit flaky :/

@Vlix
seeing as 'threadDelay' doesn't guarantee immediate pickup, we should…
599ce8d 
…n't test that. Did extend the test to verify a trigger does at least restart the timer
@Vlix
Copy link
Contributor Author

Vlix commented Nov 5, 2024

When this is merged, I'll update the CI in #1011 to include quic again.

kazu-yamamoto
kazu-yamamoto reviewed Nov 6, 2024
View reviewed changes
auto-update/Control/Debounce.hs
DI.DebounceSettings,
defaultDebounceSettings,

-- * Accessors
-- ** Accessors
DI.debounceFreq,
DI.debounceAction,
DI.debounceEdge,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why DebounceEdge is NOT exported while deboundeEdge is exported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind.
leadingEdge should be used instead of Leading.

kazu-yamamoto
kazu-yamamoto approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

LGTM

kazu-yamamoto added a commit that referenced this pull request Nov 6, 2024
@kazu-yamamoto
Merge PR #996
69bd9a9
@kazu-yamamoto
Copy link
Contributor

Rebased and merged.
Nice work!

@Vlix Vlix deleted the trying-to-rework-Control-Debounce branch November 6, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kazu-yamamoto kazu-yamamoto kazu-yamamoto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Vlix @kazu-yamamoto