Move icon determination into TrackLabelButton again #7209
Conversation
Fully undo the changes made in commit 88e0e94 because the intermediate revert made in commit 04ecf73 seems to have led to a performance problem due to the icon being set over and over again in `TrackLabelButton::paintEvent`. The original intention of the changes made in pull request LMMS#7114 was to remove the paining code that dynamically determines the icon over and over again. Ideally the icon that is used by an instrument should be somewhat of a "static" property that should be known very early on when an instrument view is created. There should not be any need to dynamically resolve the icon over and over, especially not in a button class very far down in the widget hierarchy. However, due to technical reasons this is not the case in the current code. See pull request LMMS#7132 for more details.
Revert some of the changes made in commit 88e0e94. The underlying idea was that the `InstrumentTrackView` should be responsible for assigning the icon that's shown by its `TrackLabelButton`. However, this does not work because in some cases the `InstrumentTrack` that's passed into `InstrumentTrackView::determinePixmap` does not have an `Instrument` assigned. This in turn seems to be caused due to initalizations that are running in parallel in different threads. Here are the steps to reproduce the threading problem (line numbers refer to commit 360254f): 1. Set a break point in line 1054 of `InstrumentTrack`, i.e. the line in `InstrumentTrack::loadInstrument` where `m_instrument` is being assigned to. 2. Set a break point in `InstrumentTrackView::determinePixmap`, e.g. inside of the first if statement. 3. Drop an instance of "Sf2 Player" onto the Song Editor. 4. The first break point in `InstrumentTrack` is hit in a thread called "lmms::Instrumen" (shown like that in my debugger). Try to step over it. 5. The second break point in `InstrumentTrackView` now gets hit before the code is stepped over. This time we are in the thread called "lmms". I guess this is the GUI main thread. 6. Continue execution. If you now switch to the application then the icon is shown. I guess the debugger is halted long enough in the main thread so that the InstrumentTrack gets an instrument assigned in another thread. If you delete/disable the break point in `InstrumentTrack::determinePixmap` and follow the coarse steps above then the icon is not shown because the track has no instrument. The current fix still delegates to the `InstrumentTrackView` to determine the pixmap in hopes that one day there will be a better solution where the parent component can be fully responsible for its child component. Fixes #7116.
|
As a mitigation, I'm okay with merging this revert. As a solution we should probably tie the icon to a slot/signal of some sort and pull it out of paintEvent altogether. |
It's now back to the old implementation. If you check pull request #7114 you will find that the original improvement has just set the icon of the In my opinion even a signal/slot connection would be the wrong solution. Every instrument should know it's icon from the get-go so that it can be used in the view. The problem with #7114 was that for some funny reasons some instruments do not know their icons after construction and somehow get it set/changed later when it's too late and |
Fully undo the changes made in commit 88e0e94 because the intermediate revert made in commit 04ecf73 seems to have led to a performance problem due to the icon being set over and over again in
TrackLabelButton::paintEvent.The original intention of the changes made in pull request #7114 was to remove the paining code that dynamically determines the icon over and over again. Ideally the icon that is used by an instrument should be somewhat of a "static" property that should be known very early on when an instrument view is created. There should not be any need to dynamically resolve the icon over and over, especially not in a button class very far down in the widget hierarchy. However, due to technical reasons this is not the case in the current code. See pull request #7132 for more details.
Should fix #7207.