Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Center PianoView in resizeable plugins (SlicerT) #7731

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

regulus79
Copy link
Contributor

@regulus79 regulus79 commented Feb 26, 2025

Related: #7513

Now that the instrument window for SlicerT is resizeable, one of the many issues is that the piano at the bottom of the window is not centered when you resize the window to be larger than the piano width. This PR attempts to fix that.

Before:

image

After:

image

Simply setting the alignment of the piano view to Qt::AlignHCenter did not work, since that broke the automatic resizing. However, putting the piano view into a QHBoxLayout and putting that into the instrument window worked nicely.

However, I would like to get someone's opinion on whether I am managing the layout pointer correctly. Passing this as the parent for the layout caused warnings, since that automatically tries to set the layout as the default layout for the parent. Should I leave it as is and add an explicit call to setParent(this) to make sure this is responsible for deleting the layout?

@tresf
Copy link
Member

tresf commented Feb 27, 2025

I think this is a sane and worthwhile change (tested and works).

By having SlicerT being a resizable plugin it introduced a lot of bugs and potential features to the software... some adjacent to this PR and some are just bugs that were never ironed out when resizable plugin windows were introduced. Some thoughts here...

  1. Ideally, it may be cosmetically beneficial to extend some part of the whitespace on the sides of this piano roll into something that fills the width to indicate that this is a deliberate design decision.

  2. When the piano roll is full-width, the horizontal scrollbar disappears, but the space it occupies is still there.

  3. By centering the piano roll, it now causes the left-align controls of SlicerT to appear as if they should be centered too.

  4. Just like the whitespace for the piano, a similar, deliberate design decision should be made to fill this area with something that matches the theme of the instrument.

    image
  5. Unrelated to this PR, but brings it to light... the envelope screen does not size properly (I'm sure there's bug out there for this)

    image

Side note... I appreciate the screenshots. Moving forward, I would advise using the default theme for screenshots if it's not terribly inconvenient, it helps reviewers focus on the baseline impact. In this case, your theme looks strikingly similar to SlicerT's theme, leaving some ambiguity in regards to how it'll look.

@@ -264,9 +264,11 @@ InstrumentTrackWindow::InstrumentTrackWindow( InstrumentTrackView * _itv ) :
m_tabWidget->addTab(m_tuningView, tr("Tuning and transposition"), "tuning_tab", 5);

// setup piano-widget
auto pianoLayout = new QHBoxLayout();
m_pianoView = new PianoView( this );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@regulus79 wrote:

I would like to get someone's opinion on whether I am managing the layout pointer correctly. Passing this as the parent for the layout caused warnings, since that automatically tries to set the layout as the default layout for the parent. Should I leave it as is and add an explicit call to setParent(this) to make sure this is responsible for deleting the layout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried calling setParent but that resulted in more warnings, so I'm not sure how to let qt know that it needs to be deleted. Unless it automatically deletes sub-layouts?

@RoxasKH
Copy link
Contributor

RoxasKH commented Feb 27, 2025

4. Just like the whitespace for the piano, a similar, deliberate design decision should be made to fill this area with something that matches the theme of the instrument.

About 4, the decision when i made it was to keep the background of the right portion, as it's pretty common for resizable instruments that keep graphs boxed to have empty space in between.
As it needs to be procedurally drawn, it can only be a gradient or a fill color. Unless someone manages to make an svg of the background, but either way, it felt like too much to keep the same bg and expand it to me, as it'd also unbalance the simmetry of the elements in their box (unless extended fully to the whole bottom portion, which was the original design).
Just like a 3rd different approach for just the inbetween space would stress it even more imo, and make it feel like a filler more than the plugin expanding.
This doesn't mean the current approach is perfect, that's just my opinion. Would like to know others' opinions.

Original design sketch for reference

Example, FL Limiter expanded, maybe slightly different cause of the shading, but same situation

immagine

@tresf
Copy link
Member

tresf commented Feb 28, 2025

Linking #7513. Milestoning for 1.3.

@tresf tresf added this to the 1.3 milestone Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants