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

Bug when marking all plants as cared for #25

Open
saenglert opened this issue Sep 27, 2024 · 4 comments · May be fixed by #26
Open

Bug when marking all plants as cared for #25

saenglert opened this issue Sep 27, 2024 · 4 comments · May be fixed by #26

Comments

@saenglert
Copy link

When using the button to mark all plants as cared for, the app marks resets all tasks, including ones which are not due today.

OS: GrapheneOS 2024091900
Florae version: 3.0.0

@saenglert saenglert linked a pull request Oct 2, 2024 that will close this issue
@aeri
Copy link
Owner

aeri commented Oct 20, 2024

Actually this is the desired behavior, my idea was to be able to mark all the plants cared for that require care today, in addition to those that have not been cared for in the past. If for some reason you could not take care of a plant at a past time, these are not accumulated but kept in pending.

We cannot apply past care to a plant but we can do it as soon as possible, so I think the idea of taking care of all the plants is good that it also applies to the care that could not be taken care of and is applied at the moment of selecting the option.

@saenglert
Copy link
Author

I think we have the same idea, but I guess I didn't explain my point well enough. So I made a couple images and diagrams

The first image shows one of my plants. None of its task are due today:

before

The second image shows the same plant after I pressed "Care all" on the home screen. Note how all care tasks have been updated, although none of them were due today:

after

Looking at the code, I'd expect the logic in the careAllPlants Function to work similar to the following diagram. Also, I included a version of the current behaviour that would keep the current behaviour as well.

florae
care_all_updated

@aeri
Copy link
Owner

aeri commented Oct 22, 2024

I see, now with the example I understand what you mean, thanks for detailing it.

The current implementation is not the right one because it also affects future care that should not be taken into account, so I think that this condition should be left as follows:

if (daysSinceLastCare != 0 && daysSinceLastCare >= c.cycles)

With this the expected behavior is achieved, the current plants will be taken care of and those that were not taken care of in the past, but future plants will not be touched.

What do you think?

@foss-
Copy link

foss- commented Oct 28, 2024

This explains why I never saw some plants showing in the due today view. Apparently those were unexpectedly updated when using "Care All". I agree, plants not shown in due today or due in the past should not be updated when using the "care all" option, which should only update the plants shown in the today view / overdue view.

What you described in your last comment sounds like the expected behavior to me.

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 a pull request may close this issue.

3 participants