leo-editor icon indicating copy to clipboard operation
leo-editor copied to clipboard

Log Pane deleteTab() Does Not Delete Underlying Widget

Open tbpassin opened this issue 3 years ago • 5 comments

The Log Pane is a QTabWidget. To delete a tab, the Leo method deleteTab() calls the QTabWidget's removeTab() method. According to the Qt docs, this method does not delete the "page" widget. This "page" widget is the one the that is actually visible in the tab.

deleteTab() also does not remove the tab name from the tab dictionary, log.contentsDict.

Doing a tab removal this way would be helpful if the tab is expected to be added back again. But if one expects it to be completely gone so a new or modified version of the page widget can be inserted, then it is confusing and will lead to a problem of unfreed c++ Qt objects.

I don't want to say this is a bug until I understand if the current behavior is as intended.

tbpassin avatar Mar 27 '22 14:03 tbpassin

After a little more experimenting, it seems that with deleteTab(), the pane loses the reference to the display widget. It so, then there is probably a problem of either the python or the c++ object not getting destroyed.

The Qt docs say that when a tab is created the page widget's ownership gets passed to the tab widget. So the tab widget would ordinarily to responsible for deleting the page. But the docs say the page doesn't get deleted when the tab is removed. This means the page must hang around until the entire log pane is deleted, which would be when Leo gets closed. I suppose it's OK to have some one or two dangling pages hanging around, but I worry that the Python garbage collector will collect the Python wrappers, leaving the underlying Qt objects in a questionable state.

This whole topic of the Python wrapper getting deleted but the c++ object not, or vice-versa, is very confusing. It's hard to understand the consequences.

To me, it would seem that one should get a reference to the page widget with the tab widget() method, do deleteLater() on it, then run deleteTab(). But that might end up with the log frame trying to delete an already-deleted object at some later time. Ugh!

tbpassin avatar Mar 27 '22 15:03 tbpassin

This present behavior may be intentional. I have a dim recollection that old code may have reused the page.

I would prefer not to change the code unless there are demonstrable problems with it.

edreamleo avatar Mar 27 '22 17:03 edreamleo

@tbpassin On second thought, I can understand why you may be concerned with this issue.

I have delegated this issue to you, and tentatively assigned it to 6.6.1. I will leave it to you to submit a PR. Any such changes must be tested thoroughly.

edreamleo avatar Mar 27 '22 17:03 edreamleo

My thinking right now is to write a new method for deleting a tab - IF we can figure out something better to do - but to leave the existing method as is. That's in case some existing code expects it to be exactly this way. Future code could start using the new method.

tbpassin avatar Mar 27 '22 18:03 tbpassin

@tbpassin Sounds like a good plan.

edreamleo avatar Mar 27 '22 21:03 edreamleo