-
Notifications
You must be signed in to change notification settings - Fork 100
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
Feat/navigate across monitor #417
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this shouldnt happen, do we really need this code? :)
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this shouldnt happen, do we really need this code? :)
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I didn’t see that. Will update
do { | ||
this.SwitchFocusToPreviousMonitor(); | ||
} | ||
while(!FocusedWorkspace.ManagedWindows.Any()); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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