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 BeeWare Tutorial 1 support for GTK4 #3087

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danyeaw
Copy link
Member

@danyeaw danyeaw commented Jan 10, 2025

This PR is a first step for #3069 through:

  • Add a switch to turn on using GTK4 by setting the environmental variable TOGA_GTK=4
  • Make minimal changes to allow running a Hello World app using GTK4
  • Add a new CI test backend for linux-wayland-gtk4

Future PRs would implement libadwaita support and changes for additional widgets until we have a complete backend and then GTK3 can be removed.

Things left to do:

  • Determine why I am getting a libxapp error when trying to run the test suite with TOGA_GTK=4 set, maybe XApp is GTK3 only for Cinnamon, MATE and Xfce
  • Update tests / mark them xfail if not migrated yet
  • Fix my co-authored statements to give @MuhammadMuradG credit
  • Add changelog entry

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

image

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 in draft, but I've flagged a couple of things that stood out to me in a quick look through the code.

Also - what are you considering the point of completion for this specific PR? The title suggests "hello world"... but I'm not sure how complex your intended hello world target is. Is it Tutorial 0? The BeeWare tutorial? Something else? To be clear - I'm totally on board with landing a PR that only works for a big subset of widgets so that we can avoid a monster PR that needs to live for months; I'm just trying to gauge how far you're thinking of going with this specific PR.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
gtk/src/toga_gtk/libs/utils.py Outdated Show resolved Hide resolved
# Start Window Manager
echo "Start window manager..."
# mutter is being run inside a virtual X server because mutter's headless
# mode is not compatible with Gtk
Copy link
Member

Choose a reason for hiding this comment

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

I know you're probably in the "getting it to work at all" phase if development; but at some point it might be worth checking if this is a GTK3-specific problem (...or a problem with an older version of mutter. Our move to ubuntu-24.04 was motivated by a bug in the version of mutter that shipped with 22.04).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @freakboy3742, I can definitely investigate this either as part of this PR or as a separate one. Something like this should work with at least GTK4:

    env:
      XDG_RUNTIME_DIR: /tmp
    run: |
      eval $(dbus-launch --auto-syntax)
      DISPLAY=:99 MUTTER_DEBUG_DUMMY_MODE_SPECS=2048x1536 \
        mutter --wayland --no-x11 --sm-disable --headless toga &
      sleep 1

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to get this to work today. Although it does support running / testing a GTK app, our usage of Gdk.Screen / Gdk.Display doesn't seem to support a virtual display, and Toga throws a runtime error since the default_display returns None.

@danyeaw danyeaw changed the title Add hello world support for GTK4 Add Tutorial 1 support for GTK4 Jan 13, 2025
@danyeaw danyeaw changed the title Add Tutorial 1 support for GTK4 Add BeeWare Tutorial 1 support for GTK4 Jan 13, 2025
@danyeaw danyeaw force-pushed the gtk4-minimal branch 4 times, most recently from 21fdd13 to 830913e Compare January 17, 2025 20:55
@danyeaw danyeaw marked this pull request as ready for review January 18, 2025 02:23
Co-authored-by: Muhammad Murad <[email protected]>
@danyeaw
Copy link
Member Author

danyeaw commented Jan 18, 2025

In the spirit of a minimal implementation, I am only running a few of the test modules (window, app, and desktop) with wayland-gtk4. We could then enable additional tests as we implement more modules. What do you think of this approach?

No rush on the feedback, next week would be completely fine. Cheers!

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.

In the spirit of a minimal implementation, I am only running a few of the test modules (window, app, and desktop) with wayland-gtk4. We could then enable additional tests as we implement more modules. What do you think of this approach?

I can see how this approach would work, and I'm not completely opposed to it as an approach. However, if I had a choice, I'd prefer a "converging on 0%" approach, rather than a "converging on 100%" approach. That is, as part of this PR, we add a whole lot of explicit exclusions; when a subsequent PR adds support for a specific feature, that PR also deletes the testing exclusion.

That way the end state of the GTK porting effort is "all exclusions deleted", with the list of individual exclusions also working an effective TODO list. The approach you've used here would require the last PR to also delete the full list of "features that work" to revert to a "run all" configuration, which means it's a little harder to evaluate what is left to be done - and fairly easy for someone to add a new test file and forget to add the test to the GTK4 test list. It also means that code coverage testing is hidden until we reach the end-state, so it's difficult to evaluate if the PR enabling a specific feature has actually reached 100% coverage for the file implementing that feature.

The "converge on 0" approach is a little more invasive initially, but in a fairly straightforward way - adding a if GTK_VERSION >= (4,0): pytest.skip() in the constructor of each GTK probe implementation. It also means the initial test run is 95% skips, which is admittedly a little wasteful, but if the skip is early enough in the constructor process, then tests should run pretty quickly.

Speaking of coverage, the other thing that I noticed from the CI run is that we're going to add a new conditional-coverage qualifier - we already have a bunch of these for Python versions and PIL availability; adding one based on the TOGA_GTK environment variable should be fairly straightforward.

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