Hyprland
Hyprland copied to clipboard
hyprwm
Add switching to previous workspace
Describe your PR, what does it fix/add?
Add the ability to switch to the last visited workspace as described in #324, inspired by i3. The syntax suggested in the issue has been adopted: bind=MODIFIER,KEY,workspace,previous.
Also included is a toggle setting general:auto_back_and_forth to allow switching back to the previous workspace by trying to switch to the same workspace again. I use it all the time in sway and my workflow would be greatly improved by the ability to quickly switch to and from a workspace to check on email, discord, spotify, etc.
E.g. start in workspace 1, then Super+7 to workspace 7, and then Super+7 again will move to workspace 1.
Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)
I've moved a lot of code from CKeybindManager::changeworkspace to CCompositor::changeWorkspace, to make it easier to reuse the raw workspace switching.
This is also my first pull request, so my apologies if I've broken any conventions, just let me know and I'll fix it. I'm also not super experienced with C++, but I think I worked out enough for this small change.
Is it ready for merging, or does it need work?
Yes it's ready as far as I know (it builds and works in debug mode). However:
- I'm not sure if I've put
m_bAutoBackAndForthin the right place - let me know if there's somewhere else for settings like that to go. - Not sure if some hyprctl commands need to be added too.
- Of course wiki updates will be necessary too, but I don't know how that's normally done.
generally 90% of the code here is just a big ????
Why are you storing the last workspace in CCompositor?
Why are you moving the function?
Why did you refactor it???
this entire thing can be done with a chain in 10 lines of code or less.
generally 90% of the code here is just a big ????
Why are you storing the last workspace in CCompositor?
I stored it there because I figured it was information scoped to the compositor, not workspaces or even monitors (previous workspace may be on another monitor). But you have a point, it might make more sense to implement as a chain for a series of 'back' operations. In which case, I could just store the source workspace in each workspace, instead? And use that to go back?
Why are you moving the function?
Why did you refactor it???
I wanted to re-use the workspace switching code and figured that it would make sense to go there, rather than in CKeybindManager. But I probably did unnecessarily change it too far, my bad, I didn't consider that would be an issue. I'll fix that, and just have conditions for setting the target workspace in CKeybindManager::changeworkspace instead. Much smaller change then.
this entire thing can be done with a chain in 10 lines of code or less.
Probably.
Look, sorry if you felt like I wasted your time. I'm excited to contribute here and I really like what you've made here. But I'm learning, and I really do want this feature. I'll try and fix it up as you suggest, just need some clarification:
- Is it acceptable for me to store the previous workspace inside each workspace object?
- Where can I store/access the
autoBackAndForthsetting?
Is it acceptable for me to store the previous workspace inside each workspace object?
you should do that. Store by ID though and check if the previous workspace is valid in case the chain gets broken at any point.
Where can I store/access the autoBackAndForth setting?
g_pConfigManager->getInt("blah:blah")
Is it acceptable for me to store the previous workspace inside each workspace object?
you should do that. Store by ID though and check if the previous workspace is valid in case the chain gets broken at any point.
Where can I store/access the autoBackAndForth setting?
g_pConfigManager->getInt("blah:blah")
Cool, alright. I'll reset my commits and do it like that. Probably later tomorrow (Australian time).
On the matter of checking if the previous workspace is valid though - I've made the assumption that even if a 'previous workspace' doesn't currently exist, it should just be created rather than doing nothing. Does that work for you? I just figure, why have a command sometimes do nothing if there's a possible interpretation?
If you're just talking about the first workspace on startup though, yeah for sure, I'll check for that.
This implementation will also allow for cycles to occur, unless I destroy (set to -1) the 'previous workspace' id each time the user goes back a workspace. Which sounds better? I'm leaning towards eliminating cycles, but perhaps being able to create a custom workspace cycle could be useful for some users?
doesn't currently exist, it should just be created rather than doing nothing
no
This implementation will also allow for cycles to occur, unless I destroy (set to -1) the 'previous workspace' id each time the user goes back a workspace.
make it configurable
Any progress on this?
Shoot, thanks for the reminder! I completely forgot about this. I'm starting a new job tomorrow so I'll try and redo this PR over the next few days or at the latest next weekend. If anyone else wants to give it a go instead - please, feel free. Can't be hard to do better than I did 😅
Any progress on this?
Shoot, thanks for the reminder! I completely forgot about this. I'm starting a new job tomorrow so I'll try and redo this PR over the next few days or at the latest next weekend. If anyone else wants to give it a go instead - please, feel free. Can't be hard to do better than I did sweat_smile
Good luck on the job!
@yavko Thanks! It's been a good first week, lots of flexibility to work from home has been lovely.
@vaxerski I've tried to take on your feedback as best I could. The only thing I didn't do was ignore non-existent workspaces when going to previous. I checked in i3 & sway, and this is how they do it too - users who want this setting will expect the behaviour I've implemented (automatically re-creating previous workspaces which have been destroyed).
The below PR description has been updated.
Describe your PR, what does it fix/add?
Add the ability to switch to the last visited workspace as described in https://github.com/hyprwm/Hyprland/issues/324, inspired by i3. The syntax suggested in the issue has been adopted: bind=MODIFIER,KEY,workspace,previous
Also included is a toggle setting general:workspace_back_and_forth to allow switching back to the previous workspace by trying to switch to the same workspace again. I use it all the time in sway and my workflow would be greatly improved by the ability to quickly switch to and from a workspace to check on email, discord, spotify, etc.
E.g. start in workspace 1, then Super+7 to workspace 7, and then Super+7 again will move to workspace 1.
Finally there's a general:allow_workspace_cycles option which toggles whether or not the previous workspace ID is retained after switching to the previous workspace. If this is not done, then there are situations in which a cycle can be formed and the previous workspace can be switched to endlessly.
Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)
Nope, besides some potentially unsafe pointer use, but as far as I can tell it's inline with the rest of the surrounding code and it would appear to be safe as far as I can tell. No bugs found in my testing, it's not a huge change.
My only real caveat is that today I've been getting an "Attempted to draw NULL texture!" assertion error when I run in debug, which gives me a black screen when I ignore it - nonetheless I've still been able to check that my code works by just looking at the live logs. So it should be fine as I suspect the issue is that there's something up with my dev environment.
Is it ready for merging, or does it need work?
Yes it's ready, pending approval.
Of course wiki updates will be necessary too, but I don't know how that's normally done.
Hey! Is there any way to bind workspace_back_and_forth to something?
For example:
bind = $mainMod, TAB, workspace_back_and_forth,
Hey! Is there any way to bind workspace_back_and_forth to something?
You need to setup this general option, it's not a dispatcher.
Use this instead:
bind = $mainMod, TAB,workspace, previous
You need to setup this general option, it's not a dispatcher.
Use this instead:
bind = $mainMod, TAB,workspace, previous
Thank you! This was exactly what I wanted :)