Skip to content

add ActivityIndicator for Winforms#3473

Merged
freakboy3742 merged 40 commits into
beeware:mainfrom
johnzhou721:winforms
Jun 6, 2025
Merged

add ActivityIndicator for Winforms#3473
freakboy3742 merged 40 commits into
beeware:mainfrom
johnzhou721:winforms

Conversation

@johnzhou721

@johnzhou721 johnzhou721 commented May 20, 2025

Copy link
Copy Markdown
Contributor

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

@johnzhou721

Copy link
Copy Markdown
Contributor Author

This is untested as I do not have a Windows machine. If someone has a Windows machine: please help me test this, thanks.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

I haven't written any tests yet. I'll be happy for any guidance on how to do so, though I'm practically limited to a macOS machine.

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One simplification suggestion and a tweak to the release note; there's also a need for two documentation updates:

  1. An update to the widget availability chart data
  2. A screenshot on the ActivityIndicator docs page

As for testing - you don't need to write any new tests; you only need to write a probe implementation. The probe provides a generic interface by which the set of existing tests validates the behavior of the widget is consistent. You can use the existing ActivityIndicator probe on any other platform as a starting point for finding the methods that need to be defined; as soon as the probe class exists, the testbed will run the tests on Winforms.

Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread changes/3473.feature.rst Outdated
@johnzhou721 johnzhou721 requested a review from freakboy3742 May 20, 2025 02:31
@johnzhou721

Copy link
Copy Markdown
Contributor Author

Done. I cannot provide a screenshot since... I don't have a windows machine laying around.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Also... I'm not 100% confident that this is a good spinner.

By the way, let me add a LICENSE to my spinner-anim. Do you think we should link it in the docs so there's no question about the origins of that file?

@johnzhou721

Copy link
Copy Markdown
Contributor Author

I apologize for a0dc0a0 after the review was requested; I misread your comment.

@johnzhou721

This comment was marked as resolved.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

OK... so how am I supposed to have complete coverage? Where should I add more tests to cover everything?

@freakboy3742

Copy link
Copy Markdown
Member

OK... so how am I supposed to have complete coverage? Where should I add more tests to cover everything?

Ah - I forgot one more change - in the testbed's tests_activityindicator, you need to remove windows from the skip list.

As for the screenshot - I'll need to wait until I'm back at home before I can test this and get an actual screenshot.

@johnzhou721

johnzhou721 commented May 20, 2025 via email

Copy link
Copy Markdown
Contributor Author

@freakboy3742

Copy link
Copy Markdown
Member

So like we're supporting AcitivityIndicator on Winforms rather than the Winforms backend providing it...

The release notes are in the voice of "This project now has a feature that...".

They're in a very different voice to comments in the code, and PRs - in those contexts, we try to be "egoless" in the sense that we're describing what the code does, not describing our deep personal struggle to find a solution to something.

@johnzhou721

johnzhou721 commented May 20, 2025 via email

Copy link
Copy Markdown
Contributor Author

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Hi! I'm not sure what's going on in the testbed and I'm not sure why the widget isn't showing on an actual Windows machine with Python 3.12 (I packaged it by running python -m build in all relevant directories and put it onto the Windows machine and used pip (using python subprocess, we don't have command line) and --no-deps to install toga, toga-core, and toga-winforms) either... I tried a couple of things like making visible = true and also using autosize as sizemode at init but nothing shows. I'm using

import toga
 
def build(app):
    box = toga.Box()
 
    indicator = toga.ActivityIndicator()
    indicator.start()
    box.add(indicator)
 
    return box
 
 
def main():
    return toga.App("First App", "org.beeware.toga.examples.tutorial", startup=build)
 
 
if __name__ == "__main__":
    main().main_loop()

@freakboy3742

Copy link
Copy Markdown
Member

I can't think of any obvious reason either - but I don't have access to a Windows test box, so I won't be able to provide any help until I'm back in my office early next week.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Thanks. Any tips on debugging those things

@freakboy3742

Copy link
Copy Markdown
Member

Not really; it depends entirely on what behaviour you are (or aren't) seeing. In this case, is this image being added at all? If you use a different (static) image, does it work? If you change background colors, can you see the outline of the image?

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Different static image did not work if I remember correctly.

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AFAICT, the issues you're seeing with testing are entirely caused by the handling of visibility logic.

Comment thread winforms/src/toga_winforms/resources/spinner.gif
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
@johnzhou721

Copy link
Copy Markdown
Contributor Author

Thanks! As someone who does math... you'd expect me to be very good at logic...

@johnzhou721

Copy link
Copy Markdown
Contributor Author

OK so GIF cannot have partial transparency... higher resoltuion does not seem to fix the problem...

Should I leave it like this?

@johnzhou721

Copy link
Copy Markdown
Contributor Author

OK So I've used ChatGPT to generate a script to do this, is there anything obviously wrong with it? Probably, but it seems to work with the latest releases.

from PIL import Image, ImageSequence, ImageFilter

def process_gif(input_path, output_path, rgb_background):
    src = Image.open(input_path)
    size = src.size

    # This will hold the fully reconstructed frames
    full_frames = []

    # Create a "previous frame" canvas
    prev = Image.new("RGBA", size, (0, 0, 0, 0))

    # 1) Reconstruct full frames
    for frame in ImageSequence.Iterator(src):
        frame_rgba = frame.convert("RGBA")

        # if this frame has a box, paste it onto prev; otherwise it's a full-frame update
        if frame.tile:
            update_region = frame.tile[0][1]  # (x0,y0,x1,y1)
            prev.paste(frame_rgba.crop(update_region), update_region)
        else:
            prev = frame_rgba

        # Make a copy of the fully built frame
        full_frames.append(prev.copy())

    # Now process each full frame
    processed = []
    for frame in full_frames:
        # 2) Composite on your RGB background
        bg = Image.new("RGBA", size, rgb_background + (255,))
        comp = Image.alpha_composite(bg, frame)

        # 3) Apply a smoothing filter
        sm = comp.filter(ImageFilter.SMOOTH_MORE)

        # 4) Quantize back to P mode with adaptive palette
        pal = sm.convert("P", palette=Image.ADAPTIVE, colors=255)
        
        # Reserve palette index 255 for transparency
        # Find which palette entry in `pal` corresponds to the fully transparent pixels
        transparent_mask = sm.split()[3].point(lambda a: 255 if a == 0 else 0)
        pal.paste(255, mask=transparent_mask)

        processed.append(pal)

    # Finally, save as a new GIF
    processed[0].save(
        output_path,
        save_all=True,
        append_images=processed[1:],
        loop=src.info.get("loop", 0),
        duration=src.info.get("duration", 100),
        transparency=255,
        disposal=2,
    )

# Usage
rgb_background = (255, 200, 100)
process_gif("output2x.gif", "output_full.gif", rgb_background)

I will try to learn pillow properly tomorrow... but in the meantime I'm trying to work on a CPython issue which will take the rest of my available time today.

@freakboy3742

Copy link
Copy Markdown
Member

OK So I've used ChatGPT to generate a script to do this, is there anything obviously wrong with it?

I won't profess expertise in Pillow - but honestly, I'm a little surprised it's that complex - especially with regard to re-cropping frames.

The other thing is that we probably don't want to save it to a file - we want to keep it in-memory, stored in a cache against different background colors (so we only generate one image in the simple case).

Or, we could generate one anti-aliased image against the default Windows background color, and leave this as a known issue.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Great. Of course, ChatGPT is Almost Always Wrong (AAW (tm) :)) ("))" looks so imbalanced), so let me learn pillow a bit further tomorrow.

On the other hand my ability to contribute is severely limited... I didn't even know lstrip and rstrip exists, and someone pointed it out on py/cpy and got 3 laugh responses...

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Also we can save it to a temp file convert it into a Winforms bitmap and delete the on disk file

@johnzhou721

Copy link
Copy Markdown
Contributor Author

So... breaking news! I've gotten a way to generate a convincing looking image after hours of experimentation but only for 64x64 and not 128x128.
output_anti

@johnzhou721

Copy link
Copy Markdown
Contributor Author

HOLD ON.... I found the bug

When using convert I believe the PDF is first rasterized at a fixed size and THEN scaled. If you do e.g. 1024x1024 the output will essentially become minecraft.

I used \scalebox in LaTeX to increase page size (=initial "resolution" for ImageMagick) but realized I used preview for tikzpicture and not around the scalebox

Scrub everything in this thread, and will retract all releases from the repo immediately after I finish fixing.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

In other words, higher resolution works, and ALL of this is an error on my end. I apologize for causing you all the sleeplessness and trouble, if any.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

OK now let's make the spinner thicker.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Still looks just a little jagged when the zoom is at 100% but it's good for anything above... but it's acceptable for now.

Comment thread winforms/src/toga_winforms/resources/antialias_spinner.py Outdated
@johnzhou721 johnzhou721 requested a review from freakboy3742 June 3, 2025 10:53

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is getting pretty close; I think we're down to the last pieces of cleanup and internal documentation.

Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated

@johnzhou721 johnzhou721 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For my own benefit.

Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
Comment thread winforms/src/toga_winforms/widgets/activityindicator.py Outdated
@johnzhou721 johnzhou721 requested a review from freakboy3742 June 5, 2025 14:25
@johnzhou721

johnzhou721 commented Jun 6, 2025

Copy link
Copy Markdown
Contributor Author

FYI: It says processing updates even after 13 minutes, but I'm able to pull your commit into my local repo and it looks good to me. Thanks for making the code DRYer, and for the refresh thing.

EDIT and also thanks for handling my DPI concerns and for untangling my spaghetti and for understanding why I'm caching the size!

@freakboy3742

Copy link
Copy Markdown
Member

Yeah - not sure what is going on with Github. I think this is ready to land... but I need to see CI first.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Hmm... Should I close this and open a new PR with this exact same branch?

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Doing the above since 23 minutes already passed... ironically the processing updates is displayed as a spinner which is what we're trying to add here.

[pun b/c if I just hit close it'd send an email anyways]

@johnzhou721 johnzhou721 closed this Jun 6, 2025
@johnzhou721 johnzhou721 reopened this Jun 6, 2025
@johnzhou721

Copy link
Copy Markdown
Contributor Author

@freakboy3742 It's good now.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

CI Pass!

@johnzhou721

Copy link
Copy Markdown
Contributor Author

Sounds intermittent.

@freakboy3742

Copy link
Copy Markdown
Member

I can re-run Ci on a single task if there's an intermittent failure. You don't need to push a blank commit.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

OK so I removed the latest commit in hopes of the commit will stop eating CI resources but turns out the last commit was re-run which wasted even more CI... sorry...

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like that does it - time to merge!

@freakboy3742 freakboy3742 merged commit 8061ef8 into beeware:main Jun 6, 2025
101 of 103 checks passed
@johnzhou721

johnzhou721 commented Jun 6, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for working me through this! Sorry for wasting your time on errors in spinner-anim.

Looks like that does it

Yak shaving... correlates well with the mascot. :)

  • "As soon as we refactor this, it's done."
  • "Wait, a test isn't enabled"
  • "What why is the CI failing?"
  • "Let me push an empty commit
  • "Hold on, I'm wasting resources... let me abort this CI run"
  • "Wait, why is the previous commit ran now?"

EDIT (sent early) we're seeing 103 checks because both runs associated with that commit got counted... Potential bug in GitHub.

@johnzhou721

Copy link
Copy Markdown
Contributor Author

@freakboy3742 Sorry for commenting on a closed ticket, but should the default background color be transparent?

@freakboy3742

Copy link
Copy Markdown
Member

@freakboy3742 Sorry for commenting on a closed ticket, but should the default background color be transparent?

Yes - all widgets start with a background color of TRANSPARENT. This matches the CSS specification.

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.

2 participants