FrancescoCeruti / linux-show-player

Linux Show Player - Cue player designed for stage productions
https://linux-show-player.org
GNU General Public License v3.0
205 stars 49 forks source link

Sometimes cues in Cart Layout are not restored into their saved positions. #156

Closed s0600204 closed 5 years ago

s0600204 commented 5 years ago

(Tested on the develop branch, haven't tried in v0.5.1 release.)

Expected Behaviour

When loading a showfile, cues in the "Cart" layout are placed in the positions they were in when the showfile was last saved,

Actual Behaviour

Under certain conditions, cues in the "Cart" layout are not in the positions they were in when the showfile was last saved.

Instead they appear in the order they are recorded in the showfile's JSON, with the first cue at 0,0,0 (page,row,column), the second at 0,0,1, the third at 0,0,2, and so on.

To replicate:

  1. Create a showfile using the "Cart" layout. Add at least one cue, and move it to a different row, column, and/or page,
  2. Save, and close LiSP,
  3. Open this file via the the myriad of ways possible.
What works:
What doesn't work:

Diagnosing:

  1. Open the file core/signal.py
  2. Find the method Signal::connect (about line 172).
  3. Add logging.warn(slot_callable) before the line with self.__lock:.
  4. Start LiSP as usual from the Command Line, so that you start up in "Cart" layout.
    • If you've automatically started in "List" layout, set LiSP to ask which layout to use, then restart LiSP.
  5. Thanks to the added line, whenever a method is connected as a slot to a signal (or whatever the correct terminology is), that method's name is printed to the console.
    • You might note that towards the end of the list, some of the methods listed come from the Cart Layout (or one of its models).
  6. Open the showfile created above.
    • You will note that more methods/slots are listed.
  7. Close, and restart LiSP, this time into "List" layout.
    • The methods being connected are similar to those before, but with "List" layout methods instead of "Cart" layout methods.
  8. Open the showfile created above.
  9. A lot more methods are added. Look closely: you might note that some of them are "List" layout methods (even though we're now using the "Cart" layout).

And here appears to be the problem. We've opened a showfile using the "Cart" layout. But because we've previously had the "List" layout active (and not restarted the application), the "List" layout methods/slots are still connected.

Theory

In the "Cart" layout, cue indexes can be non-contiguous. In the "List" layout, they must be contiguous. Thus, the "List" layout has code in the method that runs when a cue is added to ensure that the cue added has a cue index that conforms, and "corrects" it if not.

And that method is still being called...

To test this theory:

  1. Open plugins/list_layout/models.py,
  2. Find the method CueListModel::_item_added (about line 61),
  3. Replace item.index = len(self.__cues) with pass,
  4. Restart LiSP into "List" layout,
  5. Open the showfile created above in "To Replicate" Step 1,
  6. This time, the cue-button is on the correct page, and in the correct place within it, because its index is not being "corrected" by a slot that should no longer be connected.

Solution

Ideally, slots belonging to the "List" layout (and its models) should be disconnected when switching to the "Cart" layout. (There are some slots that should or could remain connected, such as those added by the Triggers, Controller and Timecode plugins.)

This could be achieved by altering the finalize method of the two layouts to disconnect any and all Slots the layout (and their specific sub-classes and models) have connected.

Thoughts?

FrancescoCeruti commented 5 years ago

Thanks for the very accurate report :smile: I can't reproduce the problem :confounded: which is odd, but I think I know what is happening.

So your theory is very likely correct, as the direct cause of the problem, but, even if you don't disconnect the signals (those from core/signal.py not the Qt ones) this shouldn't happen. Those connections will die with the old layout (and it's model), this should happen during Application._new_session where the old session is "deleted", if this doesn't happen it means that probably something is keeping one of the object in the chain session->layout->layout-model-adapter alive.

:rocket: To debug this you can use objgraph specifically objgraph.show_backrefs, something like objgraph.show_backref(objgraph.by_type("<ClassName or fully.qualified.ClassName>"))

Those connections will die with the old layout

To be more clear on this point, the Slot class used by Signal to wrap the connected callable (functions/methods/...) keeps only a weak-reference of the the callable, this way a connection signal->slot will never keep alive another object.

There is a second (less likely) possibility, as said by the python weakref documentation:

However, until the object is actually destroyed the weak reference may return the object even if there are no strong references to it.

:rocket: You can try to test the above using gc.collect, and force a garbage-collection in Application._new_session

s0600204 commented 5 years ago

:man_facepalming: seems I've committed a rookie error - I didn't check whether this was the base code, or caused by some of the various changes I've made locally. This error is, in fact, self-inflicted.

FrancescoCeruti commented 5 years ago

@s0600204 no problem :smile: