Skip to content

Conversation

@rubiefawn
Copy link
Contributor

@rubiefawn rubiefawn commented Mar 22, 2025

Resolves #7798.

The culprit was the little text warning under the instrument tuning view informing the user that MIDI-based instruments do not support the microtuner. The entire instrument window was expanding horizontally to accommodate this. The "fixed size" of the instrument itself has no bearing on the other tabs in the instrument window, it seems.

This PR solves this by setting the width of the other tabs to be no wider than the instrument they belong to.

Recording 2025-11-04 at 22 26 31

@rubiefawn rubiefawn added this to the 1.3 milestone Mar 22, 2025
@JohannesLorenz JohannesLorenz self-requested a review March 22, 2025 16:32
@JohannesLorenz
Copy link
Contributor

I tested the PR and it works and the code is correct.

but there are no instruments that are or can be resized to larger than 250px that are MIDI-based

That's right for now, but with #7201 going to master (hopefully soon), many Lv2 instruments can be resized larger than 250px - if we allow Lv2 UI to be displayed inside the InstrumentTrackWindow (vs in a new popup window).

If "instrument windows [...] have their width determined by the instrument itself" (quoting you from Discord, btw this is the same solution I proposed in #7524 which I withdrew in favor of #3532), this PR might be restricting the size unwanted.

@regulus79
Copy link
Member

WHOAH YOU ACTUALLY FOUND IT?! Congratulations, I could not figure it out!!! 🎉
I briefly tested it, and it's definitely fixed!
image

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

This PR solves this problem the same way it was solved in d447cb0: the text has received a fixed maximum width. This is a bit of a hack, but there are no instruments that are or can be resized to larger than 250px that are MIDI-based. A cleaner possible solution would be to refactor things so the other tabs have their width determined by the instrument size, but this works for now.

Maybe we can explain some of this in a TODO comment, but LGTM overall

@rubiefawn
Copy link
Contributor Author

rubiefawn commented Mar 22, 2025

WHOAH YOU ACTUALLY FOUND IT?! Congratulations, I could not figure it out!!! 🎉 I briefly tested it, and it's definitely fixed!

Hmmm, in the images you shared I am still seeing ~3px of extra space on the right that shouldn't be there. It's much less noticeable than the previous gap, but it looks like the problem isn't fully solved. It may be that the problem is better or worse for any given user depending on the system UI font, which explains why I don't see that gap on my machine. I'll take a closer look.

Edit: Sanity check, I cannot replicate this on 1.2.2 stable, nor the current 1.3.0-alpha, so whatever is causing it happened since then.

@he29-net
Copy link
Contributor

Just for more information, this is how it normally looks like:

I.e. the text is supposed to simply wrap around automatically, without forcing the parent to grow wider. From the screen shots I can see that it is the classic Qt issue that I've been running into for years: everything is scaled up by some factor, say 1.5x if you have fractional scaling enabled, and then, some text is scaled up by 1.5x again. You can also see it in your VeSTige screenshot: the "Show/Hide GUI" and "Turn off all notes" button labels are larger than they are supposed to be. For comparison:

AFAIK this affects all standard Qt widgets (buttons, labels, ..). LMMS fixed-size widgets seem to be unaffected (possibly because they have fixed font sizes specified in pixels?) Not sure how to fix that "properly" for all the widgets, or if it is even possible. Qt DPI scaling support is just.. fun...

@bratpeki
Copy link
Member

bratpeki commented Jun 4, 2025

I cannot replicate this on 1.2.2 stable, nor the current 1.3.0-alpha, so whatever is causing it happened since then.

@bratpeki
Copy link
Member

bratpeki commented Jun 4, 2025

I can confirm there's still a few pixels to the right, but the fact you found this is awesome! 👍

@bratpeki bratpeki self-assigned this Jun 4, 2025
@JohannesLorenz JohannesLorenz removed their request for review June 4, 2025 20:05
@regulus79
Copy link
Member

I don't know if something changed, but ZynAddSubFX isn't as wide as it was anymore on master, despite this PR not having been merged yet.

@rubiefawn rubiefawn changed the title Fix microtuning warning from widening instruments Temporary fix for microtuning warning text widening instruments Oct 20, 2025
@rubiefawn rubiefawn added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Oct 21, 2025
@rubiefawn rubiefawn force-pushed the fix/wide-instruments branch from 4d84a0a to 4cc9acc Compare October 24, 2025 22:49
@rubiefawn rubiefawn changed the title Temporary fix for microtuning warning text widening instruments Fix microtuning warning text widening instruments Nov 4, 2025
@rubiefawn
Copy link
Contributor Author

I've updated this so it no longer uses the evil hack. Now the other tabs actually have their width restricted by the instrument tab.

@rubiefawn
Copy link
Contributor Author

rubiefawn commented Nov 5, 2025

I've also made some more changes, so this resolves #7876 as well now.

Edit: #8096 happens to resolve #7876, so I'll revert my changes here and let that other PR take care of it. Issues with vertical tiling will once again occur on this branch when dropping certain instruments onto an instance of SlicerT.

The detach window feature happens to fix this so I'll leave it alone lol
@bratpeki
Copy link
Member

bratpeki commented Nov 13, 2025

  • Testing (ZynAddSubFX)
  • Code Review

2025-11-13-170446_1240x1019_scrot

Confirmed on closer inspection that this does indeed fix it.

The code also looks fine. It takes care of the resizable instruments like SlicerT, removes the old erronous behavior and applies proper changes to all tabs for future changes to any one of them.

@bratpeki
Copy link
Member

@sakertooth Can we merge?

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

Code looks fine to me, free to merge if the PR has already been tested.

@rubiefawn rubiefawn merged commit dc2a461 into LMMS:master Nov 13, 2025
11 checks passed
@rubiefawn rubiefawn deleted the fix/wide-instruments branch November 13, 2025 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug gui needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instrument window width is incorrect, shows unintentional background tiling

6 participants