Skip to content

Conversation

HighPriest
Copy link

@HighPriest HighPriest commented Sep 18, 2025

  • [/] Format the code with prettier
  • [/] Make icons reflect possible interaction, instead of current state
    • Discuss with @semanticdata , which icons would be appropriate for the non-initiated state. It is a crossed out clock right now.
    • Logic being, that timer state should be extrapolated from the timer ticking, or not. The Icon does not need to say that. The icon should be there, to tell the user, what is going to happen on interaction.
  • Renamed variables to more appropriate values
  • Moved from manual timing to waiting for an epoch
  • Pause state managed by storing left-over duration in timer
  • Running state inferred from ticker interval being set
  • Start / Pause managed in single toggleTimer place (you either pause, or unpause. no in-between)
  • In persistentNotification mode, let the timer overflow
    • ?? Fix the negative sign position, when the timer overflows (don't use moment.abs(), it is broken. Doesn't just return the value, it overrides the object it is ran on...)
    • Add a test, which is going to check for a standardised format of the output text (both when it is positive and negative)
  • Make moment lib usable with jest
  • [/] Handle tests with new code
    • Fix issue with floating point precision of moment.duration().toISOString() (.seconds() has the same issue)
    • Fix issue with Obsidian Settings Mock, not having .addDropdown() available

Hello there @semanticdata
I had tried your plugin a few months ago and had in the back of my head, that I would like to see persistentNotifications in it. Hence, this PR.
I've got to say... the amount of tests in the code is ASTRONOMICAL. There are more tests than there is actual code 😅

I don't think it is something I am going to be able to figure out on my own. Help and suggestions would be greatly appreciated.

@HighPriest
Copy link
Author

I've adapted all the tests, so they are passing.
Some of them had really odd logic, which looked like they were made to always pass, no matter the actual application logic... So, all the tests, even those that are currently passing, should be looked through.


I am still seeing an error

TypeError: (intermediate value).setName(...).setDesc(...).addDropdown is not a function

      147 |                     .setName("Sound Selection")
      148 |                     .setDesc("Choose a built-in sound or select 'custom' to use your own.")
    > 149 |                     .addDropdown(dropdown => {
          |                      ^
      150 |                             const builtInSounds = this.soundManager.getBuiltInSounds();
      151 |                             builtInSounds.forEach(sound => {
      152 |                                     dropdown.addOption(sound, sound.replace('.wav', ''));

      at PomodoroSettingTab.display (src/components/SettingsTab.ts:149:5)
      at Object.<anonymous> (tests/settings-tab.test.ts:146:18)

which I would like to ask you, @semanticdata to correct. I don't know where to start looking.

- Renamed variables to more readable values
- Moved from manual timing to waiting for an epoch
- Pause state managed by storing left-over duration in timer
- running state inferred from ticker interval being set
- Start / Pause managed in single toggleTimer place (you either pause, or unpause. no in-between)
There are still issues with floating point precision
and "addDropdown" for some reason does not exist in Obsidian Settings mock
@HighPriest HighPriest force-pushed the persistentNotification branch from 64837d3 to 5db6309 Compare September 18, 2025 23:22
Comment on lines 96 to 98
value ? this.plugin.settings.persistentNotification = false : null;
await this.plugin.saveSettings();
this.display();
Copy link
Author

Choose a reason for hiding this comment

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

This is fun.
Toggles the persistentNotification setting off, when autoProgress is enabled. Neither option can function with the other enabled, so... this is a nice alternative to dropdown selector.

Comment on lines -9 to +17
jest.mock('obsidian'); // This will use the __mocks__/obsidian.ts automatically
jest.mock('obsidian', () => {
const obsidian = jest.requireActual('obsidian');
const moment = jest.requireActual('moment');

return {
...obsidian,
moment
}
}); // This will use the __mocks__/obsidian.ts automatically
Copy link
Author

Choose a reason for hiding this comment

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

This was required for moment to work with the tests.
It might be a reason why the Settings mock breaks and we get an error with .addDropdown()

@semanticdata
Copy link
Owner

Hello, sorry I've been busy at work recently. I'll take a look at this as soon as I get a chance.

Thank you very much for taking the time to collaborate.

@semanticdata semanticdata self-assigned this Sep 23, 2025
@semanticdata semanticdata added bug Something isn't working enhancement New feature or request labels Sep 23, 2025
@HighPriest
Copy link
Author

HighPriest commented Sep 23, 2025

No pressure @semanticdata

I have been actually using the timer, with persistent notification & enjoyed it. Only issues being:

  • The first notification is often delayed, because the sound has to be pulled from CDN every time
  • There is no stats storage, which makes the app feel like it has no agency over me using it, or not. Making it forgetable.

There is still alot that can be done better. And, I am going to be gone from programming, for at least a month now. So, no pressure whatsoever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants