-
-
Notifications
You must be signed in to change notification settings - Fork 93
fix windows unable to move between screens #375
fix windows unable to move between screens #375
Conversation
...instead of forcing it back where it was. KWin changes a window's size before moving it between screens, so we can't react to a geometry change by always changing it back.
e35a3e0
to
d275b2c
Compare
Actually the problem is that kwin gives a I don't know what the previous behavior of kwin was before this broke but I've tested as old as kwin 5.21.4 and it still has the bug. Anyway I've finished going over the signal handling and I pushed an update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how can this PR fix the linked problems, since it does not change the behavior of the program (except may the line you removed, presumably by mistake).
From your description, the problem has to do something with the order of the signals handling, so the PR should do something to run all the necessary handlers in the order we want.
@@ -336,7 +342,11 @@ export class ControllerImpl implements Controller { | |||
|
|||
public onWindowGeometryChanged(window: EngineWindow): void { | |||
this.log.log(["onWindowGeometryChanged", { window }]); | |||
this.engine.enforceSize(window); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you want to remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure we do. I'm sure it made sense when it was added but we can't do that now. Perhaps kwin used to send this event less often or perhaps it was always a race condition waiting to happen. This line causes us to move a window back to its original location instantaneously after kwin moves it for example to a new screen. It's the direct cause of the linked bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the change is needed. Otherwise bismuth is not usable by keyboard on multimonitor setups.
Check #374 for how it looks without this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function exists to enforce the window geometry in case an application moves and resizes the window by itself.
The reason why the bug is present, is because this function incorrectly assumes, that window was move and resized by application and not by the user.
One of the ways to make a proper fix without potential breakages is to fix that function. Another option is to remove this function entirely (what this PR is doing de-facto) and let the apps break your tiling layout. I am not actually totally against this, because I think, that when an app moves your window it has a good reason to do so (the person written this function may have a different opinion).
So, this was just to explain why I was hesitant to apply this fix.
We can remove this function now and look for the results from the presence or absence of bug reports about this.
Indeed that one line is the problem. KWin changes a window's |
Hi, @benemorius - Thank you for helping fix issues lately. I was just testing this fix, but unfortunately, this doesn't fix on my system. And it's for Google Chrome and Microsoft Edge. Firefox, and other windows are all fine. I have shared more in the linked issue, would you know what's going wrong? |
This PR fixes the problem for me. Without it, the only way I can move windows around on kwin 5.25 is to float -> move -> unfloat. |
This one fixed my problem on X11. I'm too lazy to compile, so I just copy-pasted code to |
Solved it for me too. Cloning the current master branch and merging this fix gets me the desired behavior. git clone https://github.com/Bismuth-Forge/bismuth.git
cd bismuth
git remote add benemorius https://github.com/benemorius/bismuth.git
git fetch benemorius
git checkout -b <new_branch>
git merge benemorius/pr/fix/kwin-move-windows-between-screens
make setup-dev-env
[...] # rest of build instructions This really needs to be merged. Not being able to move windows to other screens from the keyboard is a deal breaker. |
Can this please get another look? This bug has been an issue for weeks by now with a fix available but not being merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presented a bit of reasoning in the code comment. So, could you please remove the definitions of enforceSize
function? The fix should be good to go.
@@ -336,7 +342,11 @@ export class ControllerImpl implements Controller { | |||
|
|||
public onWindowGeometryChanged(window: EngineWindow): void { | |||
this.log.log(["onWindowGeometryChanged", { window }]); | |||
this.engine.enforceSize(window); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function exists to enforce the window geometry in case an application moves and resizes the window by itself.
The reason why the bug is present, is because this function incorrectly assumes, that window was move and resized by application and not by the user.
One of the ways to make a proper fix without potential breakages is to fix that function. Another option is to remove this function entirely (what this PR is doing de-facto) and let the apps break your tiling layout. I am not actually totally against this, because I think, that when an app moves your window it has a good reason to do so (the person written this function may have a different opinion).
So, this was just to explain why I was hesitant to apply this fix.
We can remove this function now and look for the results from the presence or absence of bug reports about this.
Are there still no plans to merge this pr? |
I have been using this fix for a month and I haven't got any bugs. Without this, I wouldn't use Bismuth because this is happening for most electron apps. |
Since the OP is not active, I applied the fixes I requested in a separate PR: #419 (In other words: the issue is fixed). |
Summary
Currently on recent (?) versions of kwin we don't rearrange the tiles correctly when kwin moves windows between monitors instantaneously (versus dragging by mouse) and instead we arrange the moved window back to its original location. This PR fixes that.
I don't think this is the right way to fix it, but since the fix is reported to work and the bug is reported to be severe, I think we should do this for now. I'm currently investigating what changed about the events we receive from kwin. I think there are some other event-related issues too like for example when moving windows between activities or desktops, so I think a wider solution is needed here like maybe finding some missing events.Test Plan
Related Issues
Closes #370, closes #374