Brewtarget / brewtarget

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

Deleting a non-empty recipe folder causes a crash #622

Closed TechnicolorNarwhal closed 1 year ago

TechnicolorNarwhal commented 2 years ago

If a recipe folder is deleted and there is a recipe in it, Brewtarget crashes with "Segmentation fault (core dumped)". However, the folder can be deleted if all of the recipes are deleted first.

matty0ung commented 2 years ago

Thanks for raising this.

From a quick check I believe this is fixed by https://github.com/Brewtarget/brewtarget/pull/617 but that fix introduces a related bug that dragging a recipe into a folder does not show up the change in the UI until you restart the program (almost certainly a signals issue that will be a small fix).

Once https://github.com/Brewtarget/brewtarget/pull/617 is merged, I will have a look at fixing the drag-and-drop.

matty0ung commented 2 years ago

Making progress. Fixed the drag-and-drop, but also confirmed that deleting a non-empty folder does still cause a crash even with the new database layer (https://github.com/Brewtarget/brewtarget/pull/617). At first glance, I think it may be related to the fact that we delete the folder before we delete its contents (in BtTreeModel::deleteSelected). But my initial thoughts on such things often turn out to be wrong, so now investigating more deeply.

mikfire commented 2 years ago

There isn't really a folder to delete and nothing is in the folder. The folder is a display layer trick, driven by an attribute on each object. If no object refers to a folder, that folder ceases to exist. It gets tricky with sub-folders.

If I were to make a random guess, check the signals being emitted and make sure all of the deletes are done before we finish the remove row operation.

matty0ung commented 2 years ago

Thanks for the suggestion, will take a look.

matty0ung commented 1 year ago

I believe this is fixed by the commits above. Please re-open if not.