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

Add support for NetworkService::MoveBefore/After #24

Merged
merged 1 commit into from Feb 22, 2024

Conversation

iceaway
Copy link
Contributor

@iceaway iceaway commented Feb 14, 2024

Hello. I noted that he MoveBefore/MoveAfter methods were not implemented so I gave it a go.

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment but guess this should be generally fine.

cc @LaakkonenJussi

libconnman-qt/networkservice.cpp Outdated Show resolved Hide resolved
@iceaway
Copy link
Contributor Author

iceaway commented Feb 20, 2024

One comment but guess this should be generally fine.

cc @LaakkonenJussi

Well spotted! I updated the PR with that change.

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No wait, this doesn't compile:

networkservice.cpp:336:49: error: no matching function for call to ‘NetworkService::Private::InterfaceProxy::asyncCall(const char [11], const QDBusObjectPath&)’
{ return asyncCall("MoveBefore", service); }

Could also squash the commits into one.

@LaakkonenJussi
Copy link
Contributor

No wait, this doesn't compile:

networkservice.cpp:336:49: error: no matching function for call to ‘NetworkService::Private::InterfaceProxy::asyncCall(const char [11], const QDBusObjectPath&)’ { return asyncCall("MoveBefore", service); }

Could also squash the commits into one.

And shouldn't these new functions be also defined in plugin/plugins.qmltypes ?

@pvuorela
Copy link
Contributor

And shouldn't these new functions be also defined in plugin/plugins.qmltypes ?

We haven't bothered with those during development. Changes updated with tooling going through all the projects when getting close doing an os release.

@iceaway
Copy link
Contributor Author

iceaway commented Feb 20, 2024

No wait, this doesn't compile:

networkservice.cpp:336:49: error: no matching function for call to ‘NetworkService::Private::InterfaceProxy::asyncCall(const char [11], const QDBusObjectPath&)’ { return asyncCall("MoveBefore", service); }

Could also squash the commits into one.

That's strange. On my machine it compiles without any errors using this command:

$ qmake && make -j

@pvuorela
Copy link
Contributor

That's strange. On my machine it compiles without any errors using this command:

What's the build environment and qt version? I'm guessing it might be some relatively new.
On 5.14 there's QDBusObjectPath::operator QVariant() added.

On our 5.6 I get this compiling by using QVariant::fromValue(service)) as parameter. Disclaimer: didn't confirm the D-Bus call gets done with the proper signature.

@iceaway
Copy link
Contributor Author

iceaway commented Feb 21, 2024

What's the build environment and qt version? I'm guessing it might be some relatively new. On 5.14 there's QDBusObjectPath::operator QVariant() added.

I'm using 5.15 here.

On our 5.6 I get this compiling by using QVariant::fromValue(service)) as parameter. Disclaimer: didn't confirm the D-Bus call gets done with the proper signature.

I updated my code to use this instead and can confirm that it works on my system.

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better now. Thanks.

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry again. Being about to merge and got one more 'wait a minute'.

So the API here uses QDBusObjectPath on the methods while the 'path' property and the NetworkService ctor use QString as type. I think it would be nice having the actual d-bus details under the hood, not leaking externally on the headers, and thus use also QString for the services here. Would also work better if exposing the functionality to QML.

@iceaway
Copy link
Contributor Author

iceaway commented Feb 21, 2024

Sorry again. Being about to merge and got one more 'wait a minute'.

So the API here uses QDBusObjectPath on the methods while the 'path' property and the NetworkService ctor use QString as type. I think it would be nice having the actual d-bus details under the hood, not leaking externally on the headers, and thus use also QString for the services here. Would also work better if exposing the functionality to QML.

No worries! Better to get it right :)

Do you mean something like this:

void NetworkService::moveAfter(const QString &service)
{
    if (m_priv->m_proxy) {
        m_priv->m_proxy->MoveAfter(QDBusObjectPath(service));
    }
}

@pvuorela
Copy link
Contributor

Do you mean something like this:

Yea.

@iceaway
Copy link
Contributor Author

iceaway commented Feb 21, 2024

Yea.

Great, updated and tested.

Implement support for using the NetworkService::MoveBefore and
NetworkService::MoveAfter commands over dbus.

Signed-off-by: Pelle Windestam <pelle@windestam.se>
Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good and hopefully this time for real :D

@pvuorela pvuorela merged commit 2769ae2 into sailfishos:master Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants