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

Protect window_switcher from split errors #126

Closed
wants to merge 1 commit into from

Conversation

xdg
Copy link

@xdg xdg commented Oct 19, 2021

This commit stops window_switcher from erroring when the wm_class has
more or less than a single . character. For example, here are two
problematic lines of outputs from wmctrl -l -x where the wm_class is
N/A or rufus-3.17.exe.rufus.3.17.exe (from Wine):

0x01a00078  1 N/A                            remus Ozone X11
0x03e00003  2 rufus-3.17.exe.rufus-3.17.exe  remus Rufus 3.17.1846

If there are no dots, the entry is skipped and not matched. If there is
more than one dot, the part before the first dot is used for the display
name (e.g. "rufus-3").

@squalou
Copy link

squalou commented Oct 23, 2021

Hi, I do have a concern about this fix.
It sures prevents the crash but wine applications are badly displayed :

image

it comes from the spit, original string is for instance program.exe.program.exe

==> win_class_parts[1] in non-wine cases should be replaced by win_class_parts[2] depending on win_class_parts length. (which I don't know how to write in a nice one-liner)

maybe this one

win_class = len(win_class_parts) > 2 and win_class_parts[2] or win_class_parts[1]

is better

image

This commit stops window_switcher from erroring when the wm_class has
more or less than a single `.` character.  For example, here are two
problematic lines of outputs from `wmctrl -l -x` where the `wm_class` is
`N/A` or `rufus-3.17.exe.rufus.3.17.exe` (from Wine):

    0x01a00078  1 N/A                            remus Ozone X11
    0x03e00003  2 rufus-3.17.exe.rufus-3.17.exe  remus Rufus 3.17.1846

If there are no dots, the entry is skipped and not matched.  If there is
more than one dot, the part before the first dot is used for the display
name (e.g. "rufus-3").
@xdg xdg force-pushed the window_switcher branch from 1307610 to 21273c7 Compare October 25, 2021 02:06
@xdg
Copy link
Author

xdg commented Oct 25, 2021

I've revised the approach to skip when there are zero dots and take only the first segment when there are more than one.

In my Wine testing, I happened to find rufus-3.17.exe.rufus-3.17.exe. I don't think there's any good way to be clever short of trying to pattern match .exe, and even that is probably brittle. Using the first segment rufus-3 is probably a reasonable heuristic that shouldn't be too confusing.

@ManuelSchneid3r
Copy link
Member

@edjoperez @dshoreman can you please review?

@xdg
Copy link
Author

xdg commented Jan 5, 2022

Ping. This has been open for a couple months. I'm sure people are busy, but would it possible to get a review soon, please? It's only ~20 lines and half of it is a comment explaining the behavior.

@tomsquest
Copy link
Contributor

LGTM

@ManuelSchneid3r
Copy link
Member

ManuelSchneid3r commented Jan 6, 2022

The 0.18 release will probably need changes. Since there will be no release in between I'll wait with the merge. Ty for your time. Ill comment when it is released

@ManuelSchneid3r
Copy link
Member

0.18 is out. Please check the new api . Do you mind to volunteer as a maintainer for the plugin?

@xdg
Copy link
Author

xdg commented Jan 3, 2023

I'm not really a Python person, nor do I have the time to maintain it even if I did. I was really just hoping to get this tiny change applied.

@dshoreman
Copy link
Contributor

dshoreman commented Jan 30, 2023

@edjoperez @dshoreman can you please review?

It's been over a year, but if my two cents are still desired I would probably move most of the new code into the parseWindow method, potentially modifying the named tuple to have an extra key or two if needed. That would allow the loop itself to stay (relatively) clean/simple and keeping all the parsing bits in one place.

I won't be able to test though, unfortunately. While I did contribute several fixes to this plugin via a partial rewrite in ed2778c, that was back when I was under the impression the license was GPL and I have since moved to an alternative launcher.

Sorry I haven't had the time to respond sooner.

@MABIY
Copy link

MABIY commented Feb 2, 2023

please continue

@ManuelSchneid3r
Copy link
Member

I'm not really a Python person, nor do I have the time to maintain it even if I did. I was really just hoping to get this tiny change applied.

The problem today is that this code is incompatible with recent versions of albert. Ill close this PR since this plugin has been not been ported yet.

Id appreciate if anybody actually using it could port it.

Regards

@albertlauncher albertlauncher locked and limited conversation to collaborators Aug 29, 2023
@ManuelSchneid3r
Copy link
Member

Please continue any discussion here: https://github.com/orgs/albertlauncher/discussions/1299

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants