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

ipc: support workspace sets #2156

Merged
merged 12 commits into from
Feb 24, 2024
Merged

ipc: support workspace sets #2156

merged 12 commits into from
Feb 24, 2024

Conversation

CharlieQLe
Copy link
Contributor

This is a draft PR for an implementation of querying and configuring workspace sets via IPC. This would be a way to interface with workspaces via bars such as waybar or window rule scripts specific to workspaces. Leaving it as a draft for now since I am unsure if this should live in wsets or ipc-rules. For now, it lives in ipc-rules. This is a very quick, very untested implementation as I do not have time at the moment to build and test the plugin.

This would likely require signals for seeing if a workspace set is created or destroyed. I'm not too sure if they already exist, but this would require more signals to be defined I imagine, if they aren't defined already.

Views should also know which workspace set and viewport they exist on, but I don't know if that would complicate anything in the codebase, or if that is considered out of scope for this PR. On paper, listing views and specifying the workspace set id and/or viewport position should list all the views that exist on that workspace set (and viewport if specified). This would make it less simple for scripts to know which workspace set they exist on however- an example would be seeing if a view's title change is on the second workspace. One would have to list all the views that exist on the second workspace in order to check if the view is indeed on it whereas ideally, one could simply check properties such as workspace-set-id and workspace-point. Again, that might be out of scope for this specific PR as it doesn't necessarily pertain to interfacing with workspace sets.

@ammen99
Copy link
Member

ammen99 commented Feb 20, 2024

Hi and thanks for starting the work on this. A few things:

  • I don't think we ought to filter views in the plugin. Instead, we should return the full list of views, on all workspaces/workspace-sets. We should indicate the wset id for each view (if the view is on a wset id) - this should be as simple as adding a new field to the view_to_json function. Then the ipc client can do the filtering on its own.

  • A view is not really 'on' a particular viewport/workspace/however you call it, Views can be visible on multiple of those. Again, I think it is better that we don't have a bigger API than we need. If ipc scripts need to query something in particular, they can easily go through all views and check whether they fit/overlap a particular workspace rectangle.

  • About the signal for output-workspace-changed: this signal should be wset-workspace-changed, because workspaces change within a workspace set, not within an output.

  • About workspace-set-info and output-info commands: with your patch they have a different syntax for the workspace information, output-info has output[workspace][x/y/width/height], I'd prefer workspace-set-info to report in the same format.

@CharlieQLe
Copy link
Contributor Author

@ammen99 Updated ipc-rules to reflect your comments on this.

About workspace-set-info and output-info commands: with your patch they have a different syntax for the workspace information, output-info has output[workspace][x/y/width/height], I'd prefer workspace-set-info to report in the same format.

Does the output data need to contain any workspace information except for the workspace set's id? It could easily just provide the id, and scripts can query for the workspace set info itself afterwards with that. Right now, the output data still supplies the workspace data to clients.

@ammen99
Copy link
Member

ammen99 commented Feb 21, 2024

@ammen99 Updated ipc-rules to reflect your comments on this.

About workspace-set-info and output-info commands: with your patch they have a different syntax for the workspace information, output-info has output[workspace][x/y/width/height], I'd prefer workspace-set-info to report in the same format.

Does the output data need to contain any workspace information except for the workspace set's id? It could easily just provide the id, and scripts can query for the workspace set info itself afterwards with that. Right now, the output data still supplies the workspace data to clients.

Yes, that's true but I'd like to avoid breaking changes - and I guess that this is information which would be commonly requested so I think overall it is better to spare the additional IPC call needed otherwise.

@ammen99
Copy link
Member

ammen99 commented Feb 21, 2024

Took a brief look over the PR, looks much better now :) I'll do a full review on the weekend & merge if everything is fine.

@CharlieQLe
Copy link
Contributor Author

CharlieQLe commented Feb 21, 2024

Took a brief look over the PR, looks much better now :) I'll do a full review on the weekend & merge if everything is fine.

Sounds good. I just added configure_output and configure_wset as well, allowing scripts to manipulate workspace sets.

EDIT: I also have to build and try it for myself :) Still haven't done that yet!

@CharlieQLe CharlieQLe marked this pull request as ready for review February 21, 2024 16:46
@ammen99
Copy link
Member

ammen99 commented Feb 21, 2024

Sounds good. I just added configure_output and configure_wset as well, allowing scripts to manipulate workspace sets.

I think this features should belong to the wsets plugin though, since it can also create/destroy wsets and already has all the logic for moving wsets around (and also I think it keeps an internal mapping of wsets). It can also show a popup when the wset changes (which I guess should happen also for ipc-triggered switches).

@CharlieQLe
Copy link
Contributor Author

Sounds good. I just added configure_output and configure_wset as well, allowing scripts to manipulate workspace sets.

I think this features should belong to the wsets plugin though, since it can also create/destroy wsets and already has all the logic for moving wsets around (and also I think it keeps an internal mapping of wsets). It can also show a popup when the wset changes (which I guess should happen also for ipc-triggered switches).

Good point! I'll revert the commit for now and look into adding this feature before the merge, if it is in scope for this PR.

@ammen99
Copy link
Member

ammen99 commented Feb 21, 2024

Good point! I'll revert the commit for now and look into adding this feature before the merge, if it is in scope for this PR.

I don't personally mind merging either two separate PRs or one bigger PR, feel free to organize it as you wish.

plugins/single_plugins/wsets.cpp Outdated Show resolved Hide resolved
plugins/single_plugins/wsets.cpp Outdated Show resolved Hide resolved
plugins/single_plugins/wsets.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

I pushed one small commit with a few smaller fixes and also made it so that when selecting a workspace set on an output which doesn't exist, the workspace set with the given index will be created. This matches the behavior of the bindings that the wset plugin provides - otherwise we'd need a dedicated command for creating a wset which seems clumsy.

Otherwise LGTM, thanks!

@ammen99 ammen99 merged commit 2e0926f into WayfireWM:master Feb 24, 2024
4 checks passed
ammen99 added a commit that referenced this pull request Mar 13, 2024
* Basic untested support for ipc workspaces

* Updated ipc-rules

* Fix find_workspace_set_by_id

* Added configure_output and configure_wset

* Include workspace-set.hpp in ipc-helpers

* Revert "Added configure_output and configure_wset"

This reverts commit 36d5adf.

* Check code style with uncrustify

* Add ipc wset manipulation in wsets plugin

* Updated ipc plugins

* small fixes

---------

Co-authored-by: Ilia Bozhinov <ammen99@gmail.com>
@bluebyt
Copy link

bluebyt commented Mar 18, 2024

I would like to switch between workspace in a bar.
Can I have an example of a script to do that?

@ammen99
Copy link
Member

ammen99 commented Mar 19, 2024

@bluebyt Workspace or workspace set? Here is an example for workspaces using the vswitch plugin:

from wayfire_socket import WayfireSocket, get_msg_template
import os

addr = os.getenv('WAYFIRE_SOCKET')
sock = WayfireSocket(addr)

msg = get_msg_template('vswitch/set-workspace')
msg['data']['x'] = x
msg['data']['y'] = y
msg['data']['output-id'] = output-id
sock.send_json(msg)

For reference:

ipc_repo->register_method("vswitch/set-workspace", request_workspace);

wf::ipc::method_callback request_workspace = [=] (const nlohmann::json& data)
{
WFJSON_EXPECT_FIELD(data, "x", number_unsigned);
WFJSON_EXPECT_FIELD(data, "y", number_unsigned);
WFJSON_EXPECT_FIELD(data, "output-id", number_unsigned);

@bluebyt
Copy link

bluebyt commented Mar 19, 2024

@ammen99 Something like this, all workspace are displayed in the bar with numbers or icons and with the mouse you can navigate between them by clicking or scrolling on the number in the bar.
20240319_032522

I don't known what is workspace set?

I tried the script on the terminal, but I get multiple error, my system should be all set, I can run other script like inactive-alpha.py without any error.

./workspace.py: line 1: from: command not found
import: unable to grab mouse '': Resource temporarily unavailable @ error/xwindow.c/XSelectWindow/9354.
import: unable to read X window image '': Success @ error/xwindow.c/XImportImage/4961.
import: unable to read X window image '': Resource temporarily unavailable @ error/xwindow.c/XImportImage/5068.
import: os' @ error/import.c/ImportImageCommand/1289. ./workspace.py: line 4: syntax error near unexpected token ('
./workspace.py: line 4: `addr = os.getenv('WAYFIRE_SOCKET')'

@ammen99
Copy link
Member

ammen99 commented Mar 19, 2024

@bluebyt In case you're not looking at the matrix chat .. maybe you need #!/bin/env python3 at the start?

@bluebyt
Copy link

bluebyt commented Mar 19, 2024

After added the line "#!/bin/env python3" I have less error.
Traceback (most recent call last):
File "/home/bluebyt/.config/ipc-scripts/./workspace.py", line 9, in
msg['data']['x'] = x
^
NameError: name 'x' is not defined

@ammen99
Copy link
Member

ammen99 commented Mar 19, 2024

Yes, replace x/y and the output-id appropriately xD

@bluebyt
Copy link

bluebyt commented Mar 19, 2024

What is output-id? I tried this HDMI-A-1, but I get an error.
I have only one monitor, I guess I set it number 1 .
that working!

@ammen99
Copy link
Member

ammen99 commented Mar 19, 2024

You can see a list of available output by making a query window-rules/list-outputs, no arguments needed. Then you'll get in return a json object with a list of all outputs with their names and IDs

@killown
Copy link
Contributor

killown commented Mar 19, 2024

also you can try: pip install wayfire

from wayfire import sock
focused_view_id = sock.get_focused_view()["id"]
sock.set_workspace({"x":1, "y":1}, focused_view_id)

# or not moving the view and yet going to the workspace
# sock.set_workspace({"x":1, "y":1})

@bluebyt
Copy link

bluebyt commented Mar 19, 2024

Thanks @ammen99 and @killown I will play around with this!

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.

4 participants