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

Snapping to screen edges when resizing #1515

Closed
wants to merge 1 commit into from

Conversation

Sp3EdeR
Copy link

@Sp3EdeR Sp3EdeR commented Jan 26, 2022

No description provided.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Feb 4, 2022

Hi, @clsid2, is there any way with which I can help to progress this and the other PRs?

@clsid2
Copy link
Owner

clsid2 commented Feb 7, 2022

What is this patch supposed to do?

There already is such an option:
Options > Player > Snap to Desktop edges

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Feb 7, 2022

@clsid2 This expands on the mentioned, existing feature to not only snap to screen edges when moving the window, but also snap to edges when resizing the window. It works when resizing from any corner or side, as well as with the hotkey I introduced recently. It is active only when the mentioned feature is turned on.

@clsid2
Copy link
Owner

clsid2 commented Mar 14, 2022

I will look at this after 1.9.20 release. Sorry for long delay.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Mar 14, 2022

Thanks, looking forward to it! 🙂

@Sp3EdeR Sp3EdeR force-pushed the feature/snap-sizing branch 2 times, most recently from a23ec44 to 235b136 Compare April 24, 2022 15:27
@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 24, 2022

@clsid2 I have updated the branch and resolved the conflict.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented May 7, 2022

@clsid2 have you got any updates on when you will have time to look into my PRs maybe? :)

@clsid2
Copy link
Owner

clsid2 commented May 9, 2022

Will take a bit longer. I try to spend a little less time on MPC-HC this month. But maybe next week.

@Sp3EdeR Sp3EdeR force-pushed the feature/snap-sizing branch 3 times, most recently from d215e2c to b4e4b21 Compare June 24, 2022 11:38
@Sp3EdeR
Copy link
Author

Sp3EdeR commented Aug 10, 2022

Any update on when you might look at my pending PRs, @clsid2 ? It's been quite a while...

@kkdexz
Copy link

kkdexz commented Aug 19, 2022

@clsid2 If this is functional, would you consider merging/committing it? Would much appreciate it as a feature.

Also, not sure if this would be the place to ask for this, but @Sp3EdeR could you perhaps make the window resizer dependent on it's position/location? Esp if its snapped to an edge, e.g. if snapped to bottom-left corner, window resizes up + right; if in top-right corner, window resizes down + left. I think you may have mentioned this or something similar in #1513 (err...or maybe not?).

I use the resizer hotkeys quite often, but the way it is currently, I can only use them if the window is snapped to the (top-) left corner/edge of the desktop or somewhere in the center, as the window only increases/decreases downwards & to the right. Hope this makes sense. Thanks.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Aug 19, 2022

@kkdexz I considered a few possibilities for the implementation of step-by-step resizing, but I think anything more than the current implementation would result in far too complex code (the corner snap logic is not easily generalized, so it gets very bloaty with many cases). I bound window resizing to the mouse wheel, which makes moving the window quite fluid.

What I did think about separately - which I think may solve your problem - is perhaps a separate window management function, bound to the Win+Arrow keys. Aero snap is currently disabled for the MPC-HC window, so these hotkeys are "free", so for example it could be: Win+Up -> snap to top and fit to horizontal center; Win+Left -> snap to left, fit to vertical center, etc.

But for now, I will wait and see whether my PRs get integrated, so I can return to master on my own computer. I'm not inclined to maintain a separate fork on the long run.

@kkdexz
Copy link

kkdexz commented Aug 20, 2022

@Sp3EdeR Are you sure about Aero snap being disabled for the MPC-HC window? That doesn't seem to be the case for me. In fact, it was explicitly offered as a suggestion for #1817. The only way it is disabled is if all menus and borders are hidden. Not sure if that is how you usually use the player?

Anyways, seems like this is beyond the scope of this PR; perhaps we can revisit it sometime in the future, after this PR is hopefully accepted. Thanks for the explanation.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Aug 20, 2022

Yea, @kkdexz, no. I just didn't do enough testing while formulating the idea. I use the minimal window mode, where the window's style disables Aero snap. It will need further pondering...

I still think that in case of the video player, the Win+Arrow keys would probably make more sense to align the window to the screen edges rather than to tile it with other windows. An option could possibly be added to the program to override the normal Aero snap functionality. Then a piece of code could install a keyboard hook to detect Win+Arrow presses and then override the window messages that Windows generates by default with an alignment logic. At least, right now, that seems like a better way to align windows than a multidirection resizer logic.

@clsid2
Copy link
Owner

clsid2 commented Apr 3, 2024

@adipose
Can you review this patch if you have time to spare?

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 3, 2024

To expand on this explanation:

@clsid2 This expands on the mentioned, existing feature to not only snap to screen edges when moving the window, but also snap to edges when resizing the window. It works when resizing from any corner or side, as well as with the hotkey I introduced recently. It is active only when the mentioned feature is turned on.

When resizing the window, imagine the bottom-right corner point in relation to the screen's rectangle. The sizing is logically correct if the screen fits within the screen. This means it should fit the edge that results in a smaller video size. One option would be to draw a screen diagonal and find the intersection with the screen rectangle. This implementation just calculates with integer-math to find the closest edge. The critical use-cases are when resizing the video window to be very close to the corner of the screen. The if-branches always find the "smaller" fit. Perhaps this can help. You can ask additional questions if needed.

@adipose
Copy link

adipose commented Apr 3, 2024

I have reviewed the behavior and it seems to work well. I did observe that snapping to certain aero "hot" points has the effect of enabling potentially undesired behavior. For example, snapping to the bottom of the screen also enables the full vertical size. If you only wanted to snap to the bottom, you now are snapped top and bottom.

I wonder if it could snap 1 pixel short and then adjust it after the placement, unless they kept moving?

@adipose
Copy link

adipose commented Apr 3, 2024

The problem is better solved by increasing the snap zone from 16->32 or something. What happens is that sometimes, depending on where exactly the cursor is gripping the window, that snapping to the edge actually causes the cursor to reach the monitor edge. Other times, it falls short. If you go back and forth a few times you will see that sometimes you can't avoid it trying to do an aero snap.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 4, 2024

The problem is better solved by increasing the snap zone from 16->32 or something. What happens is that sometimes, depending on where exactly the cursor is gripping the window, that snapping to the edge actually causes the cursor to reach the monitor edge. Other times, it falls short. If you go back and forth a few times you will see that sometimes you can't avoid it trying to do an aero snap.

The potential issue with large snap distance is when you use it in connection with the window size incremet/decrementing function (ID 33456, 33457). The function may get stuck on snap mode, not being able to get free of it. I did not have time to test this now, though.

I did not try aero snap, since I have it disabled on my computer. I suppose the user's setting to have Aero Snap on in Windows should probably be the primarily respected function, and should be kept supported when the mouse reaches the screen edge, normally. Perhaps it would make sense to disable Aero snapping, when edge snapping is turned on, but I don't know of a good way to disable Aero snapping for resizable windows with WinAPI. But anyway, the snapping algorithm should not influence how snapping works, it is simply that the two features may compete for supremacy in some specific scenarios, Aero snap being the stronger.

@adipose
Copy link

adipose commented Apr 7, 2024

What I am talking about is not a snap, it's a resize. If you drag to the bottom (or top) of the monitor (I believe your cursor must touch the last pixel), it makes the window full monitor working space height.

For some reason the cursor moves there with the snap, and I will try to record it. However, if you are very precise you can avoid the resize.

They aren't really related behaviors, rather snapping to the edge requires getting close to the edge, which sometimes mistakenly activates the vertical sizing.

Patch is fine and works correctly. Perhaps an advanced option to control the snap distance would help though.

@adipose
Copy link

adipose commented Apr 10, 2024

2024-04-09_18-16-26.mp4

Here's a video of the behavior. On the last snap it goes to resize without any intentional movement of the mouse to the edge.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 11, 2024

2024-04-09_18-16-26.mp4
Here's a video of the behavior. On the last snap it goes to resize without any intentional movement of the mouse to the edge.

I did some testing myself on this, on both the official release and my modified version. It seems to me that for both versions, the Aero Snap distance is inconsistent in some cases. It seemed like when the Window got docked once, the aero snap distance increased sometimes. Unfortunately the Aero Snap logic can't really be influenced (except for using the minimal view mode, which turns snap off completely for the Window). So I don't see any improvement opportunities.

@adipose
Copy link

adipose commented Apr 11, 2024

So I don't see any improvement opportunities.

If you change the snap distance to 20 or something, does it change the frequency of the problem?

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 11, 2024

So I don't see any improvement opportunities.

If you change the snap distance to 20 or something, does it change the frequency of the problem?

I did some calculations that should help answer objectively. From what I can see, the aero snap is ~6px on first go, and can be increased to a maximum of ~24px (cursor from screen edge), and the resizing touch area is ~10px on default Win11 frames with 100% DPI. On the initial drag to the edge of the screen, the Aero Snap distance seems to be ~6px from the cursor (so ~6px to ~-4px from window edge), so ~10px to ~20px is left for snapping before Aero is triggered. With the 24px aero snap distance, we're at ~-8 to ~2px left for snapping before Aero is triggered. With a snap distance of 20, we'd be at ~-4 to ~6px left for snapping in this use-case, so certainly a reduced frequency, but not by much. 26px snap distance would probably eliminate this (~2px to ~12px before Aero is triggered), but that would feel significantly different from the move-snap distance (16px).

I re-checked the diff more carefully here, and luckily this condition will handle the use-case of avoiding getting stuck to snapping on increments, so the snap distance could be increased.

The 24px aero snap distance does not seem to be a very common user use-case, most users will probably just size the window with one move. I wouldn't expect this to ever really be triggered if the "Limit Window proportions on resize" is set to on (while playing a video) or when using the minimal view. I leave it to your consideration what you feel most comfortable with.

@adipose
Copy link

adipose commented Apr 11, 2024

Thanks for the analysis. I suggest adding an advanced option to tweak the snap distance with a range of 16-32 and let the user decide if the issue is worth overriding.

@clsid2
Copy link
Owner

clsid2 commented Apr 11, 2024

The issue is not really important imo. Window snapping is disabled by default. So if you enable it, you should expect it to snap when getting close to screen edge.

@adipose
Copy link

adipose commented Apr 11, 2024

The issue is not really important imo. Window snapping is disabled by default. So if you enable it, you should expect it to snap when getting close to screen edge.

I think the patch is fine as-is. But the problem I am describing is that it snaps and also activates an aero vertical resize function at the same time. The oddity comes from the way microsoft activates the resize, which is technically (supposed to be) if the first pixel of the cursor touches the edge of the monitor. You can see that the cursor position does not remain consistent when dragging in my video.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 12, 2024

I may be able to find some time to add an advanced option for the snap distance in the weekend. But I would only work on it, if the PR will actually be merged afterwards. Don't want to do work in vain.

@clsid2
Copy link
Owner

clsid2 commented Apr 12, 2024

Fix the merge conflict that Github shows and I will merge it.

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 12, 2024

Fix the merge conflict that Github shows and I will merge it.

It's not a simple conflict, the function clashes with the ed46604 change (#2710). I'm not familiar with that feature. But if it changes the position of all four edges of the window, then the snapping logic would probably be insanely complex.

@adipose
Copy link

adipose commented Apr 12, 2024

My gut says it won't be a problem.

What that does is to make zooming center the window instead of growing down and to the right.

But GetZoomWindowRect has checks for snapping and basically does nothing interesting in at least some cases.

I can try to merge it if you aren't sure.

@adipose
Copy link

adipose commented Apr 12, 2024

#2736

You can check this out and see how it works for you. It's merged to current with some fixes.

@adipose
Copy link

adipose commented Apr 13, 2024

    if (nID == ID_VIEW_ZOOM_ADD && rect.right <= rectOrig.right ||
        nID == ID_VIEW_ZOOM_SUB && rectOrig.right <= rect.right)
    {
        rect.right += step;
        rect.bottom = ceil(rect.top + rect.Width() / ratio);
        OnSizingFixWndToVideo(WMSZ_RIGHT, &rect);
    }

What is the purpose of this code?

@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 14, 2024

    if (nID == ID_VIEW_ZOOM_ADD && rect.right <= rectOrig.right ||
        nID == ID_VIEW_ZOOM_SUB && rectOrig.right <= rect.right)
    {
        rect.right += step;
        rect.bottom = ceil(rect.top + rect.Width() / ratio);
        OnSizingFixWndToVideo(WMSZ_RIGHT, &rect);
    }

What is the purpose of this code?

That is what ensures that when you increment the window edge past the screen border, the snap does not keep the window edge stuck to the screen edge. Multimonitor people may want to increase window size past one monitor, snap ensures they have a step that fits the monitor border.

This is dependent on enabling the fSnapToDesktopEdges.
Also fixed a crash issue when using the resizing hotkey with no video playing
@Sp3EdeR
Copy link
Author

Sp3EdeR commented Apr 14, 2024

Okay, I think the best approach is that I pushed snap sizing without any incremental zoom support, so that it can be incremented quick and easy. I pushed this update to the branch now.

I have opened the #2740 issue to document some issues with the 2.2.1 behaviour of the incremental zoom feature. The desired behaviour can be discussed there separately to decide how to handle incremental zooming before any code changes are done.

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.

None yet

4 participants