cfg attribute on `use super::*`

[ratijas]

Very confused about these two lines:

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

here:

qmetaobject-rs/qmetaobject/src/scenegraph.rs

Lines 19 to 20 in ad28e6c

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

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]

Introduced in 4e74c4e by @rubdos.

[ogoffart]

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

Edit: No

[ogoffart]

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

[ratijas]

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]

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]

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]

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]

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]

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]

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]

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.