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

A zombie recipe ;), just cosmetic thing. #645

Closed
ghost opened this issue Apr 2, 2022 · 5 comments
Closed

A zombie recipe ;), just cosmetic thing. #645

ghost opened this issue Apr 2, 2022 · 5 comments

Comments

@ghost
Copy link

ghost commented Apr 2, 2022

Please create a recipe, add just one kind of malt (for testing purpose) then delete the recipe and do nothing else, just exit BT.
Once again start BT, on the right side of the screen you'll see the content of deleted recipe in all its glory,
but it should be gone for good now, right?.The recipe was deleted and program restarted, why it is still there.
Having déjà vu, a glitch in matrix :), I thought I had successfully removed it. Just a cosmetic thing.
Regards :)

@matty0ung
Copy link
Contributor

Ah, good spot! Yes, in many circumstances, when items are deleted in Brewtarget they are just tagged with a "deleted" flag rather than actually removed from the DB.

What should happen is that, when you delete the currently selected recipe, we should select another one - perhaps the first in the list. Otherwise the deleted recipe remains displayed and editable - including after closing and restarting the program, as you've spotted.

Hopefully this is a small fix. I'll have a look...

@matty0ung
Copy link
Contributor

From a bit of looking, I think the issue only arises when the recipe you're deleting is the last one in the list. If you delete a recipe that's not last in the list, then the next one in the list becomes the selected one straight away. So, probably just need to add some logic to try to select the previous one when the last one is deleted.

@ghost
Copy link
Author

ghost commented Apr 2, 2022

Hello
Good Sir from my perspective, some more logic sounds good :).
What about idea that recipe composer window will get open with default values
( zeroed, no ingridients etc.) in case when there are no available recipes. Well, it’s just a thought.
Anyways, from now on it can be only better :).
As always thanks for explanations and good job.

@matty0ung
Copy link
Contributor

Just to let you know, I found the issue. There is already logic in MainWindow::deleteSelected() to select a new recipe after one is deleted, and to return to the top of the list when we deleted the last recipe. However, it needed a small enhancement to deal with the case where there are folders. When you go back to the top of the "tree" of recipes, if the first item is a folder, you need to keep skipping through the list until you find an actual recipe or you get to the end of the tree.

I've coded a quick fix that will make its way into the code at some point and improves things in many cases. Essentially, when you delete the end recipe, it looks for the first recipe that's not in a folder and sets that as the selected one. However, there is another edge case that the fix doesn't cover, which is where all your recipes are in folders. This needs a slightly bigger fix in a different part of the code - to look through the full list of recipes including ones in folders. I've made a note in the code to come back to that at some point.

If it's OK with you, I won't do a separate patch for my quick fix, as the issue is more of a quirk rather than a problem. The fix will appear in Brewtarget at some point in the not too distant future, next time I merge a larger feature or fix back from Brewken. (I'm currently working on BeerJSON support, which isn't yet ready to merge!)

Please do keep reporting bugs you find. 👍

matty0ung pushed a commit to matty0ung/brewken that referenced this issue Apr 3, 2022
matty0ung added a commit to Brewken/brewken that referenced this issue Apr 3, 2022
@ghost
Copy link
Author

ghost commented Apr 3, 2022

You're very kind, there's no need to rush here.
I will gladly wait for bigger bunch of fixes, take your time please.
Best,

@ghost ghost closed this as completed Apr 11, 2022
matty0ung pushed a commit to matty0ung/brewtarget that referenced this issue Aug 14, 2022
This issue was closed.
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

No branches or pull requests

1 participant