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 special workspaces bug on expose.py #120

Closed
wants to merge 5 commits into from

Conversation

axelKeizoStahl
Copy link
Contributor

@axelKeizoStahl axelKeizoStahl commented Jul 6, 2024

#119
The solution was to use the name of the workspace instead of the id.
The id of special workspaces are negative, and using a negative workspace for moveworkspacesilent does not work.
Since the name the workspace returns the id (if it is normal) or the name (if it is a special workspace), using name is more desirable.

Copy link
Collaborator

@fdev31 fdev31 left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!
I made some comments which needs clarification before I merge it.

@@ -760,7 +760,7 @@ async def _handle_focus_tracking(self, scratch: Scratch, active_window: str, act
if tracker and not tracker.prev_focused_window_wrkspc.startswith("special:"):
same_workspace = tracker.prev_focused_window_wrkspc == active_workspace
if scratch.have_address(active_window) and same_workspace and not scratch.have_address(tracker.prev_focused_window):
await self.hyprctl(f"focuswindow address:{tracker.prev_focused_window}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change to scratchpads intended ?
(it'as about an expose bug... isn't it ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, it seems I pushed to main instead of a branch. You're right, this was totally an accident! I will fix it soon. 👍

@@ -23,7 +23,10 @@ async def run_expose(self) -> None:
If expose is active restores everything and move to the focused window
"""
if self.exposed:
commands = [f"movetoworkspacesilent {client['workspace']['id']},address:{client['address']}" for client in self.exposed_clients]
commands = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

you may want to run "ruff format" on the files, it's part of the pre-commit hooks in theory...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, it should be all good now 👍 .

@fdev31
Copy link
Collaborator

fdev31 commented Jul 8, 2024

I think the other branch which has been merged just now had the two set of changes... can you please review it's "as expected" ?

@axelKeizoStahl
Copy link
Contributor Author

By the two set of changes, do you mean the fix for #118 and #119?
After the merge of the other PR, #118 is fixed, but #119 is not, since line 26 on expose.py still uses "id" instead of "name".
That being said, I found a small inaccuracy that would lead to a NoneType object is not subscriptable error. I should be able to PR the fix in around an 1.5 hours.

@fdev31
Copy link
Collaborator

fdev31 commented Jul 8, 2024

By the two set of changes, do you mean the fix for #118 and #119?
After the merge of the other PR, #118 is fixed, but #119 is not, since line 26 on expose.py still uses "id" instead of "name".
That being said, I found a small inaccuracy that would lead to a NoneType object is not subscriptable error. I should be able to PR the fix in around an 1.5 hours.

That's what I meant. Thanks for the clarity. I'll only have a look tomorrow, so there's no rush.
Thank you again for the care!

@fdev31
Copy link
Collaborator

fdev31 commented Jul 11, 2024

What about this PR ?

@axelKeizoStahl
Copy link
Contributor Author

This now works for special workspaces. There is a problem with pyprland scratchpads in this, since it tries to go to another workspace, but I can open another PR/issue for that. This PR should be good to merge/it fixes the issue.

@fdev31
Copy link
Collaborator

fdev31 commented Jul 12, 2024

There are some issues applying the main branch, maybe related to the cherry picks, I get an infinite loop solving them.
The only change I see is already applied in the official repo (I see only this change: main...axelKeizoStahl:pyprland:main).

@axelKeizoStahl
Copy link
Contributor Author

I just synced my fork, are you able to see the diff now?

@fdev31
Copy link
Collaborator

fdev31 commented Jul 15, 2024

I can see the diff, np, what I'm saying is:

  • it looks already applied into main branch
  • it is marked as conflict
  • git will not be happy with a rebase of it (it "looped" with the same merge conflict)

I see you use merge, while I try to have a clean history with rebase only... that could be one of the problems. But is this change still relevant if we have similar code already merged into main?

PS: I saw you did one more merge, but it's still can't be merged... something is fishy in your main branch, having clean commits rebased on top of upstream would be ideal (if this PR is still relevant)

@axelKeizoStahl
Copy link
Contributor Author

Sorry, I almost always use branches and not forks. My fork is pretty messed up, I think I will just make a new one.
However, the only intended diff here was to change from id to name on line 26 of expose.py (in the current main branch). This would let you go into special workspaces when you go to them through expose.
I will probably just delete my current fork and make a new one, then pr this and also the other pr I have open.
Sorry for the trouble. 👍

@fdev31
Copy link
Collaborator

fdev31 commented Jul 15, 2024

I looked at your main branch: it became very messy...
I would suggest you prune it and fetch it from the upstream again...
I cherry-picked the commit implementing it, tell me if something is missing on the official main branch.
If none, we can close this bug, thanks!

@fdev31
Copy link
Collaborator

fdev31 commented Jul 16, 2024

I double checked, expose is only using name but to filter the list of exposed clients.
If there is anything left, please reopen, I'm closing it!
Thank you for the PR!

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