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

Fix WindowTabs use of parse_text #4712

Merged
merged 4 commits into from
Mar 3, 2024

Conversation

heitzmann
Copy link
Contributor

This fixes the issue raised in #3741 by using the parser function for each window title before any state or markup is applied, similarly to what is done for the TaskList widget.

Additionally, instead of checking if the value is callable, it uses a default callable value and also skips the try...except block. I believe any raised errors will be logged anyway, won't they? And with better debug information than only "parse_text function failed:"

Copy link
Member

@elParaguayo elParaguayo left a comment

Choose a reason for hiding this comment

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

Thanks for this. I've made a request for one change.

libqtile/widget/windowtabs.py Show resolved Hide resolved
Copy link
Member

@elParaguayo elParaguayo left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I've added one question.

Also, would be good to add a test for this. This should do it:

diff --git a/test/widgets/test_windowtabs.py b/test/widgets/test_windowtabs.py
index 424044b7..f582b8cd 100644
--- a/test/widgets/test_windowtabs.py
+++ b/test/widgets/test_windowtabs.py
@@ -28,6 +28,10 @@ from libqtile.config import Screen
 from libqtile.confreader import Config


+def custom_text_parser(name):
+    return f"TEST-{name}-TEST"
+
+
 class WindowTabsConfig(Config):
     auto_fullscreen = True
     groups = [libqtile.config.Group("a"), libqtile.config.Group("b")]
@@ -40,6 +44,7 @@ class WindowTabsConfig(Config):
             top=bar.Bar(
                 [
                     widget.WindowTabs(),
+                    widget.WindowTabs(name="customparse", parse_text=custom_text_parser)
                 ],
                 24,
             ),
@@ -136,3 +141,14 @@ def test_escaping_text(manager):
     """
     manager.test_window("Text & Text")
     assert manager.c.widget["windowtabs"].info()["text"] == "<b>Text &amp; Text</b>"
+
+
+@windowtabs_config
+def test_custom_text_parser(manager):
+    """Test the custom text parser function."""
+    manager.test_window("one")
+    assert manager.c.widget["customparse"].info()["text"] == "<b>TEST-one-TEST</b>"

Comment on lines +67 to +70
name = self.parse_text(w.name if w and w.name else "")
except: # noqa: E722
logger.exception("parse_text function failed:")
name = w.name if w and w.name else "(unnamed)"
Copy link
Member

Choose a reason for hiding this comment

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

Do you need the if w part of the check here? If there any scenario where w is not a window object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That I'm not sure. It comes from the original implementation, so I kept it.

This fixes the issue raised in qtile#3741 by using the parser function for each window title before any state or markup is applied, similarly to what is done for the TaskList widget.

Additionally, instead of checking if the value is callable, it uses a default callable value and also skips the `try...except` block. I believe any raised  errors will be logged anyway, won't they? And with better debug information than only `"parse_text function failed:"`
Signed-off-by: Lucas Heitzmann Gabrielli <heitzmann@gmail.com>
@heitzmann heitzmann force-pushed the heitzmann/window_tabs_parsing branch from 2415d5e to c1c5170 Compare March 3, 2024 14:35
Signed-off-by: Lucas Heitzmann Gabrielli <heitzmann@gmail.com>
@elParaguayo
Copy link
Member

Great. This looks ok now. I'll set it to merge once tests pass.

@elParaguayo elParaguayo enabled auto-merge (squash) March 3, 2024 14:38
@elParaguayo elParaguayo merged commit 9c684ec into qtile:master Mar 3, 2024
25 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.

None yet

2 participants