Skip to content

Conversation

@szeli1
Copy link
Contributor

@szeli1 szeli1 commented Mar 3, 2025

closes #7751

@bratpeki bratpeki self-assigned this Mar 3, 2025
@bratpeki
Copy link
Member

bratpeki commented Mar 3, 2025

That's it! Someone review this, please.

@bratpeki bratpeki requested a review from tresf March 3, 2025 22:07
@tresf
Copy link
Member

tresf commented Mar 3, 2025

That's it! Someone review this, please.

@bratpeki I see you've marked me for approval however I've never seen this specific bug before. I read #7751 but I don't see steps to reproduce.

@bratpeki
Copy link
Member

bratpeki commented Mar 4, 2025

I didn't specify it here, my bad! The steps to reproduce are given in the Google Drive video in the related issue, but I'll write them out here as well.

  1. Open any LMMS subwindow in fullscreen (Piano roll, Mixer, Notepad...)
  2. Close it
  3. Move around in the workspace
  4. Open that same subwindow again
  5. The subwindow is no longer centered

@messmerd
Copy link
Member

This partially fixes the issue, but it doesn't move a maximized window's normal position, so when you un-maximize a window, the normal position of the window is in the wrong place.

@bratpeki
Copy link
Member

@messmerd explained in more detail how to recreate his bug. Results below:

bug.mp4

@regulus79 can also replicate the issue.

@tresf
Copy link
Member

tresf commented Mar 11, 2025

I didn't specify it here, my bad! The steps to reproduce are given in the Google Drive video in the related issue, but I'll write them out here as well.

  1. Open any LMMS subwindow in fullscreen (Piano roll, Mixer, Notepad...)
  2. Close it
  3. Move around in the workspace
  4. Open that same subwindow again
  5. The subwindow is no longer centered

Thanks however I can't for the life of me reproduce this. I'm testing https://lmms.io/download/pull-request/7595 on macOS, so either I'm testing wrong or the issue does not impact macOS.

@szeli1
Copy link
Contributor Author

szeli1 commented Mar 11, 2025

This partially fixes the issue, but it doesn't move a maximized window's normal position, so when you un-maximize a window, the normal position of the window is in the wrong place.

This can not be fixed. The subwindows use setWindowState() which loads back normalGeometry that is private as far as I see.

@szeli1
Copy link
Contributor Author

szeli1 commented Mar 11, 2025

A commit was made that moves hidden windows also.

@bratpeki
Copy link
Member

The subwindows use setWindowState() which loads back normalGeometry that is private as far as I see.

I think it was @Rossmaxx that told me we can increase the visibility of things if it's necessary.

@Rossmaxx
Copy link
Contributor

Not me, might be @regulus79

@tresf
Copy link
Member

tresf commented Mar 13, 2025

Thanks however I can't for the life of me reproduce this. I'm testing https://lmms.io/download/pull-request/7595 on macOS, so either I'm testing wrong or the issue does not impact macOS.

I'm comparing this branch to master and what I find is that the size of the window was not preserved. Is that part of the fix? The title of this PR says "subwindows not centered" which I'm not sure what this means.

For example, with this PR, if I drag the canvas around and then open a new piano roll editor, it can render way off the screen. If this is what's desired, we need some way of informing a user that all new windows will be opened relative to a central point on the canvas.

  1. New project
  2. Drag everything off the screen
  3. Move the song-editor back to the top left corner
  4. Create an empty piano roll segment
  5. Double click this segment
  6. Piano roll renders way off of the screen

Now repeat this, but instead open the plugin interface for TripleOscillator, it renders on the visible canvas, not off the screen.

@messmerd
Copy link
Member

@szeli1 The issue I found isn't too serious, so I wouldn't want it to hold up merging this PR if there's really no easy way to fix it. Maybe it'd be good to leave a comment about it though.

@szeli1
Copy link
Contributor Author

szeli1 commented Mar 14, 2025

@szeli1 The issue I found isn't too serious, so I wouldn't want it to hold up merging this PR if there's really no easy way to fix it. Maybe it'd be good to leave a comment about it though.

Where would you leave a comment?

@messmerd
Copy link
Member

@szeli1 Just before the for loop you added, I guess. Unless you think there's a better place. The idea is to just document what might be surprising behavior and why it can't really be fixed as far as we know.

@szeli1
Copy link
Contributor Author

szeli1 commented Mar 17, 2025

I'm comparing this branch to master and what I find is that the size of the window was not preserved. Is that part of the fix? The title of this PR says "subwindows not centered" which I'm not sure what this means.

What do you mean by this?
Sorry for the unclear description It supposed to fix the mentioned issue. Maybe @bratpeki could edit their issue too to better reflect the problem.

If this is what's desired, we need some way of informing a user that all new windows will be opened relative to a central point on the canvas.

It is intended, what if the user hides something and then moves and reopens it, I think it is better for it to reopen at the old location. The issue you were experiencing with TripleOscillator is because the window is created when the user opens the instrument, so QT handles the placement and all past movement doesn't apply to it.

@tresf
Copy link
Member

tresf commented Mar 17, 2025

I think it is better for it to reopen at the old location.

I do not agree, but if others agree, I won't press to hard on this.

The issue you were experiencing with TripleOscillator is because the window is created when the user opens the instrument, so QT handles the placement and all past movement doesn't apply to it.

Assuming this is desired, I think there should be some type of indication that something was rendered off-screen. The whole MDI canvas is very bad user experience (yet we're stuck with it for now) and clicking a button that reveals a window should reveal that window. Perhaps we move the viewport to bring it to focus?

@tresf
Copy link
Member

tresf commented Mar 17, 2025

I'm comparing this branch to master and what I find is that the size of the window was not preserved. Is that part of the fix?

What do you mean by this?

When showing or hiding a window on master branch, the size is lost.

  • Fullscreen the song editor
  • Hide the song editor
  • Move the canvas a lot
  • Show the song editor

Now that I explain this to a rubber duck, I can see this is the exact issue. The full screen is the correct size, but it's not "centered". Approving lol.

@tresf
Copy link
Member

tresf commented Mar 17, 2025

Assuming this is desired, I think there should be some type of indication that something was rendered off-screen. The whole MDI canvas is very bad user experience (yet we're stuck with it for now) and clicking a button that reveals a window should reveal that window. Perhaps we move the viewport to bring it to focus?

Opened a dedicated bug report for this: #7790

@tresf tresf merged commit f72ae32 into LMMS:master Mar 17, 2025
10 checks passed
sakertooth pushed a commit to sakertooth/lmms that referenced this pull request Jun 1, 2025
LMMS#7752)

Fix bug introduced with LMMS#7595 where drag-moving the MDI canvas background would break the show/hide behavior of a fullscreen window.

Closes LMMS#7751
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.

[Bug] Fullscreened subwindows not centered after #7595

5 participants