search

nobody-famous / alive-lsp

Language Server Protocol implementation for use with the Alive extension
81 stars 9 forks source link

Remove extraneous refresh notification #46

Closed madkins23 closed 1 year ago

madkins23 commented 1 year ago

A simple call to evaluate a text string containing LISP results in two refresh notifications, one before the result and one after:

16:44:02 INF Send !=tester-->server #size=127 $Type=request %ID=1092 %method=$/alive/eval <package=cl-user <storeResult=true <text="(+ 2 (/ 15 5))"
16:44:02 INF Rcvd !=tester<--server #size=58 $Type=notification %method=$/alive/refresh
16:44:02 INF Rcvd !=tester<--server #size=51 $Type=response %ID=1092 <>method=$/alive/eval <>package=cl-user <>storeResult=true <>text="(+ 2 (/ 15 5))" >text=5
16:44:02 INF Rcvd !=tester<--server #size=58 $Type=notification %method=$/alive/refresh

On the client side the refresh notifications result in multiple calls to update information. Alive PR 146 would add one (or possibly two) more calls to the server to refresh data. While these calls are quite fast it seems like one of the is redundant.

It looks like the run-in-thread calls run-fn which sends out the first refresh notification before running the code and then afterwards run-in-thread issues the second one. Since the call to run-fn is made inside of an unwind-protect which also contains the refresh notification of run-in-thread it seems like there is no down side to removing the run-fn notification.

I also checked for other calls to run-fn but it seems to only be used within run-in-thread so there shouldn't be any negative fallout from removing the redundant call.

nobody-famous commented 1 year ago

That call is actually necessary, it's mainly to update the threads. Without it, the threads view doesn't get updated during a long running function. I think I'll move it to run-in-thread, because it makes a little more sense there, so I'll just add it to the work I'm currently doing.

madkins23 commented 1 year ago

That call is actually necessary, it's mainly to update the threads. Without it, the threads view doesn't get updated during a long running function. I think I'll move it to run-in-thread, because it makes a little more sense there, so I'll just add it to the work I'm currently doing.

That makes sense. It does make me think about extending the refresh notification to specify one or more specific elements to refresh, with the default (no specifications) being to do all of them. I suspect I'm over-thinking it, everything runs pretty fast and local CPU cycles are essentially free.