Conversation
|
@eric-stokes-architect Some implementation details to be aware of:
|
|
This looks awesome! I will review it as soon as possible.
Sent from Proton Mail Android
…-------- Original Message --------
On 3/9/25 23:22, base1172 wrote:
***@***.***(https://github.com/eric-stokes-architect) Some implementation details to be aware of:
- Excel doesn't expose a trivial way to know when a UDF is no longer in use, which means that the =NetSet() UDF doesn't have a way to drop subscriptions that are no longer in use. I believe this is a solvable problem, but this PR doesn't try to tackle it. For now, any subscriptions opened by =NetSet() will remain open until the process terminates.
- The =NetSet() UDF is marked as non-volatile. This means if a workbook is saved and then reopened, cells that use =NetSet() will only write a new value if an input changes. If no inputs change, no value will be written. This can be unintuitive for users, as they probably expect =NetSet() to re-write all values exactly once when the workbook is loaded.
- All interaction with Excel is done via the Excel XLL SDK. Bindings for the SDK were generated with bindgen and are in src/xll_utils/xlcall.rs. Instead of making this a static file, we could generate the bindings at build time; however, the user would need to have the Excel XLL SDK headers installed to make it work.
- With this PR, the add-in will now register its COM RTD server automatically when the add-in is loaded by Excel, with registry entries placed in the user hive instead of HKEY_CLASSES_ROOT. From the user's perspective I believe this behavior is strictly better...the add-in should "just work" as soon as it is loaded in Excel, without users needing admin privileges or running regsvr32.
—
Reply to this email directly, [view it on GitHub](#3 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABWJHLDKNFDXCQNOCU6T2VT2TUAOJAVCNFSM6AAAAABYU2JZSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBZGMZTAOBZGI).
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
[base1172]base1172 left a comment [(netidx/netidx-excel#3)](#3 (comment))
***@***.***(https://github.com/eric-stokes-architect) Some implementation details to be aware of:
- Excel doesn't expose a trivial way to know when a UDF is no longer in use, which means that the =NetSet() UDF doesn't have a way to drop subscriptions that are no longer in use. I believe this is a solvable problem, but this PR doesn't try to tackle it. For now, any subscriptions opened by =NetSet() will remain open until the process terminates.
- The =NetSet() UDF is marked as non-volatile. This means if a workbook is saved and then reopened, cells that use =NetSet() will only write a new value if an input changes. If no inputs change, no value will be written. This can be unintuitive for users, as they probably expect =NetSet() to re-write all values exactly once when the workbook is loaded.
- All interaction with Excel is done via the Excel XLL SDK. Bindings for the SDK were generated with bindgen and are in src/xll_utils/xlcall.rs. Instead of making this a static file, we could generate the bindings at build time; however, the user would need to have the Excel XLL SDK headers installed to make it work.
- With this PR, the add-in will now register its COM RTD server automatically when the add-in is loaded by Excel, with registry entries placed in the user hive instead of HKEY_CLASSES_ROOT. From the user's perspective I believe this behavior is strictly better...the add-in should "just work" as soon as it is loaded in Excel, without users needing admin privileges or running regsvr32.
—
Reply to this email directly, [view it on GitHub](#3 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABWJHLDKNFDXCQNOCU6T2VT2TUAOJAVCNFSM6AAAAABYU2JZSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBZGMZTAOBZGI).
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
|
In the initial version of this PR there was a subtle memory leak in the I've updated the PR to fix that issue and manually confirmed that the new implementation no longer leaks. For background on xlbitXLFree and Excel memory management, see https://learn.microsoft.com/en-us/office/client-developer/excel/memory-management-in-excel#returning-xloperxloper12s-to-be-freed-by-excel |
|
I've disabled registration of the I don't know why Excel leaks memory when using the I plan to revisit this issue at a later date, as using |
|
Have you tried wrapping it in VBA? I know that's not an ideal situation, but IIRC it may not leak memory. It may also not perform as well. Don't we just LOVE Excel 😂 |
estokes
left a comment
estokes
left a comment
There was a problem hiding this comment.
This is a solid piece of work, thanks! I have a couple of small comments, and one bigger suggestion.
|
|
||
| ``` | ||
| =RTD("netidxrtd",, PATH) | ||
| =NetGet(PATH) |
There was a problem hiding this comment.
If we're rolling back wapping in a UDF then fix this doc
| "Win32_System_Threading", | ||
| "Win32_Security", | ||
| "Win32_Security_Authorization", ] } | ||
|
|
There was a problem hiding this comment.
Wow, thank you for updating this. Microsoft pretty much rewrites the public API of the windows crate every time they release a new version.
| return 0; | ||
| } | ||
| _ => (), | ||
| } |
There was a problem hiding this comment.
They seriously switched to Result in 0.48 and then back to HRESULT in 0.60 ...
would this maybe look nicer as
if CoInitialize(None).is_err() {
... error stuff
}
| Ok(s) => match NetSetType::try_from(ty) { | ||
| Err(()) => XLOper12::error(XlErr::NA).into(), | ||
| Ok(typ) => { | ||
| let path: netidx::path::Path = Into::<netidx::path::Path>::into(s); |
There was a problem hiding this comment.
Are both annotations really needed?
| rt.block_on(async move { Subscriber::new(cfg, desired_auth) })?; | ||
| std::thread::Builder::new().name("netidx-setter".into()).spawn(move || { | ||
| // TODO: Add support for closing unused [Dval] by integrating with the RTD server. We need to marshal [Dval] ids to/from strings when a new path is subscribed or when an old path is dropped. | ||
| // Paths that are already subscribed and still in use won't need to be marshaled. |
There was a problem hiding this comment.
Dvals are automatically shared on the same subscriber, so all you would need to do to make this happen is ensure that you are using the same subscriber for RTD and for write. Then, when you write, just subscribe to the path again, and you'd get back a clone of the existing Dval, do the write, and then drop it when done, the subscription will be kept alive as long as the RTD server is still using that subscription.
There's no easy way we're ever going to be able specify a netidx config for Excel, so at this point I'd support initializing netidx, the async runtime, and the subscriber in a global LazyLock. Then we can share the subscriber everywhere it needs to be shared, and avoid a lot of problems.
after this you don't need a hashmap, you should be able to just call subscriber.subscribe(path).write(value), since everything related to Dvals is ordered this should work even if we were not previously subscribed.
| @@ -0,0 +1,128 @@ | |||
| mod registration; | |||
There was a problem hiding this comment.
This is a solid piece of work, should it be a separate crate? It seems general enough that others may wish to use it.
3 participants
This PR makes several changes:
=NetSet()UDF=NetGet()UDF=RTD()irtd_update_event_threadthat could cause itsIStreamobject to be released twiceonce_cellcrate (that functionality is now available instd::sync::LazyLock)winregcrate.