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

WIP: add setting to change tracking interval #2355 #3460

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

Minnowo
Copy link
Contributor

@Minnowo Minnowo commented Sep 14, 2024

Description

Added a setting Time tracking interval under the Time Tracking category which lets the user change the tracking interval.

image

I've put the title here as WIP since I am a new contributor, I have never used Angular, and have very limited experience with Typescript. So feedback is appreciated.

I've got some more testing but I think most of the functionality is there. Changing the setting properly updates the interval, and the value is persisted across page loads.

I still need to setup Electron and I'm not sure what to do about the other i18n files.

Issues Resolved

This is a bit of a simple fix for #2355.

Check List

  • New functionality includes testing.
  • New functionality has been documented in the README if applicable.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there Minnowo! 👋

Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖

We will try to review it soon!

@johannesjo
Copy link
Owner

Thank you very much! This looks very clean and I am quite happy. Two thoughts:

  1. Maybe we should put the setting at the end of the settings form and mark it with something like "EXPERIMENTAL!", since it is something for more technically versed users.
  2. Related to that we should add more context on why someone want to change this setting (to reduce disk writes).

@johannesjo
Copy link
Owner

johannesjo commented Sep 15, 2024

Something like

    TestBed.configureTestingModule({
      providers: [provideMockStore({ initialState: {} })],
    });

in global-tracking-interval.service.spec.ts will likely fix the error.

@Minnowo
Copy link
Contributor Author

Minnowo commented Sep 16, 2024

Thank you very much! This looks very clean and I am quite happy. Two thoughts:

  1. Maybe we should put the setting at the end of the settings form and mark it with something like "EXPERIMENTAL!", since it is something for more technically versed users.
  2. Related to that we should add more context on why someone want to change this setting (to reduce disk writes).

Glad everything looks good!

I have added (EXPERIMENTAL) to the title, extended the description a bit. Here is how it looks now:
image

@Minnowo
Copy link
Contributor Author

Minnowo commented Sep 16, 2024

I have also added the change you suggested for that test case. Running tests using Chromium seems to pass now on my machine.

@johannesjo
Copy link
Owner

Thank you very much! This is a great addition! I'll try to figure something out for the timers once I have more time on my hands!

@johannesjo johannesjo merged commit d34ef38 into johannesjo:master Sep 16, 2024
5 checks passed
@davidhealey
Copy link

Why is the max limited to 100000? I would like to limit writes to every 5 minutes as I don't see any need for it to write more frequently than that.

@Minnowo
Copy link
Contributor Author

Minnowo commented Nov 18, 2024

Why is the max limited to 100000? I would like to limit writes to every 5 minutes as I don't see any need for it to write more frequently than that.

It was an arbitrary number I had picked that seemed large enough at the time. I didn't really think much of it.

Here is the line if you're interested in changing it:

@davidhealey
Copy link

Thank you

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

Successfully merging this pull request may close these issues.

3 participants