filcuc/nimqml

Passing QVariant(QObject) with float property to QML results in wrong data

arkanoid87 opened this issue · 14 comments

arkanoid87 commented

ints and strings are OK, but float properties end up with wrong data in QML.

I've modified slotsandproperties example to expose this bug

append intNumber and floatNumber (read only) properties in contact.nim

import NimQml

QtObject:
  type Contact* = ref object of QObject
    m_name: string
    m_intNumber: int
    m_floatNumber: float

  proc delete*(self: Contact)
  proc setup(self: Contact)
  proc newContact*(): Contact =
    new(result, delete)
    result.m_name = "InitialName"
    result.m_intNumber = 42
    result.m_floatNumber = 42.12345
    result.setup

  proc delete*(self: Contact) =
    self.QObject.delete

  proc setup(self: Contact) =
    self.QObject.setup

  proc getName*(self: Contact): string {.slot.} =
    result = self.m_name

  proc nameChanged*(self: Contact, name: string) {.signal.}

  proc setName*(self: Contact, name: string) {.slot.} =
    if self.m_name == name:
      return
    self.m_name = name
    self.nameChanged(name)

  QtProperty[string] name:
    read = getName
    write = setName
    notify = nameChanged

  proc getIntNumber*(self: Contact): int {.slot.} =
    self.m_intNumber

  QtProperty[int] intNumber:
    read = getIntNumber

  proc getFloatNumber*(self: Contact): float {.slot.} =
    self.m_floatNumber

  QtProperty[float] floatNumber:
    read = getFloatNumber

print values in qml

import QtQuick 2.8
import QtQuick.Controls 2.3
import QtQuick.Layouts 1.3

ApplicationWindow {
    width: 400
    height: 300

    Component.onCompleted: {
        visible = true;
        console.log(contact.intNumber)
        console.log(contact.floatNumber)
        console.log(Number.isSafeInteger(contact.floatNumber))
    }

    ColumnLayout {
        anchors.fill: parent

        Label { text: "Current name is:" + contact.name }

        TextField { id: textField }

        Button {
            text: "Change Name"
            onClicked: contact.name = textField.text
        }
    }
}

expected output:

qml: 42
qml: 42.12345
qml: false

current output:

qml: 42
qml: 2
qml: true

DOtherSide version: v0.8.2 [7b7c0a91b558b13a968c57b9647bfc15ed962ead]

I can push this to a new branch if requested

arkanoid87 commented

If I pass the same value as QVariant float, it works

adding to previous example:

  proc getFloatNumberAsVariant*(self: Contact): QVariant {.slot.} =
    newQVariant(self.m_floatNumber)

  QtProperty[QVariant] floatNumberAsVariant:
    read = getFloatNumberAsVariant
    Component.onCompleted: {
        visible = true;
        console.log(contact.intNumber)
        console.log(contact.floatNumber)
        console.log(Number.isSafeInteger(contact.floatNumber))
        console.log(contact.floatNumberAsVariant)
    }

results:

qml: 42
qml: 2
qml: true
qml: 42.123451232910156

I've found here a discussion about using QVariant as property type vs the inner type:
https://stackoverflow.com/questions/45613030/usage-of-qvariants-in-c-and-properties-in-qml

filcuc commented
  • 👍1
arkanoid87 commented

np, I can use QVariant properties in the meanwhile, but actually I'm not sure how to handle memory here

filcuc commented
arkanoid87 commented

TL;DR; why here delete is never called on newQVariant?

nimqml/examples/contactapp/contactlist.nim

Line 40 in 4f237b4

of ContactRoles.FirstName: result = newQVariant(contact.firstName)

I mean, I'm not sure if I should manually call delete for every QObject or QVariant instantiated on the Nim side.

In nimqml examples I generally see that:

In my experiments I'm adding QVariant objects inside other QVariant objects and it works, but I'm having some hard time figuring out leaks with valgrind so I'm better asking you

This is the toy project where I'm applying the concepts

To be more specific, here I have a custom QObject named QGeometry and it holds geometry information for 2d Point (list of 2 floats), polygons (list of Point) and so on.
The result is a lot of QVariant wrapping, but leaks seems happening and I don't know the general scheme around this

arkanoid87 commented

I'm trying to figure out some general rules when to delete QObjects and QVariants.
I'm using also the code in https://github.com/status-im/status-desktop as example. I just need some confirmation from you:

filcuc commented

@arkanoid87 simply don't delete nothing if don't need explicit deletion. This will be due automatically by nim. Otherwise if you need more deterministic deletion use ARC or ORC.
That said:

  1. for qvariant that are return value of a proc is just stupid to delete them. Because they're should stay valid for all the scope of the parent stack call. i.e if i call model->data() and inside data you delete the return value i will obtain a dengling qvariant....so don't delete the result of proc
  2. The link "but here they dont the opposite" is totaly different since they're deleting the qvariant in the "delete" proc that is called from the class finalizer. So it's correct. Like destroying objects in the destructor of a class (if you know c++ or any language that have destructors)
  3. QObject follow the normal GC/scope rules. If you keep a reference to a QObject it will not be deleted. It will be deleted when the number of reference goes to 0.
filcuc commented

Regarding why most of examples have the explicit delete is just because initial release of nim use a not deterministic GC. If you latest version and use ORC or ARC you will have automatic deletion of objects on scope close

filcuc commented

If you test leaks with valgrind you need:

  1. To understand where leaks are coming
  2. Qml use a Javascript engine that is not deterministic so memory will be delete when the Javascript garbage collector wants or think it's appropriate
  3. Nim 1.2 and (maybe later) don't enable reference counting gc (i.e ARC/ORC) on by default. Better to check valgrind by using this GC strategies (see nim --gc:arc/orc)
arkanoid87 commented

EDIT: I've reproduced the issue outside nimqml, I've created a post here https://forum.nim-lang.org/t/8817#57493
Seems that forwarding delete function as finalizer won't work with RootObj. Here the difference:

It is probably required to rewrite the finalizers to avoid forwarding declaration, or maybe switch to destructors. Relevant forum thread

just a quick example

If ARC/ORC should take care of running object finalizer when object goes out o scope, why this example does not print QQmlApplicationEngine: delete?

config.nims

--gc:orc
--d:debug

main.nim

import nimqml

proc main =
  var app = newQApplication()
  var engine = newQQmlApplicationEngine()

  engine.load("qml/main.qml")
  app.exec()
  echo "main scope end"

when isMainModule:
  main()
  echo "goodbye"

main.qml

import QtQuick 2.2
import QtQuick.Controls 1.2

ApplicationWindow {
    Component.onCompleted: {
        Qt.callLater(Qt.quit)
    }
}

output

NimQml: QMetaObject: newQObjectMetaObject
NimQml: QMetaObject: newQAbstractItemModelMetaObject
NimQml: QMetaObject: newQAbstractListModelMetaObject
NimQml: QMetaObject: newQAbstractItemTableMetaObject
QSocketNotifier: Can only be used with threads started with QThread
Using Wayland-EGL
main scope end
NimQml: QApplication: delete
goodbye

QQmlApplicationEngine delete proc seems never called

here's the output of --expandArc:main

var
  app
  engine
try:
  app = newQApplication()
  engine = newQQmlApplicationEngine()
  load(engine, "qml/main.qml")
  exec(app)
  echo ["main scope end"]
finally:
  `=destroy`(engine)
  `=destroy_1`(app)

not sure about the difference from =destroy and =destroy_1, but seems to discriminate the working finalizer for QApplication and the not working one for QQmlApplicationEngine

filcuc commented

@arkanoid87 can we create a new topic regarding leaks instead of injecting this one? sorry but it's excaliting to another issue instead of the float/qvariant.
That said we need to ask to Nim compiler devs what's going on...seems like a compiler issue imho. NimQml is compiled in one big module since all its parts are "included" (instead of imported) in the nimqml.nim module.
That said i can give it a look

filcuc commented

@arkanoid87 Switching from finalizers to destructors is impossible without breaking existing code. The problem is due to destructors taking a var T but finalizers ref T.

arkanoid87 commented

@arkanoid87 Switching from finalizers to destructors is impossible without breaking existing code. The problem is due to destructors taking a var T but finalizers ref T.

I'm already doing some experiments on this

case n==0: destroy can be declared on base non-ref type and it would be called on ref object dealloc, it is possible to call parent destroy
case n==1: finalizer is called on leaf (real) type only and seems not possible to easily call parent finalizer without additional code

type
  FooObj = object of RootObj
    fooValue: int
  BarObj = object of FooObj
    barValue: int
  Foo = ref FooObj
  Bar = ref BarObj

const n = 0

when n == 0:
  proc `=destroy`(x: var FooObj) =
    echo "Goodbye Foo " & $x.fooValue
  
  proc `=destroy`(x: var BarObj) =
    x.FooObj.`=destroy`
    echo "Goodbye Bar " & $x.barValue

  proc newFoo: Foo =
    new(result)
    result.fooValue = 99

  proc newBar: Bar =
    new(result)
    result.fooValue = 42
    result.barValue = 19

when n == 1:
  proc finalize(x: Foo) =
    echo "Goodbye Foo " & $x.fooValue

  proc finalize(x: Bar) =
    x.Foo.finalize() # Error: cannot bind another '=destroy' to: FooObj
    echo "Goodbye Bar " & $x.barValue

  proc newFoo: Foo =
    new(result, finalize)
    result.fooValue = 99

  proc newBar: Bar =
    new(result, finalize)
    result.fooValue = 42
    result.barValue = 19


proc main =
  var foo = newFoo()
  echo foo.fooValue
  var bar = newBar()
  echo bar.fooValue
  echo bar.barValue

if isMainModule:
  main()
  GC_fullCollect()
arkanoid87 commented

@arkanoid87 can we create a new topic regarding leaks instead of injecting this one? sorry but it's excaliting to another issue instead of the float/qvariant. That said we need to ask to Nim compiler devs what's going on...seems like a compiler issue imho. NimQml is compiled in one big module since all its parts are "included" (instead of imported) in the nimqml.nim module. That said i can give it a look

ok, moving previos post to new issue