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

compat: add window_properties object to json output of views #2911

Closed
wants to merge 1 commit into from

Conversation

Snaipe
Copy link
Contributor

@Snaipe Snaipe commented Oct 21, 2018

Some existing tools that rely on i3's json output, like the i3ipc
python module and scripts using it, break when they can't find the
window_properties object in a view.

To achieve a better drop-in replacement compatibility with i3 and
existing tools for it, we add the window_properties object to
the output of get_tree, along with the "class", "instance", and "title"
sub-fields. These three values are set to the view's class, app_id,
and title respectively.

In addition, if the view does not have a class, window_properties.class
is set to the app_id, to avoid breaking tools that expect all windows
to have a class in the X world.

Signed-off-by: Franklin "Snaipe" Mathieu me@snai.pe

Some existing tools that rely on i3's json output, like the i3ipc
python module and scripts using it, break when they can't find the
window_properties object in a view.

To achieve a better drop-in replacement compatibility with i3 and
existing tools for it, we add the window_properties object to
the output of get_tree, along with the "class", "instance", and "title"
sub-fields. These three values are set to the view's class, app_id,
and title respectively.

In addition, if the view does not have a class, window_properties.class
is set to the app_id, to avoid breaking tools that expect all windows
to have a class in the X world.

Signed-off-by: Franklin "Snaipe" Mathieu <me@snai.pe>

// Some tools for i3 break when windows don't have a class.
if (!class) {
class = app_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

This fallback isn't performed for the same property above. Should 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.

Yeah, I initially put that at the top, but wasn't sure if that was completely the right fix. I guess we can have this fallback in place for both and see if anything breaks down the line.

Copy link
Member

@RyanDwyer RyanDwyer Oct 21, 2018

Choose a reason for hiding this comment

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

-1 to this. Loading the app id into the class or instance fields makes it confusing to end users. I value transparency - that is: telling the user what the properties actually are - over trying to allow for a zero configuration port between i3 and sway. I would add an app_id property, set it accordingly, and let the class and instance be null. This is the approach we've taken with criteria too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have agreed with that point if sway didn't try to pretend it was i3: by providing I3SOCK, you're pretty much promising that existing programs written for i3 are still going to work under sway, which is not really true as it is.

Would it be reasonable to stop populating I3SOCK altogether, or alternatively set it to a socket serving an i3-matching fallback protocol? I'm fine with either, but keeping the status-quo is confusing and against the transparency you advocate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, sorry for the spam, looks like GitHub is choking on requests right now.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a problem with setting I3SOCK because there's no bad consequences for doing it. But putting the app id into the class and instance is a decision that we could be forced to live with for a while, can confuse and frustrate users, and will become a legacy (especially when users no longer run Xwayland).

Copy link
Contributor

Choose a reason for hiding this comment

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

As a compromise, maybe set the class to wayland if it is a native wayland window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. I somewhat find it just as frustrating to see every single ipc script that check the instance or the class of a window completely break, but I can see why you wouldn't want that in sway. I just hoped that sway would own being its own thing for the sake of consistency.

I'll just write a standalone ipc proxy providing an I3SOCK socket to convert from the sway-flavored json to the i3-flavored one in the meantime.

tl;dr: Dropping this PR; window_properties are very much more so a compatibility thing with i3 rather than something critical.

@Snaipe Snaipe closed this Oct 22, 2018
Snaipe added a commit to Snaipe/sway that referenced this pull request Oct 22, 2018
window_properties is documented to contain a subset of the X11 properties
of a window (its title, class, instance, role, and transient ID). This
commit adds the missing json object from the get_tree output for
xwayland windows only.

This is a follow-up of swaywm#2911.

Signed-off-by: Franklin "Snaipe" Mathieu <me@snai.pe>
Snaipe added a commit to Snaipe/sway that referenced this pull request Oct 22, 2018
window_properties is documented to contain a subset of the X11 properties
of a window (its title, class, instance, role, and transient ID). This
commit adds the missing json object from the get_tree output for
xwayland windows only.

This is a follow-up of swaywm#2911.

Signed-off-by: Franklin "Snaipe" Mathieu <me@snai.pe>
Snaipe added a commit to Snaipe/sway that referenced this pull request Oct 22, 2018
window_properties is documented to contain a subset of the X11 properties
of a window (its title, class, instance, role, and transient ID). This
commit adds the missing json object from the get_tree output for
xwayland windows only.

This is a follow-up of swaywm#2911.

Signed-off-by: Franklin "Snaipe" Mathieu <me@snai.pe>
Snaipe added a commit to Snaipe/sway that referenced this pull request Oct 23, 2018
window_properties is documented to contain a subset of the X11 properties
of a window (its title, class, instance, role, and transient ID). This
commit adds the missing json object from the get_tree output for
xwayland windows only.

This is a follow-up of swaywm#2911.

Signed-off-by: Franklin "Snaipe" Mathieu <me@snai.pe>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants