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

Move Makie Support to QMLMakie #207

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

Conversation

steffenhaug
Copy link

This is probably not quite ready for a merge, but I'm starting a PR so we have a place to discuss where to go from here, and for visibility in case someone wants to play around with it or help out.

This PR moves the Makie Support to a new package, QMLMakie, as suggested in the roadmap
and discussed in #206.

This has a number of benefits:

  • Decoupling the Makie integration from the QML version
  • Possibility of compat bounds
  • No need to rely on janky loading of GLMakie which breaks PackageCompiler.jl

As it is, I basically just copied the code from makie_support.jl verbatim and made the necessary changes injlqml to initialize Makie Support in a separate JlCxx module, so that can be wrapped by QMLMakie.jl. There is still a lot of work to do before QMLMakie can be registered, but simple examples already work:

@barche
Copy link
Collaborator

barche commented Sep 8, 2024

Thanks, tried it out and it works beautifully. Some comments:

  • Could you split the commits in this PR in one commit to fix the white space consistency and one commit that has the actual change?
  • steffenhaug/QMLMakie should probably be named steffenhaug/QMLMakie.jl
  • In your QMLMakie package, the module is named QtMakie but that should be QMLMakie

The last two points are of no infulence on this PR of course.

@steffenhaug
Copy link
Author

Thanks, tried it out and it works beautifully. Some comments:

Glad to hear :-)

Could you split the commits in this PR in one commit to fix the white space consistency and one commit that has the actual change?

Yeah, will do! I accidentally made the commit before i noticed the LSP had completely blown out the diff :-p

steffenhaug/QMLMakie should probably be named steffenhaug/QMLMakie.jl

Agreed!

In your QMLMakie package, the module is named QtMakie but that should be QMLMakie

Oh yeah, nice catch. Thanks!

@steffenhaug steffenhaug force-pushed the rip-out-makie-support branch from 606efd3 to d048a0f Compare September 8, 2024 20:02
@steffenhaug
Copy link
Author

I ran JuliaFormatter on all the files in src while I was at it. That made for quite a large change technically unrelated to the PR. Let me know if that isn't desirable.

Also, if you have an opinion on style, we could add a JuliaFormatter config file to the project, so everyone is on the same page. As is, I just ran it with the default settings.

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