Skip to content
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

fix: setWindowState when target_space is empty #2

Open
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

shailesh17
Copy link

  • Fix crash when target space is empty during setting of window state
  • also adding default excludes for vscode editor in .gitignore

@tplobo tplobo self-requested a review November 3, 2024 15:25
@tplobo tplobo self-assigned this Nov 3, 2024
Copy link
Owner

@tplobo tplobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @shailesh17, thanks for the support! Good bug-catch! I added a minor modification suggestion to your request.

restore_spaces/rs/plumbing.lua Show resolved Hide resolved
@shailesh17 shailesh17 changed the title fix setWindowState when target_space is empty fix: setWindowState when target_space is empty Nov 3, 2024
Copy link
Owner

@tplobo tplobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the re-review; I have checked everything properly now. Once you implement the comments I will merge!

restore_spaces/rs/plumbing.lua Outdated Show resolved Hide resolved
restore_spaces/rs/porcelain.lua Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Copy link
Owner

@tplobo tplobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @shailesh17, thanks again for the support;
unfortunately testing the code after your changes leads to a bit of a bug, so another modification is needed

rs.delayExecution(0.4)
window:moveToScreen(target_screen)
window:focus()
rs.issueVerbose("target space undefined; unable to set window " .. window:id(), rs.verbose)
end

rs.setFrameState(window, frame_state, fullscreen_state)
Copy link
Owner

@tplobo tplobo Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not comply with the modifications...
both setFrameState and issueVerbose("set window"...) should only be called if target_space was identified, so they should be moved into the if statement (or maybe using a return)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're at it, can you also try to change the "target space undefined..." line to keep it under 72 characters?

In this code I have been using something like the following:

local msg = "target space undefined; "
msg = msg .. "unable to set window "
rs.issueVerbose(msg .. window:id(), rs.verbose)

restore_spaces/rs/plumbing.lua Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants