search

woboq / qmetaobject-rs

Integrate Qml and Rust by building the QMetaObject at compile time.
MIT License
633 stars 89 forks source link

cfg attribute on `use super::*` #138

Open ratijas opened 3 years ago

ratijas commented 3 years ago

Very confused about these two lines:

#[cfg(qt_5_8)]
use super::*;

here:

https://github.com/woboq/qmetaobject-rs/blob/ad28e6cc968707b2ff5c99871a1a1f193223d242/qmetaobject/src/scenegraph.rs#L19-L20

I don't know much about scene graph, but I suspect that's not how it was intended to work in Rust anyway.

Maybe it was supposed to be a module-level attribute #![cfg(...)] instead?

ratijas commented 3 years ago

Introduced in 4e74c4e99bb412cde539a0b865d7db3094fa0b79 by @rubdos.

ogoffart commented 3 years ago

Maybe that was meant as a #![cfg(...)]

Edit: No

ogoffart commented 3 years ago

I guess that's because only the code that needs Qt 5.8 uses the the import from the parent module.

ratijas commented 3 years ago

I guess that's because only the code that needs Qt 5.8 uses the the import from the parent module.

So that it does not trigger unused imports lint? For now I just removed that attribute. Let's see, if CI catches something (given that is still builds for < 5.8, no?)

ratijas commented 3 years ago

QSGRectangleNode Class was introduced in Qt 5.8, that much is true. But weirdly enough, the rest of scenegraph API is not versioned, e.g. QSGNode Class. I see no reason why the whole module should be feature-gated at qt-5.8.

ratijas commented 3 years ago

Given that c_void is now provided by top-level use crate::* / use super::*, it should be safe to remove the attribute without triggering an unused imports warning. Re: #137

ogoffart commented 3 years ago

The code is correct, if anything this was to prevent warning with Qt < 5.8

Note that i don't think Qt 5.6 is still tested because travis is dead, and Qt 5.6 is not available with the github action script i used.

ratijas commented 3 years ago

Sailfish is probably the only reason to keep < 5.8 around. Poor folks are gonna stuck with Qt 5.6 for eternity, although their official wiki page claims they are only planning a transition from Qt 5.2 xD

rubdos commented 3 years ago

I guess that's because only the code that needs Qt 5.8 uses the the import from the parent module.

So that it does not trigger unused imports lint? For now I just removed that attribute. Let's see, if CI catches something (given that is still builds for < 5.8, no?)

I think this was the case, indeed. I'm sorry I've bothered you with Qt 5.6 support...

Maybe it's better to change that use super::*; to use explicit imports? Then we can retain that attribute, and us, poor SailfishOS citizens, can stay happy for a bit longer. Jolla can't pull this off much longer though. We all hope we can get on Qt6 soon :-)

FWIW, I've finished most of the important features in my application. Next up is cleaning up my code, and then I'll be back here for the issues that I made a year ago :'-)

ratijas commented 3 years ago

I think this was the case, indeed. I'm sorry I've bothered you with Qt 5.6 support...

It's OK, no worries. Even if/when this project migrates to Qt6, Qt5 is still going to be around for a while (I think), so we better implement some past- and future-proof #[cfg] versioning anyway.

Maybe it's better to change that use super::*; to use explicit imports?

Maybe. I like explicit things. Once #137 is resolved, feel free to send a follow-up PR, if you are up to. Although, for a self-package imports I believe root globs are fine too. I don't think I broke anything for Qt 5.6 with that change, but you better check it yourself.

FWIW, I've finished most of the important features in my application. Next up is cleaning up my code, and then I'll be back here for the issues that I made a year ago :'-)

Nice! Do a blog post once you are done. No matter how hacky internals might be, I'm sure there will be something to learn from it :)

rubdos commented 3 years ago

FWIW, I've finished most of the important features in my application. Next up is cleaning up my code, and then I'll be back here for the issues that I made a year ago :'-)

Nice! Do a blog post once you are done. No matter how hacky internals might be, I'm sure there will be something to learn from it :)

I'd be glad to announce that https://www.rubdos.be/corona/qt/rust/tokio/actix/2020/05/23/actix-qt.html would be obsolete, but sadly it's still very much in that shape :'-)

Feel free to come have a chat in #whisperfish:rubdos.be though, if you're into Matrix. or #whisperfish on Freenode. We've got folks from KDE over these days, and they will be porting the application to Plasma Mobile.