dictation-toolbox / natlink

Natlink provides the interface between Dragon and python
https://dictation-toolbox.github.io/natlink/
Other
25 stars 17 forks source link

Add equivalent "getter" functions for global callbacks #128

Closed drmfinlay closed 1 year ago

drmfinlay commented 2 years ago

I wonder if additional getBeginCallback, getChangeCallback and getTimerCallback functions might be added to the natlink extension module?

This would only be a minor change. It would allow for more flexible use of the lower-level callbacks. If I recall correctly, the natlinkcore loader module is somewhat dependent on no other modules setting the Change callback. As such, I'd say this feature request is related to separating the "pure" Python parts of the project into natlinkcore.

I am familiar with Python's C/C++ API and the relevant code in NatlinkSource, so I would be happy to implement this myself and submit a pull request. That is, if it is likely to be accepted.

One argument against adding these functions is that they would be new features in a library that hasn't changed much, at least on the surface, in quite a long time. They would have to be documented somewhere as "new in Natlink version X."

dougransom commented 2 years ago

This seems reasonable to me.

It would be appreciated if the changes were also written up in the documentation for natlink, which I don't know what the current state is anyway. Also appreciated would be any additional testing in natklink/tests using either pytest or unitteset.

I had a thought that all the functions in pythwrap.cpp should be wrapped in pure python in __int__.py, for the purpose of making it easier to set breakpoints when working with Python code. Maybe that could be done at the same time? I had some code and instructions for debugging Python within natlink, but that fork was discarded and I haven't ported the changes yet. On my list…

drmfinlay commented 2 years ago

All right then, I'll start implementing this soon. It should be simple to include similar functions for the GramObj and DictObj classes.

I'll add documentation for the new functions to the library documentation, which currently resides in the natlink.txt and natlink12.txt files under NatlinkSource. (I don't recall file which is more up-to-date.) Quintijn and I have discussed making this documentation more accessible. We've made some progress in that direction.

My pull request will include unit tests in the appropriate file(s), of course.

Concerning Python wrapper functions in __init__.py, I have some reservations. It's up to you, of course, but it doesn't seem like a good replacement for mixed-mode debugging. In any case, it would be more involved than adding new "getter" functions; I think the extension module would have to be renamed to _natlink (or similar).

dougransom commented 2 years ago

I already wrapped a couple functions from pythwrap.cpp in __init.py__ to solve https://github.com/dictation-toolbox/natlink/issues/113, though I think i now know how to solve it in C++ instead. But I don't plan to spend the effort. One would have to rollback the changes I made in pythwrap.cpp (pretty minimal) and __init.py__ do the windows encoding in C++ . I think there is another function in pyhtwrap.cpp that shows how do this in C++ that I just wasn't aware of when solving the problem. I don't have a full understanding of how natlink works but learning more all the time.

drmfinlay commented 2 years ago

All right then. Getting Python string/unicode encoding done correctly can be a pain, especially in C/C++. I can have a look into that too.

Can I ask which branch to base my changes on? I see there are a few.

dougransom commented 2 years ago

master.

drmfinlay commented 2 years ago

Okay then, thanks.

quintijn commented 2 years ago

I begin to understand! then also add getHypothesisCallback for completeness!

drmfinlay commented 2 years ago

I have made some progress on this. However, after reading the documentation for Natlink's getCallbackDepth() function, I feel it would be best to leave the callback mechanism opaque.

https://github.com/dictation-toolbox/natlink/blob/ae2f65db16c643ce62399e453d10227da3a03697/NatlinkSource/natlink.txt#L200-L207

If a callback were to be invoked manually from the Python layer, the depth number would not increase.

In any case, the CallbackHandler class recently added and utilised by the natlinkcore loader appears to have made my proposed changes mostly unnecessary.

LexiconCode commented 2 years ago

Would it be possible to return how long it takes for back to complete in some sort of debugging the function?

Useful for scenarios to see if call backs are tying up the main thread by taking too long to exe.

quintijn commented 2 years ago

Yeah, and there are callbacks per grammar which never can be called from natlink directly, but from the grammars.

Please inform me for what you used the callbackhandler, just curious!

drmfinlay commented 2 years ago

Would it be possible to return how long it takes for back to complete in some sort of debugging the function?

Useful for scenarios to see if call backs are tying up the main thread by taking too long to exe.

That could be done with a profiler. Try the following code in a natlink command module:

import cProfile
import natlink

def begin1(pInfo):
    print("begin1(): hwnd=%d" % pInfo[2])

def begin2(pInfo):
    cProfile.runctx("begin1(pInfo)", globals(), locals())

natlink.setBeginCallback(begin2)  # Pass `None' to unset.

Please inform me for what you used the callbackhandler, just curious!

I haven't used it yet actually, I just noticed it. The class should make it easier to register for change events (e.g., microphone ON) without using natlink.setChangeCallback() directly. (Doing so breaks the module reload feature.) This was a bit tricky in earlier versions.

drmfinlay commented 1 year ago

I think this should be closed. I won't be implementing these "getter" functions. Users just have to be careful setting some callbacks directly, that's all.