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

Incremental zoom from center issues #2740

Open
Sp3EdeR opened this issue Apr 14, 2024 · 19 comments
Open

Incremental zoom from center issues #2740

Sp3EdeR opened this issue Apr 14, 2024 · 19 comments

Comments

@Sp3EdeR
Copy link

Sp3EdeR commented Apr 14, 2024

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 allow zoom past monitor size #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 allow zoom past monitor size #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 allow zoom past monitor size #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.

@adipose
Copy link

adipose commented Apr 14, 2024

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.

@NBruderman
Copy link

I think this can be closed now

@clsid2 clsid2 closed this as completed Apr 15, 2024
@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 15, 2024

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 .

@clsid2 clsid2 reopened this Apr 15, 2024
@Sp3EdeR Sp3EdeR mentioned this issue Apr 16, 2024
@adipose
Copy link

adipose commented Apr 17, 2024

#2743

@NBruderman
Copy link

@Sp3EdeR after the latest patch, what issues are still not fixed from the list above?

@ArcticGems
Copy link

@Sp3EdeR I also want to know what isn't fixed.

Could you use 'strikethrough' on whats fixed.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 18, 2024

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.)

@adipose
Copy link

adipose commented Apr 18, 2024

If the point overlaps the rect border, it is considered "not in."

It first hits full screen constraints. Next it expands, in testing.

@adipose
Copy link

adipose commented Apr 18, 2024

  • 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?

@adipose
Copy link

adipose commented Apr 18, 2024

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.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 19, 2024

  • 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. :)

@adipose
Copy link

adipose commented Apr 19, 2024

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
Copy link

adipose commented Apr 23, 2024

#2752

@adipose
Copy link

adipose commented Apr 27, 2024

@Sp3EdeR

Can you review latest patch?

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 27, 2024

@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).

@adipose
Copy link

adipose commented Apr 28, 2024

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
Copy link

adipose commented May 9, 2024

@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?

@Sp3EdeR
Copy link
Author

Sp3EdeR commented May 12, 2024

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 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.
    • 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 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

@clsid2
Copy link
Owner

clsid2 commented May 12, 2024

2% step is fine

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

No branches or pull requests

5 participants