Align the rename box for tracks #7414

BaraMGB · 2024-08-01T12:42:23Z

Fixing #7413 , on rename we set the text of the TrackLabelButton to an empty String. After renaming we set the text to the tracks name.

It's my first PR after a cuple of years. I hope I did it right.;)

Rossmaxx · 2024-08-01T12:52:54Z

Wow, another dev returns.

Rossmaxx · 2024-08-01T13:08:30Z

Looks like this is related to #7114 and #7209, @michaelgregorius you have any idea?

sakertooth · 2024-08-01T13:46:20Z

I think it would be better for the renaming box to be "inlined" with the text box. When the user wants to rename the track, it would replace the text box with the renaming box itself, but that might be more work.

Rossmaxx · 2024-08-01T13:50:12Z

@sakertooth i agree. That should be the default approach, @BaraMGB hope you can implement that method without much issues.

BaraMGB · 2024-08-01T14:59:26Z

Yes, I will have a look at this. Shouldn't be that difficult.

michaelgregorius · 2024-08-01T15:26:48Z

Looks like this is related to #7114 and #7209, @michaelgregorius you have any idea?

In how far do you think it is related? As in "is caused by it"? IIRC, #7209 only undoes some of the changes that have been made with #7114.

BaraMGB · 2024-08-02T10:19:37Z

I reverted the hiding approach and lined up the ReadLine to the TrackLabelButtons text.

Rossmaxx · 2024-08-02T10:37:17Z

In how far do you think it is related? As in "is caused by it"?

I suspected that #7114 caused it. About the undoing, i thought the rename line edit was left out, causing the bug.

Rossmaxx

Style review. Also might help if you posted a screenshot demonstrating the fix.

src/gui/tracks/TrackLabelButton.cpp

BaraMGB · 2024-08-02T11:18:37Z

Rossmaxx · 2024-08-02T12:09:49Z

src/gui/tracks/TrackLabelButton.cpp

@@ -60,8 +63,8 @@ TrackLabelButton::TrackLabelButton( TrackView * _tv, QWidget * _parent ) :
 	else
 	{
 		setFixedSize( 160, 29 );
-		m_renameLineEdit->move( 30, ( height() / 2 ) - ( m_renameLineEdit->sizeHint().height() / 2 ) );
-		m_renameLineEdit->setFixedWidth( width() - 33 );
+		m_renameLineEdit->move(25, ( height() / 2  - m_renameLineEdit->sizeHint().height() / 2 ) + 1);


Just thought of this now, is it possible to use layouts to dynamically place the line edit widget instead of calling move() explicitly? Sorry for the nitpicks, have to do this.

Is there an example of using layouts in the code base? So I can see, how it works?

I believe the MixerChannelView now uses layout to handle something similar.

I will have a look. I need to build for Windows and Mac, too. So I can see if it looks all the same on all systems.

@BaraMGB, if you search for QVBoxLayout, QHBoxLayout or QBoxLayout you should find several places in the code. Another type of layout is QGridLayout.

I think it might even make sense to add a new widget for the label/line edit combination which implements the switching.

Actually, I don't see the benefits of your idea. The current implementation is clean and works perfect. Actually this is only a simple bug fix PR. I don't see, why we should it make more complicated as it is. If someone wants to make the width of the track header dynamic one day, he could do it. But in my opinion, this goes beyond the purpose of this pull request.

I believe TrackContentWidget already uses a QHBoxLayout and calling move() explicitly might have different results depending on the host system.

Actually, I don't see the benefits of your idea. The current implementation is clean and works perfect. Actually this is only a simple bug fix PR. I don't see, why we should it make more complicated as it is. If someone wants to make the width of the track header dynamic one day, he could do it. But in my opinion, this goes beyond the purpose of this pull request.

I think it might be a bit more stable solution than the current one as can be seen by this issue. I agree that it is not in the scope of this PR though. It was just meant as some food for thought.

@michaelgregorius I guess, the best option for this, is to use the QLineEdit itself. There should not need a stacked widget, because the QLineEdit has got 2 modes, a label mode and an edit mode. I try to push a new commit in the next days. Thank you for your thoughts.

[...] the QLineEdit has got 2 modes, a label mode and an edit mode.

That sounds even better! 😃

src/gui/tracks/TrackLabelButton.cpp

Make sure that the rename line edit of the `TrackLabelButton` has the same font size as the widget itself. Because the font size is defined via style sheets the font size of the line edit has to be set when the actual renaming starts and not in the constructor. The reason is that style sheets are set after the constructor has run. Rename the local variable `txt` to the more speaking name `trackName`. Ensure that the line edit is moved to the correct place for different icon sizes by taking the icon size into account. To make this work this also has to be done in the `rename` method.

Streamline the default style sheet of `TrackLabelButton` by removing repeated properties that are inherited from the "main" style sheet, i.e. that are already part of `lmms--gui--TrackLabelButton`. Interestingly, the `background-color` property had to be repeated for `lmms--gui--TrackLabelButton:hover`.

michaelgregorius

I have added some additional improvements with commit c60b5fe and 2278f7a. For me this is good to go now as it fixes the problem and "perfect is the enemy of the good".

michaelgregorius · 2024-10-07T11:26:12Z

I have merged this pull request now as it solves the problem. Any potential further improvements can be done in separate pull requests.

Align the rename line edit for tracks. Make sure that the rename line edit of the `TrackLabelButton` has the same font size as the widget itself. Because the font size is defined via style sheets the font size of the line edit has to be set when the actual renaming starts and not in the constructor. The reason is that style sheets are set after the constructor has run. Rename the local variable `txt` to the more speaking name `trackName`. Ensure that the line edit is moved to the correct place for different icon sizes by taking the icon size into account. To make this work this also has to be done in the `rename` method. ## Other changes Streamline the default style sheet of `TrackLabelButton` by removing repeated properties that are inherited from the "main" style sheet, i.e. that are already part of `lmms--gui--TrackLabelButton`. Interestingly, the `background-color` property had to be repeated for `lmms--gui--TrackLabelButton:hover`. --------- Co-authored-by: Michael Gregorius <[email protected]>

fix: LMMS#7413: Renaming track visual glitch

3e898df

revert hide approach / correct alignment and font size

d679f2b

Rossmaxx reviewed Aug 2, 2024

View reviewed changes

src/gui/tracks/TrackLabelButton.cpp Outdated Show resolved Hide resolved

src/gui/tracks/TrackLabelButton.cpp Outdated Show resolved Hide resolved

src/gui/tracks/TrackLabelButton.cpp Outdated Show resolved Hide resolved

BaraMGB added 2 commits August 2, 2024 12:59

style fixes

b2f5f5e

fix horizontal alignment

5ee84d8

Rossmaxx reviewed Aug 2, 2024

View reviewed changes

sakertooth linked an issue Aug 5, 2024 that may be closed by this pull request

Renaming track visual glitch #7413

Closed

1 task

sakertooth changed the title ~~fix: #7413: Renaming track visual glitch~~ Align the rename box for tracks Aug 5, 2024

sakertooth reviewed Aug 5, 2024

View reviewed changes

src/gui/tracks/TrackLabelButton.cpp Outdated Show resolved Hide resolved

michaelgregorius added 2 commits October 7, 2024 11:10

michaelgregorius approved these changes Oct 7, 2024

View reviewed changes

michaelgregorius merged commit 066f6b5 into LMMS:master Oct 7, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align the rename box for tracks #7414

Align the rename box for tracks #7414

BaraMGB commented Aug 1, 2024

Rossmaxx commented Aug 1, 2024

Rossmaxx commented Aug 1, 2024

sakertooth commented Aug 1, 2024

Rossmaxx commented Aug 1, 2024

BaraMGB commented Aug 1, 2024

michaelgregorius commented Aug 1, 2024

BaraMGB commented Aug 2, 2024

Rossmaxx commented Aug 2, 2024

Rossmaxx left a comment

BaraMGB commented Aug 2, 2024

Rossmaxx Aug 2, 2024

BaraMGB Aug 2, 2024

Rossmaxx Aug 2, 2024

BaraMGB Aug 2, 2024

michaelgregorius Aug 2, 2024

BaraMGB Aug 3, 2024

Rossmaxx Aug 3, 2024

michaelgregorius Aug 3, 2024

BaraMGB Aug 8, 2024

michaelgregorius Aug 8, 2024

michaelgregorius left a comment

michaelgregorius commented Oct 7, 2024

Align the rename box for tracks #7414

Align the rename box for tracks #7414

Conversation

BaraMGB commented Aug 1, 2024

Rossmaxx commented Aug 1, 2024

Rossmaxx commented Aug 1, 2024

sakertooth commented Aug 1, 2024

Rossmaxx commented Aug 1, 2024

BaraMGB commented Aug 1, 2024

michaelgregorius commented Aug 1, 2024

BaraMGB commented Aug 2, 2024

Rossmaxx commented Aug 2, 2024

Rossmaxx left a comment

Choose a reason for hiding this comment

BaraMGB commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgregorius left a comment

Choose a reason for hiding this comment

michaelgregorius commented Oct 7, 2024