python icon indicating copy to clipboard operation
python copied to clipboard

Protect window_switcher from split errors

Open xdg opened this issue 2 years ago • 6 comments

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 avatar Oct 19 '21 21:10 xdg

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

squalou avatar Oct 23 '21 14:10 squalou

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.

xdg avatar Oct 25 '21 02:10 xdg

@edjoperez @dshoreman can you please review?

ManuelSchneid3r avatar Nov 07 '21 18:11 ManuelSchneid3r

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.

xdg avatar Jan 05 '22 16:01 xdg

LGTM

tomsquest avatar Jan 05 '22 16:01 tomsquest

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 avatar Jan 06 '22 01:01 ManuelSchneid3r

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

ManuelSchneid3r avatar Jan 03 '23 19:01 ManuelSchneid3r

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.

xdg avatar Jan 03 '23 22:01 xdg

@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 ed2778c9f1f687e3c6f46d01ee837b698e791102, 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.

dshoreman avatar Jan 30 '23 00:01 dshoreman

please continue

MABIY avatar Feb 02 '23 05:02 MABIY

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

ManuelSchneid3r avatar Mar 29 '23 20:03 ManuelSchneid3r

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

ManuelSchneid3r avatar Aug 29 '23 18:08 ManuelSchneid3r