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

Non-initialized .modems breaks example app #17

Merged
merged 1 commit into from Jan 26, 2023

Conversation

jmlich
Copy link
Contributor

@jmlich jmlich commented Dec 25, 2022

It seems there is an lazy initialization of ModemManager.modems as consequence, the modems[0] are not available at the time of start application and nothing works.

Similar OfonoConnMan contexts are not initialized. The binding must check if array is defined and its length

I have added also the SimManager check to make sure that the Pin is entered, because it prevents further testing.

I am not sure about netreg.currentOperator["Name"] I keept it as is was

@pvuorela
Copy link
Contributor

Please rebase instead of having a merge commit.

MouseArea {
anchors.fill: parent
onClicked: {
context1.active = !context1.active
context1.active = !context1.active;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use semicolons at the end of the lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove that. The semicolon was probably added by qmlformat.

textLine2.text = manager.available ? netreg.currentOperator["Name"].toString() :"Ofono not available"
console.log("Ofono is " + available);
console.log("netreg.currentOperatorPath: " + netreg.currentOperatorPath);
textLine2.text = (manager.available && netreg.currentOperator !== undefined) ? netreg.currentOperator["Name"].toString() : "Ofono not available";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, maybe not regression but trying this I'm getting console output of
[D] onAvailableChanged:55 - Ofono is true

and this saying "Ofono not available" on the UI. Supposedly because of:
[D] onAvailableChanged:56 - netreg.currentOperatorPath:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am not sure what was original purpose. I can guess that it should write mobile operator name, but the value is accessible via netreg.name. The array seems to be not populated. I didn't check how this interface changed in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I would avoid assignment textLine2.text = , because it breaks binding defined above (I have changed the behaviour in recent force push in this merge request). As result it is showing network operator now (KAKTUS in my case, but can be T-Mobile or AT&T as well)

// console.log("modem1.name " + modem1.name)
// console.log("modem1.model " + modem1.model)
console.log("modem1.manufacturer " + modem1.manufacturer);
console.log("modem1.features " + modem1.features);
Copy link
Contributor

Choose a reason for hiding this comment

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

These could have colon. Now I'm getting output of just "modem1.manufacturer" which is a bit strange.

Copy link
Contributor

Choose a reason for hiding this comment

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

These still apply.

For another thing the line ending semicolon is also here and on multiple other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to use semicolons explicitly because of javascript have some evil cases when it joins multiple lines into one expression. If I understand correctly, you have some guideline to avoid them if possible, so I removed them.

@pvuorela
Copy link
Contributor

Some small comments.

Shouldn't be related to this PR but executing the test app binary I'm getting console output but no window. Running qmlscene with the main qml file shows the window too. Interesting.

@jmlich jmlich force-pushed the master branch 3 times, most recently from f3d3ae5 to 00fe6b0 Compare January 13, 2023 07:34
@jmlich
Copy link
Contributor Author

jmlich commented Jan 13, 2023

Actually, I was also using qmlscene, because the binary didn't worked for me either. I have expected that there is some trouble in my setup.

The documentaion says: https://doc.qt.io/qt-5/qqmlapplicationengine.html

Unlike QQuickView, QQmlApplicationEngine does not automatically create a root window. If you are using visual items from Qt Quick, you will need to place them inside of a Window.

I have updated the code accordingly

@@ -8,5 +9,9 @@ Q_DECL_EXPORT int main(int argc, char *argv[])
QQmlApplicationEngine engine;
engine.load(QUrl(QLatin1String("/opt/examples/libqofono-qt5/qml/ofonotest/main.qml")));

if ( QWindow* window = qobject_cast<QWindow*>( engine.rootObjects().at(0) ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this appears to evaluate false here. rootObjects().at(0) is QQuickRectangle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the main.qml to contain Window. In my tests the binary already shows the window, so it works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, but the Window object is maybe enough it self without changes in C++ part. I can retest it, but not during working hours

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It must be something like:

Window {
   width: 320
   height: 200
   visible: true
}

My recent push --force contain required changes

@jmlich jmlich force-pushed the master branch 3 times, most recently from 071cdac to 58df79e Compare January 16, 2023 13:04
It seems there is an lazy initialization of ModemManager.modems
as consequence, the modems[0] are not available at the time
of start application and nothing works.

Similar OfonoConnMan contexts are not initialized. The binding must
check if array is defined and its length

I have added also the SimManager check to make sure that the Pin is
entered, because it prevents further testing.

Also window of example application wasn't showing
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.

Now getting content on the screen by running the example and the changes seem good enough.

A bit messy example and the UI on the screen is quite something, but that's how it already was :)

@pvuorela pvuorela merged commit 5c3c7c4 into sailfishos:master Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants