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

Use Qt Quick Controls 2 & Qt Labs Platform #1984

Merged
merged 6 commits into from
Nov 25, 2018

Conversation

mitchcurtis
Copy link
Contributor

Qt Quick Controls 1 is desktop-oriented, which Tiled (widgets) already
has covered. If Tiled Quick is supposed to be for touch devices,
it is better off using Qt Quick Controls 2.

This bumps the minimum required Qt version for Tiled Quick to 5.10.

Qt Quick Controls 1 is desktop-oriented, which Tiled (widgets) already
has covered. If Tiled Quick is supposed to be for touch devices,
it is better off using Qt Quick Controls 2.
@mitchcurtis
Copy link
Contributor Author

I have a bunch of changes to make. Can I just push them all to this branch?

There's going to be a qml/ directory in tiledquick.
- Move QML files into qml/
- Moved Android-specific files into qml/+android so that they will be loaded
automatically on Android by file selectors
This font will be used for icons.
@bjorn bjorn merged commit ed8eff2 into mapeditor:wip/qtquick Nov 25, 2018
@bjorn
Copy link
Member

bjorn commented Nov 25, 2018

Sorry for merging this so late. It's a great idea to switch to Qt Quick Controls 2, even on desktop I think. Thanks!

I also like the changes you made for Android, though did you actually try if it still works? I'm wondering mainly about whether it can find the tiledquickplugin, which was used statically before on Android but now you share the same main.cpp.

@bjorn
Copy link
Member

bjorn commented Nov 25, 2018

@mitchcurtis Note that I've now merged wip/qtquick into master, so if you want to make further changes you should open a pull request to master. I mainly merged it since I am now working on #949, and I want to try relying on using QObject for the base classes so that they can be easily exposed to JavaScript. In addition I expect the script API to extend into Tiled Quick.

@mitchcurtis
Copy link
Contributor Author

Sorry for merging this so late. It's a great idea to switch to Qt Quick Controls 2, even on desktop I think. Thanks!

No worries. :)

I also like the changes you made for Android, though did you actually try if it still works? I'm wondering mainly about whether it can find the tiledquickplugin, which was used statically before on Android but now you share the same main.cpp.

I'm pretty sure I did, but this is going back too far for my memory. :D

@mitchcurtis Note that I've now merged wip/qtquick into master, so if you want to make further changes you should open a pull request to master. I mainly merged it since I am now working on #949, and I want to try relying on using QObject for the base classes so that they can be easily exposed to JavaScript. In addition I expect the script API to extend into Tiled Quick.

Oh, good to hear!

@bjorn
Copy link
Member

bjorn commented Dec 28, 2018

@mitchcurtis Btw, while I merged wip/qtquick into master partly because the base data types would be QObject-derived and this would simplify exposing them to the script, I've currently changed my mind about this approach and instead chose to implement "editable" wrappers around the base classes.

This is mainly because any change made to these classes should generally be covered by undo commands, and the editable wrappers can create these commands transparently. It would not be desirable to implement this in the base classes, even if they are derived from QObject.

Because of the use of wrapper classes, there should actually be no need to still have the base classes derive from QObject, so I'll probably look at reverting that change sometime in the next weeks. Tiled Quick will then need to be updated to also use the wrapper classes to bind to properties. That in turn means the wrappers (and their dependencies) will need to be moved either to libtiled or to a new library that is shared with Tiled.

In any case it will be interesting to try if we can use these editable wrapper classes to add functionality to Tiled Quick.

@mitchcurtis
Copy link
Contributor Author

Because of the use of wrapper classes, there should actually be no need to still have the base classes derive from QObject, so I'll probably look at reverting that change sometime in the next weeks. Tiled Quick will then need to be updated to also use the wrapper classes to bind to properties. That in turn means the wrappers (and their dependencies) will need to be moved either to libtiled or to a new library that is shared with Tiled.

Oo... so then games like mine can start using just libtiled instead of having to build Tiled Quick as well?

Either way, I'm curious how this will look for users of the Quick API.. it also reminds me that I haven't updated my tiled sub-module in too long... :x

@mitchcurtis mitchcurtis deleted the controls2 branch December 28, 2018 15:09
@bjorn
Copy link
Member

bjorn commented Dec 28, 2018

Oo... so then games like mine can start using just libtiled instead of having to build Tiled Quick as well?

Hmm, that's an interesting question... actually the editable wrappers make no sense in a game, because you do not want the overhead of the undo commands when making changes to the data.

So for that use-case the question remains whether it makes sense to keep the QObject derived base classes, or whether to add a "no undo system" option to the wrappers, or whether to write different wrappers altogether for use in games... If added as an option, it would probably be best to have it at compile-time for performance reasons.

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