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

fix: change min cursor padding to 0 #6027

Merged
merged 11 commits into from
Jul 2, 2024

Conversation

drendog
Copy link
Contributor

@drendog drendog commented May 11, 2024

Describe your PR, what does it fix/add?

Since ed411f5 a padding has been added to the cursor, which means that some application that expect a hit in the window border bounds, will never detects the hit there.
For example when using firefox at full screen mode, that expect a hit on top of the windows to show the hidden tabs, it will never detects the hit.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

ed411f5 #5902

Is it ready for merging, or does it need work?

Ready

@Agent00Ming
Copy link
Contributor

Agent00Ming commented May 11, 2024

but my scroll bar 😭 #5902 (comment) (JK, but this could be turned into a per-side config maybe)

Copy link
Contributor

@Agent00Ming Agent00Ming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firefox fullscreen functionality is reduced by the 1 pixel minimum. This does indeed fix it.

@vaxerski
Copy link
Member

wouldnt it be better to add a setting to have the padding only on the bottom right?

@morr0ne
Copy link
Contributor

morr0ne commented May 14, 2024

wouldnt it be better to add a setting to have the padding only on the bottom right?

Maybe I am not understanding this right, but what is the purpose of the padding? It mainly seems to break stuff.

@Agent00Ming
Copy link
Contributor

wouldnt it be better to add a setting to have the padding only on the bottom right?

Maybe I am not understanding this right, but what is the purpose of the padding? It mainly seems to break stuff.

#5902 (comment) and #5902 (comment) should explain the reason behind it.

@morr0ne
Copy link
Contributor

morr0ne commented May 17, 2024

Okay after reading through the pr I understand the purpose of the padding. However it feels like it should default to 0 and then users affected can add the 1px padding themselves. If this gets released as is most desktop users will end up with a broken config by default without even knowing why.

+1 for this pr

@vaxerski
Copy link
Member

vaxerski commented May 17, 2024

I still think this is the wrong solution and instead #6027 (comment) should be done

@morr0ne
Copy link
Contributor

morr0ne commented May 17, 2024

I still think this is the wrong solution and instead #6027 (comment) should be done

That could be a solution. Regardless this should be considered a blocker for 0.41.0 until resolved imo

@vaxerski vaxerski force-pushed the main branch 2 times, most recently from 358e59e to 3fd6c1b Compare June 3, 2024 16:46
@drendog
Copy link
Contributor Author

drendog commented Jun 21, 2024

any news about it? Moving the problem is not a solution, since this compositor is supposed to be desktop-first, putting padding on the bottom will break the applications that expect a hit on bottom edge

Copy link

@charchil0 charchil0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default minimum of hotspot padding in the recent update has made hyprland so unusable with firefox that i have temporarily swithched to i3. Without firefox what is a computer? i so frustrating as i usually have 100 tabs open so i cannot cycle through them and i think so is the case with many people.

src/managers/PointerManager.cpp Show resolved Hide resolved
src/config/ConfigManager.cpp Show resolved Hide resolved
@drendog
Copy link
Contributor Author

drendog commented Jun 22, 2024

@vaxerski

@vaxerski
Copy link
Member

vaxerski commented Jun 22, 2024

@drendog #6027

I still think this is the wrong solution and instead #6027 (comment) should be done

@drendog
Copy link
Contributor Author

drendog commented Jun 22, 2024

putting the padding at the bottom will break those applications that expect a bottom edge cursor hit, in this way it's only a moving of the problem and not a real solution

@vaxerski
Copy link
Member

vaxerski commented Jun 22, 2024

was the cursor able to go to (x,y) on a (w,h) sized monitor before? I don't think so. That was the issue with what @Agent00Ming was dealing, precisely because apps would NOT get a bottom edge hit because the coord would be off-limit.

@drendog
Copy link
Contributor Author

drendog commented Jun 22, 2024

ok i got it, i didn't check that the cursor can go out of screen limit, i'll fix it soon

@drendog drendog marked this pull request as draft June 23, 2024 21:56
@drendog
Copy link
Contributor Author

drendog commented Jun 24, 2024

if i didn't misunderstand, the problem is the cursor hotspot can reach (w, h) instead of the maximum (w - 1, h - 1). I figured out that closestPoint of Box, here the maximum is x + w or y + h. I currently adjusted the coords in NEAREST_LAYOUT, if that was an error on hyprutils, i can open a pr for it and remove the adjustment here

@drendog drendog marked this pull request as ready for review June 24, 2024 12:47
@vaxerski
Copy link
Member

this won't work if you have two monitors side-by-side touching each other on the right or bottom.

@drendog
Copy link
Contributor Author

drendog commented Jun 24, 2024

this won't work if you have two monitors side-by-side touching each other on the right or bottom.

what do you mean? I've tried every combo using 2 monitors and i don't see any issue

@vaxerski
Copy link
Member

mon1: 1920x1080 at 0x0
mon2: 1920x1080 at 1920x0

|--------------------------|--------------------------|
|                          |                          |
|                          |                          |
|                          |                          |
|                          |                          |
|--------------------------|--------------------------|

The point (1919.5, 69) is now treated as out of bounds, despite not being such.

@drendog
Copy link
Contributor Author

drendog commented Jun 25, 2024

The point (1919.5, 69) is now treated as out of bounds, despite not being such.

should we care since the functions like move_cursor take the cursor position which are integers, and NEAREST_LAYOUT takes care of giving the correct position while having the cursor position as double?

@vaxerski
Copy link
Member

we should because wlroots is being removed and aquamarine takes doubles. Furthermore internally it's stored as a double, so when a user would be moving their mouse veeery slowly (e.g. 0.2px a frame) they would be blocked from leaving their screen:

1918 ok
1918.2 ok
1918.4 ok
1918.6 ok
1918.8 ok
1919 ok
1919.2 clamped to 1919
1919.2 clamped to 1919
....

@drendog
Copy link
Contributor Author

drendog commented Jun 25, 2024

this should fix the sub-pixel coords clamp on edges, but can I ask you why use floating points for the cursor position? Shouldn't using fixed points be more appropriate maybe using wl_fixed_t?

@vaxerski
Copy link
Member

why not?

@drendog
Copy link
Contributor Author

drendog commented Jun 26, 2024

just asking, nvm, then i found out that using double for cursor position it's not as bad an idea as I thought. Btw, can you give a feedback about last commit?

@drendog drendog requested a review from vaxerski June 27, 2024 19:14
@vaxerski
Copy link
Member

I feel like the reasonable solution is:

Make padding a 4-value special value (like gaps_in for example) which describes per-side padding.

for example: 0,1,1,0 for 1px on bottom and right.

Then, assume there is a box around the cursor, in this case at cursor x,y with size 1,1

clamp that box to the region of all screens

return coord.

@drendog
Copy link
Contributor Author

drendog commented Jun 28, 2024

Isn't that padding solution, a solution to a problem that shouldn't even exist?
I mean, from what i got, it seems that the origin of the problem is due to wrong calcs of some method (in this case closestPoint) of the CBox, which returns an extra shift of point position when we deal with bottom and right edges, because it clamps to x + w, insteand of the prev representable value of x + w.
Just look at the current wlroots implementation of wlr_box_closest_point, that it clamps up to x + w - arbitrary number, which in this case arbritrary number is 1/65536.0.
If you agree about it i can open a PR to fix it on hyprutils.

Fixing by padding hack, may not be the ideal solution, and it makes the code unnecessarily complex when such a problem can be solved by modifying a few lines.

if we were to fix by adopting padding in each direction, should i do it with 4 boxes?

@vaxerski
Copy link
Member

returning x - epsilon,y - epsilon is a fine solution by me, as long as it doesnt break something else

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the hyprutils mr merged, this is fine. Thanks!

@vaxerski vaxerski merged commit 2fa57f2 into hyprwm:main Jul 2, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants