go-qml / qml

QML support for the Go language
Other
1.96k stars 187 forks source link

Expose QQmlEngine::clearComponentCache() #36

Closed janimo closed 10 years ago

niemeyer commented 10 years ago

This looks good, thanks Jani. One last note in addition to the trivial inline comments: can we please have a test for it? Should be easy to have one that changes content and retries, I suppose? There are quite a few in all_test.go to be inspired from (see the table).

quarnster commented 10 years ago

Any progress on getting this merged? I don't think hot-reloading qml files is possible without it. I've got my own local qml tweaked, but this together with fsnotify is a huge productivity boost others would benefit from.

Here's a code snippet that could serve as a test base:

    watcher, err := fsnotify.NewWatcher()
    if err != nil {
        log.Fatalln(err)
    }
    defer watcher.Close()
    watcher.Watch("./")
    component, err := engine.LoadString("blah.qml", `
import QtQuick 2.0
import QtQuick.Layouts 1.0
import QtQuick.Controls 1.0
import QtQml.Models 2.1

ApplicationWindow {
    width: 1024
    objectName: "appWindow"
    id: appWindow
    height: 768
    function reload() {
        console.log("in reload");
        loader.source = "";
        engine.clearComponentCache();
        loader.source = "./main.qml";
    }
    Loader {
        id: loader
        focus: true
        anchors.fill: parent
        source: "./main.qml"
    }
}`)

    if err != nil {
        log.Fatalln(err)
    }
    window := component.CreateWindow(nil)
    go func() {
        for {
            select {
            case ev := <-watcher.Event:
                log.Printf("%+v", ev)
                if ev.IsModify() && !ev.IsAttrib() {
                    window.Call("reload")
                }
            }
        }
    }()
niemeyer commented 10 years ago

Sorry for being slow on this, and thanks so much for providing a use case @quarnster. The reason this is taking a bit is that I'm not quite comfortable with the idea of clearing components, because apparently it breaks what's already loaded in memory:

All previously loaded components and the property bindings for all extant objects
created from those components will cease to function.

(from http://goo.gl/N4azMC)

It's great that you provided an example for what you need to do, though. I'll spend some time on it today and get back to you on this.

@janimo Is that your use case as well?

janimo commented 10 years ago

It's my turn to say sorry, but I could not come up with an actual test for this function only. My use case was indeed a patch to the current qmlscene.go which used inotify to hotload the qml on save, thus providing a really nice feedback loop.

janimo commented 10 years ago

Gustavo, I understand you're not being comfortable with the idea of clearing components, but this is one API (one exposed to C++ developers already) that is up to people to use or not depending on their needs.

niemeyer commented 10 years ago

@janimo That logic does not hold. The mere existence of a method in C++ is not a reason to have it. A method that invalidates all previous logic in a way that makes it crash is a bad API, and I'm not keen on having it without proper reasoning and understanding.

Why is this any better than creating a new engine, for example?

janimo commented 10 years ago

It is not the mere existence of the method in C++, from what I see it is a public API of the Engine and AIUI (or maybe misunderstood) one of the aims of go-qml is to allow access to the same APIs as would be available from C++. As for other ways of doing live-reload, there may be others, this is the one I found when I needed it at the time. I don't know much about Qt/Qml internals to assess which existing method is best.

niemeyer commented 10 years ago

The main goal of Go QML is to offer a great API for creating GUI applications in Go. That conflicts with the existence of a method whose use case needs are unclear and that has a consequence of easily crashing the application.

niemeyer commented 10 years ago

I'm closing this for now. If somebody understands why creating a new engine is not enough, please reopen with details.

janimo commented 10 years ago

You won't solve all cases of PEBKAC however judiciously you pick your public facing API. I suppose there are ways you can misuse the current API in ways that crashes the apps. You can aim for extra safety but then accept that even one such missing API can make go-qml not be the choice for people who would have replaced C++ when writing QML apps.

As for the creating a new engine maybe it does a lot more than creating component cache (loads the JS engine) and is considered too much for some scenarios.

niemeyer commented 10 years ago

Sorry, but that's not the way we'll design Go QML's API. It's not about having every single C++ method available irrespective of what it does, and it's not about adding features because you have not investigated the better alternative.

quarnster commented 10 years ago

FWIW clearComponentCache is recommended all over the net.

I actually didn't stumble upon this issue number until I had already added searched the web and was about to submit a pull request similar to @janimo's.

Why is this any better than creating a new engine, for example?

That's a good idea that I haven't seen recommended anywhere else, I'll report back here if I find out that there is indeed a deal-breaking reason this isn't recommended elsewhere ;)

FWIW, the Qt developers themselves appear to recommend trimComponentCache for this use case, which makes sense as it only clears unused components. (This makes it possible to reload sub-component(s) specified in separate .qml files loaded via a loader component, but IMO the complexity involved in getting that to work is hard to justify.)

quarnster commented 10 years ago

Creating a new engine appears to work just as well for me. For reference here's a cut'n'paste of some quickly thrown together code that does the "new engine" approach .

    qml.Init(nil)
    watcher, err := fsnotify.NewWatcher()
    if err != nil {
        log.Fatalln(err)
    }
    defer watcher.Close()

    const qmlfile = "myroot.qml"
    watcher.Watch(qmlfile)
    var (
        engine    *qml.Engine
        window    *qml.Window
        reloading = true
    )
    reload := func() {
        if engine != nil {
            engine.Destroy()
        }
        engine = qml.NewEngine()
        // ... cut out engine variable bindings 
        component, err := engine.LoadFile(qmlfile)

        if err != nil {
            log.Fatalln(err)
        }
        window = component.CreateWindow(nil)
        window.Show()
    }
    go func() {
        for {
            select {
            case ev := <-watcher.Event:
                log.Printf("%+v", ev)
                if ev.IsModify() && !ev.IsAttrib() {
                    reloading = true
                    window.Hide()
                }
            }
        }
    }()
    for reloading {
        reload()
        reloading = false
        window.Wait()
    }
niemeyer commented 10 years ago

@quarnster Sweet, thanks a lot for investing this.