mpc-hc
mpc-hc copied to clipboard
clsid2
Incremental zoom from center issues
The #1626 issue and #2710 PR changed the behaviour of the incremental zoom feature. Some resulting regressions from this change:
- ~~When using a multi-monitor setup, this feature is no longer able to increase window size to larger than the current monitor's viewport. The window cannot overflow onto a second or third monitor and it cannot overlap the start menu, even if the window is always-on-top.~~ DONE IN #2743
- ~~It is generally hard to zoom a video in more than what is displayable now, one has to corner drag, and move. While this is a rare use-case, I imagine it may still be there sometimes.~~ DONE IN #2743
- When zooming the window that isn't attached to the top-left corner, there is image vibration/flickering caused by a separate move-and-rerendering event.
- The "Limit window proportions on resize" feature no longer has an effect on this function, it always always limits the window proportions on resize.
- ~~When viewing a video that doesn't fit the viewport exactly, and incrementing past the fully fitted window size, an additional black border is shown. For example, if the window is longer than the monitor space, then black borders are added to the top-and-bottom.~~ DONE IN #2743
After merging #2743, the following regressions are present (FYI, @adipose ):
- When the start menu is displayed on the bottom of the screen, when snapping the window against the bottom start menu, the zoom function grows the window on the bottom-right. Expected: the window is zoomed towards the monitor centre, until the current monitor is filled.
- On a multi-monitor setup, the window is on the left monitor, when snapping the window to the right edge of the monitor, the zoom function grows the window on the bottom-right. Expected: the window is zoomed towards the monitor centre, until the current monitor is filled.
Note for the future: Initial versions of the #1515 PR contained a screen edge snapping logic on resize. It may be reused in case of a potential update.
Good feedback. Let's keep it on the PR. If we can't merge an acceptable PR then we will leave it as-is.
Need to define desired behavior for incremental zoom when already snapped. My thinking is it should honor snapped edges as long as room exists. Beyond that I don't know but it becomes complex, and different monitor layouts could be quite messy.
uuuhh.... @NBruderman, @clsid2 why close this? I opened this issue just now to describe the problems that still exist after the integration of #2710 and #2736 aka. #1515 .
@Sp3EdeR after the latest patch, what issues are still not fixed from the list above?
@Sp3EdeR I also want to know what isn't fixed.
Could you use 'strikethrough' on whats fixed.
Crossed out resolved issues, raised new issues after testing the merged PR #2743 . I think the issue is that this condition may need to be modified to
work.PtInRect(rect.BottomRight() - CPoint(1, 1)), to account for PtInRect open interval on bottom/right. (Did not test the theory.)
If the point overlaps the rect border, it is considered "not in."
It first hits full screen constraints. Next it expands, in testing.
- The "Limit window proportions on resize" feature no longer has an effect on this function, it always always limits the window proportions on resize.
If we are zooming to scale, why should it use any proportion other than that of the video? Is there a purpose to growing "to scale" while leaving unnecessary black bars? Should it take the larger of the "zoom" dimensions and the existing dimensions?
I can make it honor existing vertical dead space, which has a less surprising result when you zoom. But horizontal dead space is a different matter, since it calculates the zoom by the video window and not the video size itself.
- The "Limit window proportions on resize" feature no longer has an effect on this function, it always always limits the window proportions on resize.
If we are zooming to scale, why should it use any proportion other than that of the video? Is there a purpose to growing "to scale" while leaving unnecessary black bars? Should it take the larger of the "zoom" dimensions and the existing dimensions?
I just highlighted any changes and potential deviancies from requirement. If you guys deem that this should not be handled, then I cross it off the list. I just wanted to make sure that the software requirements are defined within this issue ticket before I would contribute any code, since my previous contribution that spawned this thread waited over two years. :)
More of a discussion.
I think a similar fix to the other would work. If the window is already proportional to the video, zoom constrained. Otherwise, zoom bottom right.
@adipose, I have tested the behaviour of the latest patch and added issues I've found with it into that PR.
May I suggest discussing the desired behavioural requirements before further patchsets? Perhaps, once the requirements are specified, a test set could be derived, and maybe I could even help with the implementation (given I have enough time on my hands).
added issues I've found with it into that PR.
That patch is closed so perhaps it is best to update this thread until we have resolved all issues.
I think it works more reasonably now than it did. I haven't checked the auto-hide issue yet.
@adipose, I have tested the behaviour of the latest patch and added issues I've found with it into that PR.
May I suggest discussing the desired behavioural requirements before further patchsets? Perhaps, once the requirements are specified, a test set could be derived, and maybe I could even help with the implementation (given I have enough time on my hands).
If you want to write up a proposal of what you think should change, I'm happy to review and discuss with @clsid2 . I believe that "Limit window proportions on resize" should be honored now. Can you confirm?
For the current iteration of this feature (2.2.1.25):
- I think the following should definitely be changed:
- The 1% of monitor dimention should be changed to
max(moni.width(), moni.height) / 50(2% instead of 1% for easier mousewheel usage, currently it requires furious rotation of the wheel for significant size changes) - When snapping the window to the right or bottom side of the monitor, the resizing logic should still use the centered growing logic instead of the current bottom-right one. Also because this fails the #1626 issue. It should only switch to bottom-right mode after the window has the same width/height as the entire monitor or when overflowing the monitor boundary.
- When opening a video, then closing the video, then zooming the window out, the window is "gone". For me, it goes to rectangle (964,32767)-(1388,32806).
- The 1% of monitor dimention should be changed to
- I think the following should be considered:
- When auto-hiding of panels is turned on, the area of the panels should not be factored into the resize logic when visible.
- When window snapping is turned on, and the window is snapped to one or two edges of the monitor, zooming the window out should keep the window position snapped to those edges.
- Brought the following over from the PR. It's up for consideration, whatever you guys feel like doing. For me the zoom functionality was more a window resizer than a video zoomer. For @adipose, it is more a video zoomer where the window size follows that. That's okay for me. But I will still leave it in the list for consideration and test check:
When zooming with fLimitWindowProportions == false, on the main monitor, resizing the window to not fit the video, only the window width is being changed for me. The height stays the same. When zooming without fLimitWindowProportions, the window height should be increased/decreased with zoom-step * window-height / window-width
https://github.com/clsid2/mpc-hc/pull/2797
For the current iteration of this feature (2.2.1.25):
* I think the following should definitely be changed: * The 1% of monitor dimention should be changed to `max(moni.width(), moni.height) / 50` (2% instead of 1% for easier mousewheel usage, currently it requires furious rotation of the wheel for significant size changes)
Updated.
* When snapping the window to the right or bottom side of the monitor, the resizing logic should still use the centered growing logic instead of the current bottom-right one.
It now is considered on the monitor when it touches the bottom or right edges.
Also because this fails the The Zoom in/out hotkeys should take into account the window position (just like the other zoom hotkeys) #1626 issue. It should only switch to bottom-right mode after the window has the same width/height as the entire monitor or when overflowing the monitor boundary.
I changed it to use bottom right when zooming fails to grow on the current monitor. This should solve this issue, I think.
* When opening a video, then closing the video, then zooming the window out, the window is "gone". For me, it goes to rectangle (964,32767)-(1388,32806).
I don't understand the steps you are describing here.
I'll look at the other issues soon.
When auto-hiding of panels is turned on, the area of the panels should not be factored into the resize logic when visible.
It already ignores it, because it just checks the video window size, which doesn't include any hovering panels.
When window snapping is turned on, and the window is snapped to one or two edges of the monitor, zooming the window out should keep the window position snapped to those edges.
With the new code this should work automatically, because it tries not to grow outside the monitor unless it's out of space.
Tested #2797:
- The resize percentage increase is much better!
- The right/bottom docked resizing is great now!
- Autohidden interface elements still influence the window growth when shown vs. hidden.
- The following issue is no longer reproducible; so fixed:
When opening a video, then closing the video, then zooming the window out, the window is "gone".
- When the window is docked to an edge, zooming out keeps the window docked indeed, so this is great!
Overall, I think the behaviour is very good now. I've updated the issue description with the current state. The issue can be considered closable, the remaining issue is not severe in my opinion.
Autohidden interface elements still influence the window growth when shown vs. hidden.
Can you please demonstrate this with some specifics:
- Starting window size / ending window size with no toolbars
- Starting window size / ending window size with autohide toolbars
As an example with frameless view (measured with Spy++):
- Interface hidden:
- Starting position: (133, 126)-(837, 505), 704x379
- Zoom out position: (152, 136)-(818, 494), 666x358
- Seekbar, controls, status bar shown on hover:
- Starting position: (133, 126)-(837, 505), 704x379
- Zoom out position: (230, 136)-(740, 494), 510x358
Thanks. I see the problem--it wasn't using the right method to get the video window. You can try again.
Tested the change and it works correctly now! One minor issue I found in this version just now is when the window fills the width of the screen (HD), and then I zoom in, there is an incorrect zoom step:
- Starting size: (0, 121)-(1920, 929), 1920x808
- Zoom in: (0, 113)-(1920, 937), 1920x824 (this is an incorrect zoom step)
- Zoom in again: (0, 113)-(1958, 937), 1958x824 (this is the correct zoom step)
I have updated the description again.
Fixing that incorrect step was difficult. I wanted to preserve some nice behaviors, so now it will:
- Stop at the edge of the screen, if 2% would overflow the screen, but it will recalculate based on the clipped dimension
- If it was already filling the screen in one dimension, it will grow bottom right again
I did this in two passes, causing a bit of a rewrite.
You will need to retest all behaviors.
The faulty step is now fixed. I found the following issue, it is good otherwise:
- Closed video zoom
- Open a video
- Close a video
- Zoom in or zoom out the video: the video window width increases weirdly, and the height is set to 0 (if limitproportions=true) or is unchanged. Expected: zooming doesn't do anything without a loaded video