KDAB / cxx-qt

Safe interop between Rust and Qt
https://kdab.github.io/cxx-qt/book/
1.05k stars 75 forks source link

cxx_name on methods #828

Closed knoxfighter closed 3 weeks ago

knoxfighter commented 8 months ago

The API i work with doesn't follow Qt codestyle. Therefore it contains a function named OnViewChange (with a capital first letter, aka. PascalCase). cxx-qt does not support cxx_name attributes to rename such functions, they are always formatted to CamelCase.

I wrote this as another example for #667

LeonMatthesKDAB commented 8 months ago

Hi, thank you for taking the time to report this issue.

Could you provide a small code snippet that shows how you're declaring the OnViewChange method with CXX-Qt? Is it in extern "C++Qt" or extern "RustQt", a #[qinvokable], #[cxx_override]? Maybe I can provide you with a workaround for now :sweat_smile:

We know there are issues with (re-)naming items in CXX-Qt. That's definitely an area of improvement for the next release. It's loosely tracked as part of #665

knoxfighter commented 8 months ago

It is part of a rust override.

The C++ API:

class UIContextNotification {
    ...
    virtual void OnViewChange(UIContext* context, ViewFrame* frame, const QString& type){}
    ...
}

My rust override:

unsafe extern "RustQt" {
    #[base = "UIContextNotification"]
    type MyUIContextNotification = super::MyUIContextNotificationRust;
}

unsafe extern "RustQt" {
    #[cxx_name = "OnViewChange"] // this is ignored and `onViewChange` is used instead
    #[cxx_override]
    unsafe fn OnViewChange(
        self: Pin<&mut MyUIContextNotification>,
        context: *mut UIContext,
        frame: *mut ViewFrame,
        type_: &QString,
    );
}

which results in the compiler error:

ffi_ui_notifications.cxxqt.h(20): error C3668: 'MyUIContextNotification::onViewChange': method with override specifier 'override' did not override any base class methods
LeonMatthesKDAB commented 8 months ago

I'm just looking through the code. This seems to indeed be no longer possible to do at all :see_no_evil: The camel case conversion is always used.

I think this entirely broke during the conversion to our new API for 0.6... We'll definitely have to fix this.

Sorry there's no workaround at the moment.

ahayzen-kdab commented 8 months ago

Right this is an area we need to refactor, hopefully for 0.7.

For a workaround maybe you can use an alias in C++ ? Eg create a derived class with a camel case name and then use that in Rust.

The changes in #667 were an early attempt at trying to improve the problem, we were also trying to figure out if by default we still want the automatic camel <-> snake conversion (which is useful for things like QML signals as otherwise you potentially end up with my_property becoming onMy_propertyChanged).

I think before when we discussed the options we decided this was a possible route extern no attributes cxx_name / rust_name
RustQt C++: camelCase, Rust: original C++: cxx_name / original, Rust: rust_name / original
C++Qt C++: original, Rust: snake_case ? C++: cxx_name / original, Rust: rust_name / original

I am unsure about C++Qt -> no attributes -> Rust case though, maybe only the automatic conversion happens in RustQt. @LeonMatthesKDAB what do you think ?

LeonMatthes commented 8 months ago

Hm, I actually quite like your proposal, including snake_case Conversion in C++Qt... As then you'd always just write exactly what is used in the "language of origin", and it would still look native in the other language.

And with the table you provided, it should be easy enough to explain.

ahayzen-kdab commented 8 months ago

Hm, I actually quite like your proposal, including snake_case Conversion in C++Qt... As then you'd always just write exactly what is used in the "language of origin", and it would still look native in the other language.

And with the table you provided, it should be easy enough to explain.

Right that was my though, it's either "language of origin" or if you specify a cxx/rust_name then it follows those or the original. This should allow you to get to all combinations i think :thinking:

LeonMatthesKDAB commented 3 months ago

As of #1005, you should now be able to use cxx_name and rust_name on methods again :partying_face: .

I think we may still be missing the correct case conversions in the extern "C++" blocks, as described here https://github.com/KDAB/cxx-qt/issues/828#issuecomment-1920288043 , but otherwise we're making good progress.

LeonMatthesKDAB commented 3 weeks ago

We've decided to drop the automatic renaming for 0.7. There is the option to add an explicit auto_case_convert attribute to the bridge in the future to re-add this. But for now, this issue is closed.