Feat/navigate across monitor #417
Conversation
Feat/focus layout
… screen when no secondary
Feat/focus layout
merge master into fork
|
Hey there and thanks for the PR! In general the idea seems useful, and superficially the code looks good. I'll try to get this properly reviewed and merged as soon as I have time! |
| { | ||
| // shouldn’t happen. | ||
| windows[0].Focus(); | ||
| } |
There was a problem hiding this comment.
If this shouldnt happen, do we really need this code? :)
There was a problem hiding this comment.
I copied the code from the window class and didn’t think too much about it, but yeah it shouldn’t be needed
| { | ||
| // shouldn’t happen. | ||
| windows[0].Focus(); | ||
| } |
There was a problem hiding this comment.
If this shouldnt happen, do we really need this code? :)
There was a problem hiding this comment.
I copied the code from the window class and didn’t think too much about it, but yeah it shouldn’t be needed
| { | ||
| this.SwitchFocusToNextMonitor(); | ||
| } | ||
| while(!FocusedWorkspace.ManagedWindows.Any()); |
There was a problem hiding this comment.
In theory, if you have no windows open and trigger this method, won't this code loop forever?
I think we may benefit by extracting this code into its own function (SwitchFocusToNextMonitorWithWindows() ?) where we can implement a better check without complicating this method.
There was a problem hiding this comment.
yes I didn’t see that. Will update
| do { | ||
| this.SwitchFocusToPreviousMonitor(); | ||
| } | ||
| while(!FocusedWorkspace.ManagedWindows.Any()); |
There was a problem hiding this comment.
In theory, if you have no windows open and trigger this method, won't this code loop forever?
I think we may benefit by extracting this code into its own function (SwitchFocusToPreviousMonitorWithWindows() ?) where we can implement a better check without complicating this method.
There was a problem hiding this comment.
yes I didn’t see that. Will update
|
Hey there and sorry for the delay. I finally got around to reviewing this PR and I have a few small comments. If you get those issues resolved, this looks like a nice addition and I would be happy to merge it. |
|
I’m just thinking that I should also have the same method for moving windows across monitor in the same way. I’ll update the MR with that. |
|
Any news on this PR? :D |
|
Hello,
Sorry life's been a bit full. I'll try to work on that when I can, I havent
forgotten about it :)
Le mer. 19 avr. 2023 à 09:17, Jostein Kjønigsen ***@***.***>
a écrit :
… Any news on this PR? :D
—
Reply to this email directly, view it on GitHub
<#417 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF62GE7SQJV5562AGTM5JDTXB6GRVANCNFSM6AAAAAAVEHIYZQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Add two functions to workspace manager to allow for navigation across all monitors