guidoschmidt / circadian.el

Theme-switching for Emacs based on daytime
https://guidoschmidt.github.io/circadian.el
MIT License
176 stars 8 forks source link

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

Closed LemonBreezes closed 5 months ago

LemonBreezes commented 6 months ago

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 commented 6 months ago

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

LemonBreezes commented 6 months ago

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.

agzam commented 6 months ago

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 commented 6 months ago

@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)

tangxinfa commented 6 months ago

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 commented 6 months ago

develop version still have the problem

guidoschmidt commented 6 months ago

@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 commented 6 months ago

I tested the last commit(0ec8b121df73dfdea02714de6bf5b5613e78bf14) 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 commented 6 months ago

As you said in https://github.com/guidoschmidt/circadian.el/issues/28#issuecomment-2061437577, changing <= to < in circadian-a-earlier-b-p made the tests fail, but maybe the tests are wrong?

guidoschmidt commented 6 months ago

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.

Nathan-Furnal commented 5 months ago

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 commented 5 months ago

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