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

add a layout debug mode that sets background colors #2953

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aerickson
Copy link

@aerickson aerickson commented Nov 15, 2024

Makes debugging Toga's layout system easier by visually indicating container boundaries. See #2856.

features:

  • randomized background color (using colors optimized for color-blindness) for boxes
  • enabled via TOGA_DEBUG_LAYOUT env var

usage:

TOGA_DEBUG_LAYOUT=1 briefcase dev

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@aerickson
Copy link
Author

Throwing this up for initial feedback, will complete docs and tests once things are looking good.

@@ -33,6 +54,20 @@ def __init__(
:param style: A style object. If no style is provided, a default style
will be applied to the widget.
"""
# if the object has _USE_DEBUG_BACKGROUND=True and layout debug mode is on, change bg color
if hasattr(self, "_USE_DEBUG_BACKGROUND") and self._USE_DEBUG_BACKGROUND:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hope I'm not butting in here, but a handy thing you may not be aware of: getattr() can take an optional third parameter as a default to supply if the attribute is missing. Dictionaries have a roughly analogous get() method, which returns None or a user-supplied default if the key is missing.

So lines 58 and 59 could do the same thing and be written as:

if getattr(self, "_USE_DEBUG_BACKGROUND", False):
    if environ.get("TOGA_DEBUG_LAYOUT") == "1":

def test_button_debug_background():
"""A Button in layout debug mode has a default background."""
# Enable layout debug mode
environ["TOGA_DEBUG_LAYOUT"] = "1"
Copy link
Member

Choose a reason for hiding this comment

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

This modifies the global environment, so it will leak into external tests (which means this test can impact on other tests). For something like this, it's a good idea to use monkeypatch.setenv - that will automatically reset the environment variable when the test finishes.


# need enough for coverage of palette array index rollover
box = toga.Box()
toga.Box()
Copy link
Member

Choose a reason for hiding this comment

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

for i in range(0, 10):
...



# test that a non-container-like widget in layout debug mode has a default background
def test_button_debug_background():
Copy link
Member

Choose a reason for hiding this comment

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

Good idea testing a widget other than Box; - but also worth adding at test for (a) a container widget, and (b) a widget where the debug background won't be applied.


# assert that the bg is not default/white
assert hasattr(box.style, "background_color")
assert not box.style.background_color == rgb(0, 0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

"Not white" can cover a multitude of sins :-)

A better test would be to evaluate the specific colors for all the widgets created in a specific order. The order of allocation should be predictable, once you've found the color of the first box.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I know this is still marked as a draft, but I couldn't help myself providing some initial feedback. tl;dr - this is all looking very promising; with a couple of fairly minor tweaks, it should be pretty close to merge-ready.

@aerickson
Copy link
Author

I know this is still marked as a draft, but I couldn't help myself providing some initial feedback. tl;dr - this is all looking very promising; with a couple of fairly minor tweaks, it should be pretty close to merge-ready.

Awesome. :)

I think I've addressed all of the comments. Thank you both.

@freakboy3742
Copy link
Member

@aerickson Happy to give this another review; but it's currently showing a merge conflict that needs to be resolved. Also, you can flick this over to "ready for review" if you think it's ready.

@freakboy3742
Copy link
Member

The CI failure isn't anything to be concerned about - it's a recently introduced intermittent failure (#3016).

@aerickson aerickson force-pushed the debug_mode_alternating_bg_v2 branch from 206fb31 to 02f3f87 Compare December 5, 2024 00:08
@aerickson aerickson marked this pull request as ready for review December 5, 2024 00:09
@aerickson
Copy link
Author

@aerickson Happy to give this another review; but it's currently showing a merge conflict that needs to be resolved. Also, you can flick this over to "ready for review" if you think it's ready.

Changed to 'ready for review'. Rebased.

The CI failure isn't anything to be concerned about - it's a recently introduced intermittent failure (#3016).

OK, thanks. :)

@aerickson
Copy link
Author

Where would you like this documented?

https://toga.readthedocs.io/en/stable/how-to/topics/layout.html (aka docs/how-to/topics/layout.rst) maybe?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This is definitely heading in the right direction - a couple of suggestions inline, mostly about testing. The most significant of those suggestions is possibly switch to making this a parameterised test that can be evaluated on every widget, which would then give us a confirmation that we've got _USE_DEBUG_BACKGROUND set correctly for each widget.

Regarding documentation - I think this should be the start of a new HOWTO topic guide on "debugging your app"; that document will only have one section for now (debugging layout); but it gives us a place to put other debugging hints (e.g., how to use pdb et al). There should also be a cross reference to this new section in the documentation on Pack, so that someone trying to understand Pack is made aware that the debugging option exists.

"#e5e4af", # light cream
"#bde2dc", # soft turquoise
]
shuffle(debug_background_palette)
Copy link
Member

Choose a reason for hiding this comment

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

Why programmatically shuffle? We definitely need an order which cycles good contrast (i.e., we don't have "light blue" next to "very light blue"), but that order doesn't need to be random.

Plus, randomisation takes more time at startup, and a randomised order could be jarring for as successive runs won't give the same output.

Comment on lines +8 to +12
def test_box_no_background_in_normal_mode():
"""A Box has no default background."""
# Disable layout debug mode
with MonkeyPatch.context() as mp:
mp.setenv("TOGA_DEBUG_LAYOUT", "0")
Copy link
Member

Choose a reason for hiding this comment

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

Monkeypatch can be used as a fixture:

Suggested change
def test_box_no_background_in_normal_mode():
"""A Box has no default background."""
# Disable layout debug mode
with MonkeyPatch.context() as mp:
mp.setenv("TOGA_DEBUG_LAYOUT", "0")
def test_box_no_background_in_normal_mode(monkeypatch):
"""A Box has no default background."""
# Disable layout debug mode
monkeypatch.setenv("TOGA_DEBUG_LAYOUT", "0")

Copy link
Member

Choose a reason for hiding this comment

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

We try to keep the test structure matching the widget layout; or, if things are getting complex, matching the feature by the name used in the code. In this case, if there's too many tests to add to widgets/test_base.py, this should probably be widgets/test_debug_layout.py (I think the latter makes sense here)

mp.setenv("TOGA_DEBUG_LAYOUT", "0")
box = toga.Box()
# assert that the bg is default
assert hasattr(box.style, "background_color")
Copy link
Member

Choose a reason for hiding this comment

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

This assertion essentially can't fail. Toga's style framework will ensure that background_color exists as an attribute for read.

box = toga.Box()
# assert that the bg is default
assert hasattr(box.style, "background_color")
assert not box.style.background_color
Copy link
Member

Choose a reason for hiding this comment

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

A check of an actual value, rather than "falseness" would be preferable here - that avoids any odd situations where something like rgb(0,0,0) evaluates as "false" (I don't think it does, but just in case)

Comment on lines +27 to +28
assert hasattr(button.style, "background_color")
assert not button.style.background_color
Copy link
Member

Choose a reason for hiding this comment

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

As above:

  • monkeypatch can be used as a fixture.
  • the hasattr won't be asserting anything
  • A positive value check would be preferable.

The same pattern applies to the subseqeunt tests as well.

assert not button.style.background_color


# test that a label in layout debug mode has a default background
Copy link
Member

Choose a reason for hiding this comment

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

Why test label and button? Or, if it's important to check every widget (arguable, it is), is there any reason we shouldn't encode this as a test in properties.py like test_enabled/test_enable_noop - that is, a test that can be applied to every widget, with a "uses debug" and "doesn't use debug" variant?


for counter, box in enumerate(boxes, start=1):
index = counter % debug_bg_palette_length
print(counter)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print(counter)

@@ -17,10 +19,29 @@
StyleT = TypeVar("StyleT", bound=BaseStyle)


# based on colors from https://davidmathlogic.com/colorblind
debug_background_palette = [
Copy link
Member

Choose a reason for hiding this comment

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

If this is a list of constants, it should be treated as a constant by naming convention:

Suggested change
debug_background_palette = [
DEBUG_BACKGROUND_PALETTE = [

print(counter)
assert hasattr(box.style, "background_color")
assert box.style.background_color == color(
toga.widgets.base.debug_background_palette[index]
Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm missing something, this test doesn't account for the state of _debug_color_index at the start of the test. it essentially must be the first (and only?) "background modifying" test in the suite; if any other test enables debug, this test will fail because the colors won't align.

This means either the test needs to monkeypatch _debug_color_index, or the test needs to take the initial state of _debug_color_index into consideration.

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.

3 participants