Add commands ReleaseScratchPad, AttachScratchPad, NextScratchPadWindow and PrevScratchPadWindow
#768
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Do I get your issue right, that when you move an opened scratchpad window to another tag the scatchpad is nod visble on that tag at first and when toggled again, it gets tiled instead of its initial floating style? |
|
Hi VuiMuich, Not entirely: When I move the window in the scratchpad to a tag and then try to toggle the scratchpad again, I expect the scratchpad to create a new window, but the moved window gets hidden instead. This is because Leftwm still remembers the moved window as being a scratchpad. This PR fixes this edge case by checking if the |
|
I see. I guess what you propose would be useful to add a command If you wouldn't mind the extra effort to make this an actual command this would be very welcome. |
|
Works for me. When I have time, I will make the adjustments. |
* Contains removal of previous scratchpad release functionality command * Adds command to config * Adds command option to `leftwm-command`
|
Hey, finally had some time to test this a bit. The only thing I can foresee that this creates a demand for some Edit: just a nitpick on the doc side: the empty value option should mention that it uses the current tag as target, just to be complete |
|
The attach scratchpad seems like a nice idea, I will try to tackle this when I have some more time on my hands within a few weeks 😀. |
* Logic put into its own function * Refactored ToggleScratchPad to use the new functions * Added tests to make sure all still works
|
Again some late reply on my side 😆 |
VuiMuich
changed the title
ReleaseScratchPad (and probably AttachScratchPad)
|
LGTM and works great. Thanks 🙏 One thing though: when multiple windows get attached to a single scratchpad only the last attached window can be accessed. This should be mentioned in the documentation. In the future we should add the possibilty to focussing and moving commands to decide via an optional value if scratchpads should be included or not (maybe even a variant to effect scratchpads exclusivlys?). |
|
Sorry just found one more thing, that I believe es better fixed before merge: Besides that IMO merge ready. |
- Extend the current behavior so the user can cycle through the scratchpad windows
|
Some notes about the The last few commits (not the last one) added some extra checks to the scratchpad related commands so a window can not be added twice to the same scratchpad and that every time a window is shown, the window is first checked if it is still active by looking it up the pid in the still managed windows. The last commit adds 2 more commands that I really wanted now that the scratchpads can have more than 1 window. These are I hope you like these new additions as much as I liked making them 😄. |
|
Sorry for my weird response, I only saw your comments now.
I think this is fixed by the addition of
Should be included in the last few commits 😄! |
SamClercky
changed the title
ReleaseScratchPad (and probably AttachScratchPad)ReleaseScratchPad, AttachScratchPad, NextScratchPadWindow and PrevScratchPadWindow
|
Awesome work. I was a bit reluctant about the Prev/Next commands, as generally I would probably like an option to the existing moving and focusing commands a bit better. But I totally get, why you went this route and I guess for now it's absolutely ok to do it this way. Maybe in the future we will be able to support the sofisticated keybind vision (or delusion, who knows) I sport from time to time... |
|
Hey,
Otherwise the latest look really good. Sorry its "one more thing" every time 😁 |
|
Hi @AethanFoot, I applied most of the your comments to the code and put all the ones I fixed on resolved. For the ones where I still had doubts or couldn't immediatly resolve, I added some comments 😄 |
* Moving `Scratchpad` to models * Moving `scratchpad_xyhw` to models * Renaming `scratchpad_xyhw` to `Scratchpad::xyhw`
AethanFoot
approved these changes
Sep 17, 2022
There was a problem hiding this comment.
Looks good, nice work! Not set up to test right now, but seems like @VuiMuich has tested, so we can merge anytime (we need to figure which order we a gonna merge things so might need to sort some conflicts)
|
I have few regression with the latest commits:
Edit: after having a look at the last commits code, I don't think it was this one. Most certainly I didn't have a rebuild with the previous bunch of review commit. |
|
Hi VuiMuich, I can't seem to reproduce these issues. Can you provide me more information on how to produce the undesired behavior? This is what I see on my PC in Xephyr (last commit) but behavior is similar on real hardware. In the first In the second The last issue with Next/Prev commands, sounds like the same bug you mentioned the 23rd of july. Could it be it is the same? If so I should have been fixed in Thanks in advance 😄 |
|
This is my relevant config (in ron): scratchpad: [
(name: "DynamicScratchpad", value: ""),
],
keybind: [
(command: ReleaseScratchPad, value: "", modifier: ["modkey", "Shift"], key: "d"),
(command: AttachScratchPad, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "d"),
(command: NextScratchPadWindow, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "comma"),
(command: PrevScratchPadWindow, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "period"),
(command: ToggleScratchPad, value: "DynamicScratchpad", modifier: ["modkey"], key: "d"),
]For 1): on empty tag open new window -> assign to If you need more I can see if I have time to do a screencapture tomorrow. 🤔 can't find any comment from 23rd July, did you mean this:
The issue now is different, as the SP get cycled, but with all SP hidden on every second command. |
|
Hi VuiMuich, I tried to reproduce the bug, but couldn't trigger it. From looking at your config, I suspect the issue may be in that the scratchpad value is empty (shell command that does not generate a window). This creates a place in the scratchpad It might be that the scratchpad code tries to show something, but actually only a non existing window is shown. However this should not be possible as the cycle and toggle commands filter these invalid window pid's out. (see Another option for me not being able to trigger the bug is that it might be a timing related issue and that my computer is a little slower/faster, so I won't get into an invalid state. Could you maybe show me the What do you think? I would realy like to have this fixed and get this PR merged 😄 |
|
Just a quick update, as I started digging further into this: I reverted to BTW. part of the regression was also present for scratchpads configured in the 'classical' pre this PR way, I just omited them from the config example trying to be as minimal as possible. Will edit this post with updates later. First quick update: scratchpad: [
(name: "ScratchpadTerminal", value: "alacritty", x: 0.055, y: 65, width: 0.89, height: 0.34),
(name: "BTop", value: "alacritty -e btop", x: 10, y: 65),
(name: "DynamicScratchpad", value: ""),
],
keybind: [
(command: ReleaseScratchPad, value: "", modifier: ["modkey", "Shift"], key: "d"),
(command: AttachScratchPad, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "d"),
(command: NextScratchPadWindow, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "comma"),
(command: PrevScratchPadWindow, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "period"),
(command: ReleaseScratchPad, value: "", modifier: ["modkey", "Shift"], key: "d"),
(command: AttachScratchPad, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "d"),
(command: NextScratchPadWindow, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "comma"),
(command: PrevScratchPadWindow, value: "DynamicScratchpad", modifier: ["modkey", "Control"], key: "period"),
],and a log snippet for toggling the And logs for a No idea rn, why I can't find the debug message you mentioned... I would also love to get this merged. @AethanFoot could you maybe test this as well? If it just happens on my machine I would be ok to see this as corner case scenario to be fixed later. Update 2: |
This reverts commit 4e48f7f.
|
Ok, when I I'll push my revert, if @AethanFoot finds a different solution/approach, we still can improve in a future PR. A very minor thing I noticed though, but don't consider this a blocking reason: when cycling through scratchpads, the focus jumps back to a tiled window for a fraction of a second (might indeed be my 2014 laptop though 😦). |
TornaxO7
suggested changes
Sep 20, 2022
This is interesting. When trying on my machine with this commit reverted, I feel less jitter. Maybe the problem is in the delay between moving the SP to another tag (
This is something I have noticed, but didn't think it would be considered a bug. The code for cycling, first hides the currently visible window and then shows the new window leaving a fraction of a second where there is no window visible. Could it be that the severity of the delay is caused by caching in EDIT 1:
Strange you're not getting any logs from scratchpad related commands. Could it be because the log level was to low? I normally get the messages by starting |
I was running this on a full on live session, as I had a couple of things lately that didn't work the same in Xephyr and a native session, and I was logging to systemd for convenience. I'll give it a shot to reproduce in Xephyr tomorrow. |
|
@SamClercky just had an idea but can't verify right now (still at work): |
|
Maybe we merge this now and open an issue to do some refactor to recreate 4e48f7f |
|
Something not yet mentioned is that the bug for me triggers on an empty SP ( I am ok with merging now and creating an open issue. Then other PR's won't be blocked by this one and I can try and start working on a new PR that fixes this last issue when I have some more time on my hands (school is restarting soon). |
|
Thanks @SamClercky for all the work you put into this. Looking forward to any further contributions! Also thanks @TornaxO7 for your review, I guess next will be the logging PR. |
This was referenced Sep 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
This is for an issue that I had when using a scratchpad: If I wanted to move the window in the scratchpad to one of my tags, Leftwm would still remember the window as being in the scratchpad.
This PR removes the moved window from the active scratchpad with a new command
ReleaseScratchPad.There are no extra dependencies or configs (in my opinion not needed) added.In the meantime this PR adds 3 additional commands on top of
ReleaseScrachPad:AttachScratchPad,NextScratchPadWindowandPrevScratchPadWindow. It also adds the ability for a scratchpad to contain more than 1 window, but restricts its use to a stack of windows that can be paged with the added next/prev commands.Fixes #(issue): I couldn't find one, but if needed, I can create one for better visibility.
Type of change
Updated user documentation:
Keybind
ReleaseScratchPad
Moves the window in a scratchpad to a tag and removes the empty scratchpad.
The value can be one of the following:
<tag id>: Move the currently focused scratchpad to the tag with<tag id><scratchpad name>: Move the window in the scratchpad with the name<scratchpad>to the current tag even if the scratchpad is not currently visible.Example:
IN TOML
IN RON
Another use case is to move the window in a scratchpad to a specific tag and removing the empty scratchpad
in 1 keybinding:
IN TOML
IN RON
AttachScratchPad
Attaches the currently selected window to a scratchpad. The value needs to be the name of the scratchpad to which the selected window will be attached. This command makes it possible to have more than 1 window in a scratchpad.
Example:
IN TOML
IN RON
NextScratchPadWindow/PrevScratchPadWindow
Cycles through the scratchpad windows. The value needs to be a valid scratchpad name. Note that this command will be ignored if the given scratchpad is not visible.
Example:
IN TOML
IN RON
External Commands
scratchpad name,tag idor no argumentscratchpad nametotag idor the current tag if nothing provided as argumentscratchpad namescratchpad namescratchpad nameNote: Manual page changes must be performed in a commit, not in this PR section.
Checklist:
make testlocally with no errors or warnings reportedcargo clippy -- -W clippy::pedantic -A clippy::must_use_candidate -A clippy::cast_precision_loss -A clippy::cast_possible_truncation -A clippy::cast_possible_wrap -A clippy::cast_sign_loss -A clippy::mut_mutEDIT 1: added documentation for new command suggestion
EDIT 2: added documentation for
AttachScratchPad-,NextScratchPadWindow- andPrevScratchPadWindowcommands + suggestionsEDIT 3: Translate keybind config to the new RON format + extra info about this PR in the description