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: add more ipc events #2252

Merged
merged 12 commits into from
Mar 24, 2024
Merged

ipc: add more ipc events #2252

merged 12 commits into from
Mar 24, 2024

Conversation

CharlieQLe
Copy link
Contributor

No description provided.

@CharlieQLe CharlieQLe changed the title ipc: add more view ipc events ipc: add more ipc events Mar 23, 2024
@killown
Copy link
Contributor

killown commented Mar 23, 2024

because "view-focused" event, could it be "output-focused" instead of "output-gain-focus"?

@CharlieQLe
Copy link
Contributor Author

because "view-focused" event, could it be "output-focused" instead of "output-gain-focus"?

I kept it as output-gain-focus because the signal in the codebase is named as such. I'm not against renaming it to output-focused though.

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.

In general LGTM. I wonder whether for all of these signals we also should send the old state (for example view-workspace-changed, also include the old workspace in the response).

Also since you're adding quite a lot of IPC stuff lately, do you have a use-case for all of these signals? What do you need them for?

@CharlieQLe
Copy link
Contributor Author

In general LGTM. I wonder whether for all of these signals we also should send the old state (for example view-workspace-changed, also include the old workspace in the response).

I think including the old workspace would be ideal, as only providing the ID or index requires an additional IPC call just to get the view/output/workspace set that is going to be used.

Also since you're adding quite a lot of IPC stuff lately, do you have a use-case for all of these signals? What do you need them for?

Writing my own bar using Typescript, so the IPC is useful to have for stuff like a focused window widget, workspace widget, etc, since GJS doesn't have any access to Wayland protocols.

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

In general LGTM. I wonder whether for all of these signals we also should send the old state (for example view-workspace-changed, also include the old workspace in the response).

I think including the old workspace would be ideal, as only providing the ID or index requires an additional IPC call just to get the view/output/workspace set that is going to be used.

Also since you're adding quite a lot of IPC stuff lately, do you have a use-case for all of these signals? What do you need them for?

Writing my own bar using Typescript, so the IPC is useful to have for stuff like a focused window widget, workspace widget, etc, since GJS doesn't have any access to Wayland protocols.

Alright. Feel free to extend the signals with the previous state as well, and we can merge when you think it is ready.

@CharlieQLe
Copy link
Contributor Author

Alright. Feel free to extend the signals with the previous state as well, and we can merge when you think it is ready.

I also committed a change to send the json data for views, outputs, and workspace sets as well, I believe this would be more useful than simply sending the id for the associated data, since it can reduce an IPC call. I can undo this though if you feel the id should be sent instead.

@CharlieQLe CharlieQLe marked this pull request as draft March 24, 2024 10:16
@CharlieQLe
Copy link
Contributor Author

Converted this PR to a draft since I've yet to do a final pass over any IPC functions should have extra return values.

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

I also committed a change to send the json data for views, outputs, and workspace sets as well, I believe this would be more useful than simply sending the id for the associated data, since it can reduce an IPC call. I can undo this though if you feel the id should be sent instead.

I think in this respect we ought to figure out a way to only construct the json object if there are any connected clients, because constructing it of course comes with a higher CPU overhead, even if no IPC client is currently connected and listening for the corresponding object.

@CharlieQLe
Copy link
Contributor Author

I think in this respect we ought to figure out a way to only construct the json object if there are any connected clients, because constructing it of course comes with a higher CPU overhead, even if no IPC client is currently connected and listening for the corresponding object.

Perhaps we could check clients.empty() before making any JSON data and do nothing if no client is connected for anything that would force a watch event.

@killown
Copy link
Contributor

killown commented Mar 24, 2024

actually you can build an output_gain_focus through ipc already, workspace with no views will be like {'event': 'view-focused', 'view': None}
you just need to do output_gain_focus = socket.get_focused_output() every view-focused

@CharlieQLe
Copy link
Contributor Author

actually you can build an output_gain_focus through ipc already, workspace with no views will be like {'event': 'view-focused', 'view': None} you just need to do output_gain_focus = socket.get_focused_output() every view-focused

I'd prefer a more explicit event, rather than having something that looks like a workaround or requiring an extra IPC call.

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

actually you can build an output_gain_focus through ipc already, workspace with no views will be like {'event': 'view-focused', 'view': None} you just need to do output_gain_focus = socket.get_focused_output() every view-focused

Output focus can happen even if there are no views in which case only the explicit signal tells you what is going on.

@CharlieQLe CharlieQLe marked this pull request as ready for review March 24, 2024 11:27
@CharlieQLe
Copy link
Contributor Author

This should be good to go, if no other changes need to be made. I have yet to test this personally, since I won't be able to actually replace my running session ATM, but I can't imagine it would cause any problems.

@killown
Copy link
Contributor

killown commented Mar 24, 2024

two events will happen with your pr while focusing (mouse click) an output with no views, {'event': 'view-focused', 'view': None} and {'event': 'output-gain-focus'...}
unless you replaced {'event': 'view-focused', 'view': None} with {'event': 'output-gain-focus'...}

@CharlieQLe
Copy link
Contributor Author

two events will happen with your pr while focusing (mouse click) an output with no views, {'event': 'view-focused', 'view': None} and {'event': 'output-gain-focus'...} unless you replaced {'event': 'view-focused', 'view': None} with {'event': 'output-gain-focus'...}

That's just going to happen, since both events can fire at different times. Events will obviously overlap in situations like that, but that's just how it works. view-focused obviously should be run when a new or null view is focused, which is going to happen when moving to a separate monitor with no (toplevel?) views, and output-gain-focus is going to run when focusing on a new output, windows or no windows.

@killown
Copy link
Contributor

killown commented Mar 24, 2024

what I am saying here is {'event': 'view-focused', 'view': None} is doing the same as output-gain-focus, this event is triggered in an output with no views, (when you click in the output wallpaper), I am just trying to understand what will be the meaning and use case for {'event': 'view-focused', 'view': None} after your pr

@CharlieQLe
Copy link
Contributor Author

what I am saying here is {'event': 'view-focused', 'view': None} is doing the same as output-gain-focus, this event is triggered in an output with no views, (when you click in the output wallpaper), I am just trying to understand what will be the meaning and use case for {'event': 'view-focused', 'view': None} after your pr

If you close the last window on a workspace for example. It would fire off view-focused with a null view, since no views are left to be focused. In my own bar, my focused window widget is hidden when no windows are being focused (think GNOME's AppMenu before it got removed in GNOME 45). It's only use-case isn't just for tracking focused outputs after all- which it isn't ideal at anyways since it requires an additional IPC call to get the focused output data.

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

It's only use-case isn't just for tracking focused outputs after all- which it isn't ideal at anyways since it requires an additional IPC call to get the focused output data.

Yeah and you explicitly cannot use view-focused in all cases to know which output was focused. Imagine I have 3 outputs, only 1 has a view, now I click on one of the other two outputs => how would I know which one was focused? Or even if you had just two outputs with no views on each, the focused output might change when you click on the background of some of them, but no view will be focused, so you won't get an event at all. The view-focused signal simply keeps track of exactly what it said: whether a certain view has focus or not, this is a close, but not the same concept as focused output.

@CharlieQLe
Copy link
Contributor Author

CharlieQLe commented Mar 24, 2024

There's probably more breaking changes that I'll have to "undo", or at least double check before this is ready to merge.

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

I've been thinking about efficiency and maybe we ought to make it so that the signal handlers are connected/disconnected 'on demand': that is, we don't connect initially, only when an ipc client connects. But we can add that at a later PR, let's not group too many changes together.

@killown
Copy link
Contributor

killown commented Mar 24, 2024

what I am saying here is {'event': 'view-focused', 'view': None} is doing the same as output-gain-focus, this event is triggered in an output with no views, (when you click in the output wallpaper), I am just trying to understand what will be the meaning and use case for {'event': 'view-focused', 'view': None} after your pr

If you close the last window on a workspace for example. It would fire off view-focused with a null view, since no views are left to be focused. In my own bar, my focused window widget is hidden when no windows are being focused (think GNOME's AppMenu before it got removed in GNOME 45). It's only use-case isn't just for tracking focused outputs after all- which it isn't ideal at anyways since it requires an additional IPC call to get the focused output data.

if no views are being left to be focused and you have an applet to close views, and you closed the last view with that applet, what you get instead is 'role': 'desktop-environment', so you are just producing a bug in some situations, it's better to check if there views left in workspace than rely on this event

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

if no views are being left to be focused and you have an applet to close views, and you closed the last view with that applet, what you get instead is 'role': 'desktop-environment', so you are just producing a bug in some situations, it's better to check if there views left in workspace than rely on this event

@killown I think you're making too many assumptions, you're not guaranteed to get a desktop-environment view focused, because maybe there aren't any such views :)

@killown
Copy link
Contributor

killown commented Mar 24, 2024

if no views are being left to be focused and you have an applet to close views, and you closed the last view with that applet, what you get instead is 'role': 'desktop-environment', so you are just producing a bug in some situations, it's better to check if there views left in workspace than rely on this event

@killown I think you're making too many assumptions, you're not guaranteed to get a desktop-environment view focused, because maybe there aren't any such views :)

if you click in any layershell you get that instead, unless you are closing views outside the layershell

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

if no views are being left to be focused and you have an applet to close views, and you closed the last view with that applet, what you get instead is 'role': 'desktop-environment', so you are just producing a bug in some situations, it's better to check if there views left in workspace than rely on this event

@killown I think you're making too many assumptions, you're not guaranteed to get a desktop-environment view focused, because maybe there aren't any such views :)

if you click in any layershell you get that instead, unless you are closing views outside the layershell

That will not be the case if your layer-shell does not have any keyboard interactivity set.

@CharlieQLe
Copy link
Contributor Author

The breaking changes should be accounted for now, but I might have missed something.

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

The breaking changes should be accounted for now, but I might have missed something.

Ok, I'll take a look too :)

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.

One small thing which I think is unnecessary, otherwise LGTM.

plugins/single_plugins/ipc-rules.cpp Outdated Show resolved Hide resolved
@CharlieQLe
Copy link
Contributor Author

Should be good to go now! LGTM!

@ammen99
Copy link
Member

ammen99 commented Mar 24, 2024

Should be good to go now! LGTM!

LGTM too, thanks :)

@ammen99 ammen99 merged commit b2ad130 into WayfireWM:master Mar 24, 2024
4 checks passed
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.

3 participants