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 bug with markup in GroupBox widget #4239

Merged
merged 8 commits into from Jan 26, 2024

Conversation

blackgolyb
Copy link
Contributor

Description

Hi! There was a bug in the groupbox widget related to markup option. The markup option did not work in it.

Images

Bug preview:
before

After fixing:
after

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Qtile!
If you have not heard from us in a while, please feel free to ping one of the devs or anyone who has commented on the PR, as sometimes things fall through the cracks.
You can also join the chat room for real-time discussion, see the community links.
For details on what PRs might need to include before we will merge them please see the docs.

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 submitting this.

I think adding the ability to have this widget support markup is a nice addition. Have made a minor comment below.

Comment on lines 858 to 859
def max_layout_size(self, texts, font_family, font_size, markup=False):
sizelayout = self.textlayout("", "ffffff", font_family, font_size, None, markup=markup)
Copy link
Member

Choose a reason for hiding this comment

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

I think this change makes sense but could you also edit the TaskList widget (which is the only other widget that uses this method). Currently, if markup is applies, it uses regex to remove that markup from the text before calculating the width. I don't think that's the right approach as the markup can change the size of the text.

@blackgolyb
Copy link
Contributor Author

I made some edits to the TaskList. Did I understand this task correctly?

@elParaguayo
Copy link
Member

Yes, you did.

For the GroupBox, where are you specifying the markup? Is this done when creating the group in your config?

@blackgolyb
Copy link
Contributor Author

Yes, I can set a markup parameter in the configuration to change the markup in the GroupBox widget. Also it is True by default as written in the documentation

@elParaguayo
Copy link
Member

I think markup should be False by default so it doesn't change any existing behaviour.

I was actually asking where you set your markup formatting (e.g. <sup>). Was that part of your group name in your config?

@blackgolyb
Copy link
Contributor Author

Yes, the markup was part of the group name. EX: DEV<sup>1</sup>

@blackgolyb
Copy link
Contributor Author

Hi! What should I do next?

@blackgolyb blackgolyb changed the title Fix bug with markup in groupbox widget Fix bug with markup in GroupBox widget Apr 30, 2023
@elParaguayo
Copy link
Member

We should add a test for this. Here you go:

diff --git a/test/widgets/test_groupbox.py b/test/widgets/test_groupbox.py
new file mode 100644
index 00000000..1dd0f191
--- /dev/null
+++ b/test/widgets/test_groupbox.py
@@ -0,0 +1,46 @@
+# Copyright (c) 2023 elParaguayo
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+import pytest
+
+from libqtile import config, widget
+from libqtile.bar import Bar
+from test.helpers import BareConfig
+
+
+class GroupBoxConfig(BareConfig):
+    screens = [
+        config.Screen(
+            top=Bar([widget.GroupBox(), widget.GroupBox(name="has_markup", markup=True)], 24)
+        )
+    ]
+    groups = [config.Group("1", label="<sup>1</sup>")]
+
+
+groupbox_config = pytest.mark.parametrize("manager", [GroupBoxConfig], indirect=True)
+
+
+@groupbox_config
+def test_groupbox_markup(manager):
+    """Group labels can support markup but this is disabled by default."""
+    no_markup = manager.c.widget["groupbox"]
+    has_markup = manager.c.widget["has_markup"]
+
+    # If markup is disabled, text will include markup tags so widget will be wider
+    assert no_markup.info()["width"] > has_markup.info()["width"]

@kawpuh kawpuh mentioned this pull request Oct 4, 2023
2 tasks
@github-actions
Copy link

This PR is stale because it has been open 90 days with no activity. Remove the status: stale label or comment, or this will be closed in 30 days.

@elParaguayo elParaguayo enabled auto-merge (squash) January 26, 2024 19:39
@elParaguayo
Copy link
Member

@blackgolyb I've rebased, added the test, tidied up a couple of bits and set this to merge once tests pass.

Thanks for your help.

@elParaguayo elParaguayo merged commit b6dbc23 into qtile:master Jan 26, 2024
24 checks passed
@blackgolyb blackgolyb deleted the fix-groupbox-markup-bug branch January 26, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants