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 box widget background color bug #1684

Closed
wants to merge 2 commits into from
Closed

Fix box widget background color bug #1684

wants to merge 2 commits into from

Conversation

gregcowell
Copy link
Contributor

@gregcowell gregcowell commented Nov 14, 2022

Fixes #988. Currently a box widget that is a child of another box widget takes on the background colour of the parent box widget. This fix ensures that a child box uses the background colour specified via its own style.

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

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 can see how this would address the problem for at least some problems, but I don't think it's a general fix.

Consider the case of a base widget with a grandchild; the grandchild has it's own specific background color specified. This PR will cascade a background color change onto the children, and then the grandchildren. If the children don't specify the background color, then that's probably the right behavior; but overriding the grandchild's explicit background color won't be.

Pack doesn't currently have an explicit concept of inherited styles; so it could also be argued that the solution doesn't lie in cascading the style definition down to children, but to making the children transparent by default so that base styles are visible. For platforms where transparency isn't an option as a background color, that, may mean implementing "pseudo" transparency by implementing this sort of style cascade; but that would be a platform-specific implementation, not a generic property of the style applicator.

(FYI - I've merged this PR with main; we've recently landed a big repo restructure, so any outstanding PRs will need to be merged before any of the CI will run).

@gregcowell
Copy link
Contributor Author

Fair enough. With the test code below, the children each have their own explicit background color. The proposed fix seems to work in this particular scenario. Prior to the proposed change the children just used the background colour of the parent.

from toga import App, MainWindow, Label, Box
from toga.constants import BLUE, RED, GREEN, COLUMN
from toga.style import Pack


class Application(App):
    def startup(self):

        self.main_window = MainWindow(title=self.name)

        box1 = Box(
            children=[Label("Box 1 ...")],
            style=Pack(
                direction=COLUMN,
                background_color=BLUE
            )
        )

        box2 = Box(
            children=[Label("Box 2 ...")],
            style=Pack(
                direction=COLUMN,
                background_color=GREEN))

        box3 = Box(
            children=[Label("Box 3 ...")],
            style=Pack(
                direction=COLUMN,
                background_color=RED))

        box2.add(box1)
        box3.add(box2)
        self.main_window.content = box3

        self.main_window.content.refresh()
        self.main_window.show()


if __name__ == "__main__":
    app = Application("BugApp", "App")
    app.main_loop()

@freakboy3742
Copy link
Member

Yes - that example will work - but mostly by accident. The children's styles are being created (and therefore applied) after the parent's style has been applied, so the "cascading" aspect of your patch is never exercised (or, if it is exercised, the effect is being immediately overwritten).

However, if you add a button, and make the handler for that button set the color of box1, you should find it overwrites the style of children.

@gregcowell
Copy link
Contributor Author

Thanks. Yes, just works by accident. As a learning exercise I added a button that sets the background colour of box3 (the parent) and it does of course change the background colours of the children (box1 and box2). I did think it strange that the cascading aspect of my patch was actually having the opposite effect to what would be expected. It did work though (by accident) in this case because when I remove the patch all the boxes have the same background colour as box3. So paradoxically, adding cascading actually stopped the cascading from occurring somehow. Apologies for not investigating more thoroughly.

@mhsmith
Copy link
Member

mhsmith commented Nov 14, 2022

@gregcowell: Please give this PR a descriptive title rather than just an issue number. You can edit the title with the "Edit" button at the top right of the page.

A better way to link the PR to the original issue is to write "Fixes #xxx" in the PR's first comment, as shown here. You can edit the comment by clicking the "..." menu at the top right of the comment.

@gregcowell gregcowell changed the title Fix issue #988 Fix box widget background color bug Nov 14, 2022
@gregcowell gregcowell closed this Nov 14, 2022
@gregcowell gregcowell deleted the background branch November 14, 2022 21:32
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.

Windows: Background Color Not Applied to child Box
3 participants