BlackyHawky / Clock

Privacy-conscious open-source clock, based on AOSP Clock
Apache License 2.0
162 stars 11 forks source link

Timer will continue to run if it is set to less than 5 seconds. #5

Open quadratz opened 2 years ago

quadratz commented 2 years ago

Steps to reproduce:

  1. Set timer to less than 5 seconds and click Start.
  2. The timer exceeds the applied limit (resulting in a negative value).
  3. The timer always stops and ringing at exactly the 5th second, regardless of the setting.

Expectation:

qw123wh commented 2 years ago

I also made some tests, I find the same error as you. I am currently unable to find the solution.

GokhanKabar commented 2 years ago

can you assign me ?

Aladin056 commented 2 years ago

Hi @qw123wh @CikiMomogi , i am interested by the issue , i would like to know if you are still active on the project because i can fin a solution . Thank you

qw123wh commented 2 years ago

@GokhanKabar @Aladin056 the project is active, if you can solve the issue of pull requests, I will then join them to the main branch. I am looking for collaborators for this project, I have some nice ideas to make this app even more functional. Your help is greatly appreciated

BlackyHawky commented 3 months ago

Impossible to find the cause... 😭

ghost commented 3 months ago

I test the app and i have some notes:

The timer will continue counting negative (when foreground or background) if set to 1 or 2 secs. When reaching -4 or higher, it will ring and sound only if in foreground, otherwise will continue counting negative.

BlackyHawky commented 3 months ago

I test the app and i have some notes:

The timer will continue counting negative (when foreground or background) if set to 1 or 2 secs. When reaching -4 or higher, it will ring and sound only if in foreground, otherwise will continue counting negative.

I've also noticed this, but I can't find the part of the code that causes this bad behavior.

Note: it seems to be the same on the LineageOS clock application.

ghost commented 3 months ago

No, @BlackyHawky

I have another phone (with LineageOS 18.1 -Android 11- on it), and this problem is not found in the Clock app, except when you set the timer for 1 second (i think because the 1 sec. timer is very low or small to handle correctly)

BlackyHawky commented 3 months ago

@MaroEldawwy6 Check out the video below; this is the application compiled from the official LineageOS repository.

It'll confirm what I've said...

Watch video
https://github.com/BlackyHawky/Clock/assets/139015663/f458393e-6947-4c89-8480-1ce3d36bdeec
qw123wh commented 3 months ago

@BlackyHawky I found the solution,

https://github.com/botxxxx/lib-com.android.deskclock/commit/5f19c681d835959ffea71274408ffcf315f7592e

I tested it on the old AOSP clock, the first on the repository that I haven't updated for 3 years; the changes of TimerItemFragment.java works. Now see if you can adapt this to the new code, since the TimerItemFragment.java file no longer exists

BlackyHawky commented 3 months ago

@qw123wh You are magic !!!! 🎉🎉 It's an excellent search that unfortunately only works if the application remains in the foreground.

Did you find the same thing?

qw123wh commented 3 months ago

@BlackyHawky Yes same problem, I didn't notice it. Partially resolved.

Create a branch with the modification, then we may find some solution to solve it completely

BlackyHawky commented 3 months ago

Create a branch with the modification, then we may find some solution to solve it completely

Or simply include the modification that works and change the title of this issue.

@Nilsu11 It seems to me that you use or have used Omniclock. Am I right? If so, can you confirm that everything works on this app? Do you have the source of the repository?

Nilsu11 commented 3 months ago

Yes, I used it. In Omniclock the issue also exists.

@BlackyHawky LineageOS deskclock has fixed it!! (little delay after time's up the ringtone plays)

BlackyHawky commented 3 months ago

LineageOS deskclock has fixed it!! (little delay after time's up the ringtone plays)

@Nilsu11 I hadn't seen that you'd updated your comment.

Where did you find the fix?

I'm using a version compiled from the official repository for testing and the bug is still there.

Nilsu11 commented 3 months ago

I'm using an official Lineageos build. On my phone it works.

BlackyHawky commented 3 months ago

I'm using an official Lineageos build. On my phone it works.

Can you tell me what version of the app it is? ( Even if I'm not sure it helps... )

Nilsu11 commented 3 months ago

I'm using an official Lineageos build. On my phone it works.

Can you tell me what version of the app it is? ( Even if I'm not sure it helps... )

21-20240527-NIGHTLY-oriole App version it tells me 14.

odmfl commented 3 months ago

@Nilsu11 extract the apk from your phone and send it here so we can test it too.

Nilsu11 commented 3 months ago

DeskClock.apk.zip

odmfl commented 3 months ago

DeskClock.apk.zip

@Nilsu11 I tested the apk I always have the same problem, if you set 3 seconds it sounds at -2.

Nilsu11 commented 3 months ago

@odmfl @BlackyHawky It's now fixed in OmniClock

BlackyHawky commented 3 months ago

@Nilsu11 Does this work when the app is minimized and with the screen off?

Here, the file you modified in Omniclock was modified in 2016 with commit f4c174d... 😅

Nilsu11 commented 3 months ago

@BlackyHawky Yes it works. I mean something like this commit. This change should do something similar like the one from before. I used the TimerReceiver just as central point, where the message, that the timer starts is delivered. In OmniClock there sadly isn't a void to do this but just the TimerReceiver. I didn't check it on Android Studio nor compile it. This is just to visualize, what I mean.

BlackyHawky commented 3 months ago

Unfortunately it only works if you don't pause the timer.

Steps to reproduce:

In any case, it seems that you are not far from the solution!

Nilsu11 commented 2 months ago

@BlackyHawky If you put an if clause into the place, where the state is set to timer expire, to check the state the timer can expire from, this issue should be resolved (Hopefully you know what I mean).

I will demonstrate this in OmniClock, but it will have to wait for another commit I am currently working on (This will take a week or two).

BlackyHawky commented 2 months ago

(Hopefully you know what I mean).

No I don't. 😅

...but it will have to wait for another commit I am currently working on (This will take a week or two).

This issue was created over 2 years ago, so we're in no hurry. 😉

Nilsu11 commented 1 day ago

@BlackyHawky Can you try replacing the if else clause here with this?

if (nextExpiringTimer == null) {
    // Cancel the existing timer expiration callback.
    final PendingIntent pi = PendingIntent.getService(mContext,
                    0, intent, PendingIntent.FLAG_ONE_SHOT | PendingIntent.FLAG_NO_CREATE | PendingIntent.FLAG_IMMUTABLE);
    if (pi != null) {
        mAlarmManager.cancel(pi);
        pi.cancel();
    }
} else if (nextExpiringTimer.getRemainingTime() < 5) {
    startService(intent);
} else if (nextExpiringTimer.getRemainingTime() < 5000) {
    new Handler().postDelayed(new Runnable() {
        public void run () {
            updateAlarmManager();
        }
    }
} else {
    // Update the existing timer expiration callback.
    final PendingIntent pi = PendingIntent.getService(mContext,
                    0, intent, PendingIntent.FLAG_ONE_SHOT | PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE);
    schedulePendingIntent(mAlarmManager, nextExpiringTimer.getExpirationTime(), pi);
}
BlackyHawky commented 12 hours ago

@Nilsu11 I've tested it but it doesn't work properly when the application is reduced.

Moreover, a long value is missing to apply the new Handler function ; perhaps I should set the value to nextExpiringTimer.getRemainingTime() ?

Then, are you sure about nextExpiringTimer.getRemainingTime() < 5 and nextExpiringTimer.getRemainingTime() < 5000 ? Why are these values different?

Finally, should i apply this commit as well?

Nilsu11 commented 11 hours ago

@BlackyHawky Sorry, yes, I forgot you are right the long value should be nextExpiringTimer.getRemainingTime()

As for the different values the 5 is for 5 ms. It actually should be 0 but for good measure I used 5. It shouldn't make a difference. The purpose is to start the timer service directly if the time is below 5ms. Before one would have waited 5 seconds when the time is already up. The 5000 is because of the delay of the AlarmManager necessary which should trigger the method again to check if there still is time's up status or not.

No, it's either one or the other. The solution to the timer problem is looking if there is a timer to expire at the remaining time if it is under 5 seconds (min. delay when using alarmManager to trigger something).