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

Infinite loop when running (circadian-activate-current) #33

Closed
LemonBreezes opened this issue Apr 26, 2024 · 12 comments · Fixed by #35
Closed

Infinite loop when running (circadian-activate-current) #33

LemonBreezes opened this issue Apr 26, 2024 · 12 comments · Fixed by #35
Assignees
Labels
Milestone

Comments

@LemonBreezes
Copy link
Contributor

LemonBreezes commented Apr 26, 2024

Hello. I am on the latest version of Emacs 30 and running the following code from emacs -Q:

(defvar bootstrap-version)

(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
                                    'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))

(straight-use-package 'circadian)

(require 'calendar)
(setq calendar-latitude 28
      calendar-longitude -96)

(require 'circadian)

(setq circadian-themes
      '((:sunrise . modus-operandi-tinted)
        (:sunset  . modus-vivendi-tinted)))

(circadian-activate-current)

causes the package to reload the current theme in an infinite loop and consequently this makes the screen flicker a lot.

@guidoschmidt
Copy link
Owner

Hey @LemonBreezes thanks for reporting, I also encountered this the other day, but didn't quite understand why it's happeneing.

@guidoschmidt guidoschmidt added this to the 0.4.1 milestone Apr 26, 2024
@LemonBreezes
Copy link
Contributor Author

Hey @LemonBreezes thanks for reporting, I also encountered this the other day, but didn't quite understand why it's happeneing.

I'm not 100% sure why it's happening but #34 seems to fix it for me. Try it out.

@guidoschmidt guidoschmidt modified the milestones: 0.4.1, 0.5.0 Apr 26, 2024
@agzam
Copy link

agzam commented Apr 27, 2024

I apologize for pointlessly complaining without any constructive arguments, but I think any feedback sometimes is still better than none at all.

I honestly don't understand the latest development pace of this package. I used it for years, and it simply worked. There was only one problem that I didn't like - it wouldn't let you switch the theme immediately after the activation of the next circadian-theme; you'd have to wait for a minute to manually override auto-switching. It would also heat up the CPU for a bit, but it was never a deal-breaker.

Recently it has gotten too chatty - it would complain if you don't set calendar-latitude and calendar-longitude, even though you simply want to use time-based values. Now there's a new bug - the infinite loop it gets into that can't be killed even with -SIGUSR2.

Please don't get the wrong message here @guidoschmidt, I have never contributed to the project, I have never spent a dime of donation; It's not just about me, after all, I can simply pin the older commit and move on. I'm a simple consumer, like many others. I am absolutely grateful for the work you've done and I want to keep using it, and I want to impress my friends and colleagues on how awesome and smart my Emacs is - "look Ma, it's magically changing the colors". Perhaps, and it is only a suggestion, if I may, can I ask you to find a bit more conservative pace? Because no matter how great the excitement is for the new features, the disappointment of broken fundamental mechanics is always far greater.

Again, I apologize for the whiny tone and thank you for this great gift, and for your continuous effort to make it even better.

@guidoschmidt
Copy link
Owner

guidoschmidt commented Apr 27, 2024

@agzam very valid points and your feedback is much appreciated.

however, there's not much to understand about the recent development pace - I just finally found some time (and motivation) to work on it again and address some things that bothered me a long time ago personally, as this project was initially built for myself. And then as so many people did find it useful, I tried to improve on the reported bugs as much as possible as well.

I just didn't have the time with full time work and private things and issues in the recent years. So I was also just using the old version for quite some time sacrificing for the little quirks it had, just like you.

Anyway it was a shocking moment to see this bug on my machine showing up after some time and I have to admit my elisp is a bit rusty, only did a bit of config stuff here and there for my own .emacs.d lately.
Actually it was a bad idea to work on the package right on the main branch (eg as people might be using the repo via straight.el etc.), I created a develop branch yesterday instead. Also this bug is very nasty and hard to detect by automatic tests 😵‍💫.

I suggest you stick to an old version tag for now, if you were happy with the package before 0.4.0.

But I'd be more than happy to get some more experienced elisp developer eyes on this thing or just have a few people testing develop. I might need to establish a set testing workflow for myself before releasing new versions, too.

About the verbosity, imo it helps to inform what's going on. But to your point, I might want to make it configurable indeed 👍🏽

finally, thanks again for your constructive criticism, and again I'd be more than happy to get helped (eg on the open PR #35, in general via issues or even improvements via PR)

@guidoschmidt guidoschmidt linked a pull request Apr 27, 2024 that will close this issue
@tangxinfa
Copy link
Contributor

develop version looks like fixed the infinite loop problem, of course i should use at least one day to examine theme switch by time works as expected.

Use it by specify the branch:

(use-package circadian
  :quelpa (circadian :fetcher github :repo "guidoschmidt/circadian.el" :files ("*") :branch "develop")
  ...)

For avoid the package influence more users, how about rollback the package in elpa?

Or release the develop branch soon, at least let Emacs usable.

@tangxinfa
Copy link
Contributor

develop version still have the problem

@guidoschmidt
Copy link
Owner

@tangxinfa thanks for checking, also I rolled back the main branch to 0.3.3. Would you mind testing the latest commit 8dad94ee37159907472fc5046b6e3a4d0c8f5ec1 on develop?

I think I have a suspicion, why this is happening: circadian-encode-time was only setting times for the current day: here, which made any timer before the current time on that day run immediatley, e.g. if you eval-expression this at a time AFTER 13:00, it will run immediately:

(run-at-time "13:00" nil (lambda () (print "runs immediately")))

so in order to fix it we have to make sure any timer will be in the future: circadian-encode-time @ latest develop branch ... I hope this fixes is for good. I'm still testing and reading through the Emacs timer documentation a bit.

@tangxinfa
Copy link
Contributor

I tested the last commit(0ec8b12) on develop branch, it works as expected.

I configuring 5 themes, e.g:

(setq circadian-themes '((:sunrise . wombat)
                        ("10:20"   . ef-spring)
                        ("10:21"  . ef-summer)
                        ("10:22"  . ef-autumn)
                        ("10:23"  . ef-winter)))

These themes switched correctly in time, list-timers shows there is a
circadian-enable-theme timer scheduling to run tomorrow.

@meedstrom
Copy link

meedstrom commented May 9, 2024

As you said in #28 (comment), changing <= to < in circadian-a-earlier-b-p made the tests fail, but maybe the tests are wrong?

@guidoschmidt
Copy link
Owner

As you said in #28 (comment), changing <= to < in circadian-a-earlier-b-p made the tests fail, but maybe the tests are wrong?

Sure, possible. Happy to accept PRs with better or corrected test cases with a description why the test was wrong in the first place.

Though in general #35 / #develop branch seems to be quite stable now for me since a week.

@guidoschmidt guidoschmidt self-assigned this May 25, 2024
@Nathan-Furnal
Copy link

Hi there, any update on this, I'd be pretty grateful to have it updated on MELPA so I can use circadian again =)

Thanks for the package!

guidoschmidt added a commit that referenced this issue Jun 3, 2024
* Create FUNDING.yml

* PERF/circadian.el: use memoized sunset & sunrise times (#29)

* Update issue templates

* EDIT: ignore :sunrise and :sunset, if calendar lat/lng settings are missing

* FIX: correct testing, filter :sunrise and :sunset if calendar lat/lng is not set

* FIX/test.el: re-enable test-circadian-setup-benchmark

* FIX/test.el: move timezone test to the end as it changes lat lng

* EDIT: check for theme lists to support #16

* FIX/test.el: typo

* FIX 28: emacs is freezed at switch (#31)

* EDIT/circadian.el: add print statement to circadian-activate-latest-theme

* FIX: potential fix for #28

* FIX/test.el: move timezone test to end

* EDIT/.github/workflows/ci.yml: drop Emacs 26.3 support

* EDIT/circadian.el: moved check for already enabled theme from `circadian-enable-theme` to `circadian-activate-latest-theme`

* EDIT: cleanup

* FIX: after merge

* FIX: remove print statements, make sure timer runs only once

* FIX: timer issue + rename circadian-activate-latest-theme to circadian-activate

* FIX/circadian.el: proper use of new `circadian-activate-and-schedule`

* FIX/test.el: adjust tests to use new API

* DOC/README.md: adjust usage section

* DOC/README.md

* FIX: use format-time-string

* FIX: improve timer scheduling

* FIX: wrong function in timer

* FIX: solve recursive timer calls with timer variable + cancel-timer

* FIX/DOC/README.md: proper inner list for random selection from theme list

* FIX: endless loop #33 + code cleanup, adjust tests

* EDIT: introduce circadian-verbose + circadian-stop

* FIX: cleanup, remove circadian-- prefix in favor of single dash prefix

* FIX: disable-theme only if requested theme is not part of custom-enabled-themes

* FIX: check if next run is today or tomorrow

* EDIT/test.el: extend test-circadian-time-comparisons

* FIX/circadian.el: ensure a single timer + timer is always set

* EDIT/README.md: hint about `circadian-verbose`

* FIX #32: proper time zone settings according to Emacs documentation

* EDIT/circadian.el: remove print statement

* FIX/test.el

* UPGRADE/.github: upgrade actions/checkout + actions/cache to version 4

* FIX/circadian.el: check for timer nil + enable theme -> set timer to nil

* FIX/circadian.el: proper canceling of previous timer

* EDIT/circadian.el: update version in comment

---------

Co-authored-by: StrawberryTea <luneth1314@gmail.com>
@guidoschmidt
Copy link
Owner

Hi there, any update on this, I'd be pretty grateful to have it updated on MELPA so I can use circadian again =)

Thanks for the package!

@Nathan-Furnal thanks for asking, I was afk for a few weeks and got back to this today, just released 1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants