Track operations widget with layout #7537
Conversation
Put the elements of the `TrackOperationsWidget` into layouts. These are: * The grip that can be used to move tracks * The gear icon that opens the operations menu * The mute button * The solo button The grip that can be used to move tracks around is extracted into its own class called `TrackGrip`. This has several advantages: * It can be put into a layout. * It can render itself at arbitrary sizes by simply repeating its pattern pixmap. * It can be used in a much more object-oriented way because it emits signals when it is grabbed and released. * It is responsible for locally updating its cursor state. The default cursor of the grip now is an open hand which indicates to the users that it can be grabbed. While being grabbed the cursor now is a closed hand. ## Technical details The class `TrackOperationsWidget` now holds an instance of `TrackGrip` and provides a getter to retrieve it. This getter is used by `TrackView` to connect to the two new signals `grabbed` and `released`. The method `TrackOperationsWidget::paintEvent` now only paints the background as it does not need to paint the grip anymore. The `TrackView` now handles the grabbing and release of the grip in `TrackView::onTrackGripGrabbed` and `TrackView::onTrackGripReleased`. Because the events and cursor states are now handled by `TrackGrip` this code could be removed from `TrackView::mousePressEvent`. There was a comment in `TrackView` which indicated that the `TrackOperationsWidget` had to be updated when the track is moved and released because it would hide some elements during the move. The comment and the corresponding code was removed because the operations widget does not hide its elements during moves (this was already the state before the changes made by this commit). Adjust the style sheets of the classic and default themes with regards to the `QPushButton` that's used to show the gear menu in the `TrackOperationsWidget`. The `>` has been removed because the `QPushButton` is not a direct decendent of the `TrackOperationsWidget` anymore. ### Wrapping of `PixmapButton` in `QWidget` The PixmapButtons that are used in `TrackOperationsWidget` current have to be wrapped into a `QWidget`. This is necessary due to some strange effect where the PixmapButtons are resized to a size that's larger than their minimum/fixed size when the method `show` is called in `TrackContainerView::realignTracks`. Specifically, with the default theme the buttons are resized from their minimum size of (16, 14) to (26, 26). This then makes them behave not as expected in layouts. The resizing is not done for QWidgets. Therefore we wrap the PixmapButton in a QWidget which is set to a fixed size that will be able to show the active and inactive pixmap. We can then use the QWidget in layouts without any disturbances. The resizing only seems to affect the track view hierarchy and is triggered by Qt's internal mechanisms. For example the buttons in the mixer view do not seem to be affected. If you want to debug this simply override "PixmapButton::resizeEvent" and trigger a break point in there, e.g. whenever the new size is not (16, 14).
Make the `PixmapButton` more friendly for layouts by implementing `minimumSizeHint`. It returns a size that accommodate to show the active and the inactive pixmap. Also make `sizeHint` return the minimum size hint. The previous implementation would have made layouts jump when the pixmap is toggled with pixmaps of different sizes.
|
@sakertooth, requesting a review from you as you have also made some changes with regards to layouts in the area of the mixer IIRC. The long term goal of these changes is described at the end of the description above. |
|
IMO, the items within the track operations layout should center itself whenever the track is resized. Might be out of scope, wanted to raise this one. |
I guess this one would need to be discussed with the team. IMO it might be more consistent to keep everything at the top. Otherwise you always have to hunt for the middle of a track especially if they have different sizes. If the elements stay at the top it might be visually easier to locate them because they are at the border between two tracks which is already a visual distinction in itself. Nevertheless, as you have already mentioned this is out of scope of this PR anyway. |
Hi @michaelgregorius, I am taking a break from LMMS development, but if I have time to spare this weekend, I might take a look. |
@sakertooth, sorry, I was not aware of that. And no need to take a break from your break. 😅 I'd also be grateful if you could propose a different reviewer for that code area. |
There was a problem hiding this comment.
I don't have much to say. This LGTM overall. Also tested the PR and it seems to work fine. However, if there's anyway we could fix the widget wrapping, that would be nice (just so that we could maybe simplify some stuff over there, but if not I'm fine with it as it doesn't seem to be causing many issues).
Comparing it to current master, I did it a minor shift in the positioning of the widgets (there are now slightly higher I think?). However, it's very minor and I can barely notice the difference, so it might not be a big issue. You can see for yourself if you want.
Don't use `SIGNAL` and `SLOT` to connect entities. Initialize `TrackGrip::m_track` to `nullptr` in the header. Whitespace adjustments for the lambda in `TrackOperationsWidget`.
Yes, that's really weird. I have no idea why the |
| void TrackView::onTrackGripGrabbed() | ||
| { | ||
| m_action = Action::Move; | ||
| } | ||
|
|
||
| void TrackView::onTrackGripReleased() | ||
| { | ||
| m_action = Action::None; | ||
| } |
There was a problem hiding this comment.
I think it's best to define simple functions like these within the header?
There was a problem hiding this comment.
IMO it's better to keep them in the implementation file because it is an implementation detail and a potentially complex operation that's triggered due to a signal/slot connection. Even if it is not complex in this case.
Put the elements of the
TrackOperationsWidgetinto layouts. These are:The grip that can be used to move tracks around is extracted into its own class called
TrackGrip. This has several advantages:The default cursor of the grip now is an open hand which indicates to the users that it can be grabbed. While being grabbed the cursor now is a closed hand.
When a track is being resized to have a larger height the grip now grows vertically with the track.
The results of all the changes can be seen in this video:
TrackOperationWidgetInLayoutGripClass.webm
This pull request is one step towards the goal of getting rid of the hard coded widths and heights in this code area:
lmms/include/TrackView.h
Line 52 in 066f6b5
It should be possible to have flexible elements in the tracks which take the size they need. The time line and related elements should use the current (maximum) width of the elements on the left side of the track view to move accordingly instead of relying on hard-coded sizes which make it hard to implement any changes.
Getting rid of hard-coded widths and heights would for example enable the tracks to have different configurations on how they render them. There could be a (vertically) compact view for people who want to see as many tracks as possible or a more "generous" view that might include more elements, e.g. meters, etc.
Technical details
The class
TrackOperationsWidgetnow holds an instance ofTrackGripand provides a getter to retrieve it. This getter is used byTrackViewto connect to the two new signalsgrabbedandreleased. The methodTrackOperationsWidget::paintEventnow only paints the background as it does not need to paint the grip anymore.The
TrackViewnow handles the grabbing and release of the grip inTrackView::onTrackGripGrabbedandTrackView::onTrackGripReleased. Because the events and cursor states are now handled byTrackGripthis code could be removed fromTrackView::mousePressEvent.There was a comment in
TrackViewwhich indicated that theTrackOperationsWidgethad to be updated when the track is moved and released because it would hide some elements during the move. The comment and the corresponding code was removed because the operations widget does not hide its elements during moves (this was already the state before the changes made by this commit).Adjust the style sheets of the classic and default themes with regards to the
QPushButtonthat's used to show the gear menu in theTrackOperationsWidget. The>has been removed because theQPushButtonis not a direct decendent of theTrackOperationsWidgetanymore.Wrapping of
PixmapButtoninQWidgetThe PixmapButtons that are used in
TrackOperationsWidgetcurrent have to be wrapped into aQWidget. This is necessary due to some strange effect where the PixmapButtons are resized to a size that's larger than their minimum/fixed size when the methodshowis called inTrackContainerView::realignTracks. Specifically, with the default theme the buttons are resized from their minimum size of (16, 14) to (26, 26). This then makes them behave not as expected in layouts.The resizing is not done for QWidgets. Therefore we wrap the PixmapButton in a QWidget which is set to a fixed size that will be able to show the active and inactive pixmap. We can then use the QWidget in layouts without any disturbances.
The resizing only seems to affect the track view hierarchy and is triggered by Qt's internal mechanisms. For example the buttons in the mixer view do not seem to be affected.
If you want to debug this simply override "PixmapButton::resizeEvent" and trigger a break point in there, e.g. whenever the new size is not (16, 14).
More layout-friendly PixmapButton
Make the
PixmapButtonmore friendly for layouts by implementingminimumSizeHint. It returns a size that accommodate to show the active and the inactive pixmap.Also make
sizeHintreturn the minimum size hint. The previous implementation would have made layouts jump when the pixmap is toggled with pixmaps of different sizes.