search

Brewtarget / brewtarget

Main brewtarget source code repository.
GNU General Public License v3.0
312 stars 134 forks source link

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

Closed ghost closed 2 years ago

ghost commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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. :+1:

ghost commented 2 years ago

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,