eakwarren commented 2 weeks ago

17100 Don't inherit mixer height restriction

Resolves: #17100

Don't inherit height restriction in Timeline panel

[x] I signed the CLA
[x] The title of the PR describes the problem it addresses
[x] Each commit's message describes its purpose and effects, and references the issue it resolves
[x] If changes are extensive, there is a sequence of easily reviewable commits
[x] The code in the PR follows the coding rules
[x] There are no unnecessary changes
[x] The code compiles and runs on my machine, preferably after each commit individually
[x] I created a unit test or vtest to verify the changes I made (if applicable)

cbjeukendrup commented 2 weeks ago

@eakwarren Let's just remove the line, instead of commenting it out. Also, I think no comment is necessary about why there is no maximum height, because I think that's quite common.

And perhaps we should make the same change to all horizontal panels; I don't see why any would need a maximum height.

cbjeukendrup commented 2 weeks ago

And, sorry, one more thing: if you could make the commit message more descriptive than "Update NotationPage.qml" that would be great (perhaps, just make "https://github.com/musescore/MuseScore/issues/17100 Don't inherit mixer height restriction" the title of the commit, that would be good)

eakwarren commented 2 weeks ago

Sorry for the branch deletes and restores. I thought I should rename the branch since now it's more than just changes to the timeline panel, but then read Update the PR and will now squash and amend with a force push. Waiting on further input from @cbjeukendrup in discord #development question

Thanks for your patience as I learn git.

eakwarren commented 1 week ago

Consolidating discussion on Discord for convenience.

Eric — 11/02/2024 5:27 PM @ Casper Jeukendrup Re: your comment about changing all the horiz. panels max heights: The drumsetPanel doesn't set maximumHeight with root.horizontalPanelMaxHeight like the others. It's just a thin strip in the interface and is similar to the Navigator where they can't un-dock. I'm thinking leave the drumsetPanel as is with min/max: 64.

I'm unable to bring up the percussionPanel in MSS. Drumkits and single pitched/unpitched perc. instruments all show the same drumset panel when in note entry mode. Is it deprecated?

Casper Jeukendrup — 11/02/2024 6:30 PM The drumset panel is a bit of a special case. It might be best to leave that untouched, with its 64 height.

The percussionPanel is still under construction, and can be revealed by enabling some setting in “DevTools > Settings” (sorry, I don’t remember the name right now). But anyway, I think you can give that the same treatment as the other horizontal panels.

Marc Sabatella — 11/02/2024 7:13 PM I hadn't been able to find anything at first because I wasn't sure where to be looking, but armed with this knowledge, I found it. Second to last option in the list, ui/useNewPercussionPanel

Eric — 11/03/2024 3:24 PM Hmmm. After syncing my fork with master, removing the maximumHeight from the mixer panel causes a weird height issue with the mixer panel in the PR artifact build.

Restoring maximumHeight: root.horizontalPanelMaxHeight for only the mixer panel seems to fix this on a local build. (Score titles in attached videos reflect PR vs. local build behavior.)

Could recent commits (or other commits) in master be conflicting and causing the mixer panel issue? When I tested prior to syncing my fork and updating the PR, it seemed fine (mixer height could be dragged all the way up).

https://github.com/user-attachments/assets/e71716e4-d73a-4287-9933-26d67f93ebc0

https://github.com/user-attachments/assets/e0550fd7-ffa2-4b44-9e68-97d6d9663dc7

Casper Jeukendrup — 11/04/2024 11:50 AM Hm, that's mysterious. It's unlikely that it's caused by the commit you linked, because that doesn't touch any sizing code. It would be helpful if you could find out at which commit it was working. To find out, check out the master branch, then run for example git checkout HEAD~100 to go 100 commits back in time, and then make the changes (without committing them). Do you still know when approximately you created the branch, i.e. how many commits back?

Eric — 11/04/2024 12:48 PM I cloned on 10/31 @4:49pm PDT. Looks like I only need to go back ~10 commits. I'll test and report back.

Eric — 11/04/2024 3:11 PM @ Casper Jeukendrup I checked out 21d267a which was the last commit prior to my dev setup. I think I know why I thought it worked. I viewed the Timeline panel first which resized freely. Piano and the new Percussion panel as well. However, once I added the Mixer panel (which was larger than its maxHeight because I was first testing the Timeline) it appeared in the same larger space as the other panels. Now, trying to drag the size, the mixer can only get smaller, and switching to the other docked panels restricts their height as well. And the smaller the mixer gets, that's now its max height. (see video below)

At any rate, adding maximumHeight: root.horizontalPanelMaxHeight back to the mixer restores its functionality, but again locks the size of the other panels. No biggie if they are docked separately above notation or floating. Examining the DockPanel code, the only difference I see between the various panels is the mixer adds

                onResizeRequested: function(newWidth, newHeight) {
                    mixerPanel.resize(newWidth, newHeight)
                }

which may contribute the odd behavior. Thoughts?

https://github.com/user-attachments/assets/9e78a5cb-dcab-49fa-9dc5-aeede9a7d010

Eric — 11/04/2024 3:25 PM Just rebuilt without onResizeRequested (and removed maximumHeight again) and everything works as expected. Is onResizeRequested really needed anymore?

Eric — 11/04/2024 3:39 PM And is readonly property int horizontalPanelMaxHeight: 520 needed, since it's only declared but not called?

Casper Jeukendrup — 11/04/2024 5:57 PM Good research! Yes, that's still necessary, so that the mixer panel automatically expands when showing/hiding controls via its '...' menu or when applying more audio plugins. But it seems like resizeRequested in MixerPanel is emitted also while dragging, which is unexpected. You could experiment with height and implicitHeight properties inside MixerPanel.qml; perhaps there is some combination that ensures that the signal is not emitted while the panel is being resized manually. For example I wonder what happens when you set the implicitHeight of flickable to contentColumn.implicitHeight instead of contentColumn.height.

Eric — 11/04/2024 6:56 PM Tried implicitHeight. No difference in the build I can see. Mixer still can only resize down and gets stuck there.

One interesting thing I notice about onResizeRequested, when the mixer is shown it defaults to a height slightly taller than what it allows after a drag-n-resize. (This may be due to showing all the mixer elements - adding Volume, Aux send 2, and all 4 Audio FX.) In the video, note the location of the Mixer title in relation to the Github icon, and then after a resize. Then close and re-open and it's back to the taller size. Could maximumHeight be superseded by onResizeRequested?

I tried removing readonly on property int horizontalPanelMaxHeight: 520 and setting maxHeight to newHeight, but got the same behavior shown in the video.

                onResizeRequested: function(newWidth, newHeight) {
                    mixerPanel.resize(newWidth, newHeight)
                    maximumHeight = newHeight
                }

https://github.com/user-attachments/assets/a4e74022-a001-433c-a270-bdb2f1091154

Casper Jeukendrup — Yesterday at 5:58 AM I think what happens in the video is that when reopening the mixer, it goes back to the height that fits all contents, because of onResizeRequested. See also the implementation of DockBase::resize; indeed that messes with maximum height and width. It looks like that shouldn't cause problems, because a backup is created and restored, but perhaps there's still something problematic there.

Eric — Yesterday at 9:27 AM I didn't see anything obvious in DockBase:resize. I tried removing the two applySizeConstraints(); in various combinations, without success. The most functional setup I've found is horizontalPanelMaxHeight: 550 (which also addresses #24659 ) and leave maximumHeight: root.horizontalPanelMaxHeight in the mixerPanel only.

eakwarren commented 1 week ago

@cbjeukendrup Would you like me to update the PR with horizontalPanelMaxHeight: 550 (which also addresses https://github.com/musescore/MuseScore/issues/24659) and leave maximumHeight: root.horizontalPanelMaxHeight in the mixerPanel only?

cbjeukendrup commented 1 week ago

Maybe we should leave this as it is for now, and mark it as a draft. @Eism is currently working on some changes to the docking framework, necessitated by the plans to update to Qt 6.8. This may also fix the problems that are blocking this PR. If that doesn't help either, then we can consider the workaround you mention, but let's await that first.

musescore / MuseScore

Remove height restriction in horizontal panels #25398

17100 Don't inherit mixer height restriction