Auto refresh imenu

[jcs090218]

This PR is for feature #511 and #295.

I have currently bind to only after-save-hook. Open for any other suggestion! Thanks!

[brotzeit]

I think after-save-hook is fine for now, but @yyoncho suggested using a timer. IMO we should merge this and update the code when we have a more sophisticated solution.

[jcs090218]

I think after-save-hook is fine for now, but @yyoncho suggested using a timer. IMO we should merge this and update the code when we have a more sophisticated solution.

I was thinking about using the timer too. This patch is indeed a quick solution; I am looking forward to update this in the near future.

[jcs090218]

I have managed to use timer and after-change-functions. I also added lsp-ui-util.el file, but currently there aren't much yet. What do guys think about the utility module? I think is quite nice to have a helper file that holds all the generic functions/macros. Thanks!

[brotzeit]

Tested it and seems to work correctly :+1:

[brotzeit]

@jcs090218 do you know that you can close issues automatically https://docs.github.com/en/enterprise/2.13/user/articles/closing-issues-using-keywords ?

[jcs090218]

do you know that you can close issues automatically

Yeah, I think I knew it, but I like to do it separately. Maybe I am just not use to this feature yet.

@brotzeit I guess we can merge this now and improve in the next patch?

Thanks!

[brotzeit]

I'd like to hear @sebastiencs opinion first.

[sebastiencs]

Thanks for working on this @jcs090218

I think after-save-hook is fine for now, but @yyoncho suggested using a timer.

I'm not sure if using a timer on after-change-functions is the best idea, IMO the lsp-ui-imenu window is not really relevant while typing. And this could make quite a lot of requests, with potentially big buffers/slow servers.

What about refreshing it just on after-save-hook ? I think it's a more appropriate hook to use, and without the need of a timer

@yyoncho In what use cases a timer would be beneficial ?

[jcs090218]

I'm not sure if using a timer on after-change-functions is the best idea, IMO the lsp-ui-imenu window is not really relevant while typing. And this could make quite a lot of requests, with potentially big buffers/slow servers.

I think is quite nice to have a updated imenu while typing; of course, this kind of action would take some performance. Have you tried it yet? On my end it works quite well. I have managed to use run-with-idle-timer and set to the default delay to 1.0 seconds. It's quite hard to send a refresh request unless you type in a speed of 1 second per letter. Then yes, that's the worst case scenario. In average, I think the user can tweak the lsp-ui-refresh-delay base on their own typing habit.

What about refreshing it just on after-save-hook ? I think it's a more appropriate hook to use, and without the need of a timer

I'm fine with this one too. Let me know if I need to revert my updates! Thanks!

[brotzeit]

IMO you don't use imenu all the time, but only in certain situations and therefore it's not a big issue that the performance isn't optimal. However it would be nice if imenu would also update the buffer when you are focusing a different buffer than the initial buffer. I just realized that's currently not the case right ?

[brotzeit]

@ulises what do you think ?

[yyoncho]

@yyoncho In what use cases a timer would be beneficial ?

this is what other editors do. Also, there is lsp-on-change-hook and we do load the symbols info for other components with no perf issues. lsp-treemacs-symbols has optimized rendering since it updates only the changed elements.

[ulises]

Apologies for the delay in answering.

My use case would be to have lsp-symbols-imenu open side by side with a buffer that contains a class (or equivalent). So, I'd personally favour 2 hooks:

• when the main buffer changes (you're working now on a different class or equivalent)
• when the buffer is saved (you added/removed something in the current class or equivalent)

I'm not sure a timer makes sense considering that workflow, but perhaps I'm just too focused on my own needs :)

[ulises]

A further comment, this time on behaviour though: I noticed that when you press <RET> on an imenu item the accompanying buffer scrolls to the symbol, but the focus/pointer remains on the imenu buffer. I suspect it'd be nicer if the pointer was placed in the other buffer (perhaps at the beginning of the symbol?).

[wakira]

the focus/pointer remains on the imenu buffer

@ulises Pressing META-RET should bring it to the main buffer. Have a try

[jcs090218]

So, I'd personally favour 2 hooks:

I think the current implementation should be good for the 2 use cases you mentioned by binding the after-change-functions. :)

[brotzeit]

I would give it a try and see if people complain about performance issues.

[brotzeit]

Would this be ok for you @sebastiencs ?

[brotzeit]

Maybe it would also make sense to allow the user to choose if he wants to use a timer or not.

[sebastiencs]

Would this be ok for you @sebastiencs ?

Alright, let's do it with a timer. Can we just make sure that the request is made asynchronously ? To not block while waiting for the server. I don't remember how is made the request but if someone could check if it's already async, if not I think it needs to be implemented @jcs090218

[jcs090218]

Can we just make sure that the request is made asynchronously ? To not block while waiting for the server.

I don't remember how is made the request but if someone could check if it's already async, if not I think it needs to be implemented @jcs090218

Any suggestion to make it async? As I have tested; I think the current implementation isn't async? Overall I am not sure if the execution is async/sync because the overall execution goes very fast and I did not feel the delay, yet I assumed is sync.

Right now I have implemented the refresh functionality by reusing the same function lsp-UI-imenu. I mean it would be great to have some suggestions from guy who created this function. @sebastiencs Is it you? :)

Maybe it would also make sense to allow the user to choose if he wants to use a timer or not.

@brotzeit I think this is a good solution.

[sebastiencs]

@jcs090218 I just check the code and it's not async. lsp-ui-imenu calls imenu--make-index-alist which calls lsp--imenu-create-index. lsp--imenu-create-index blocks until it receives a response from the server.

the overall execution goes very fast and I did not feel the delay

That's true with your specific configuration and server. Some lsp servers might be slow and calling lsp--imenu-create-index in a timer will degrade the user experience. It is not difficult to find on google comments about lsp-ui/mode being slow, so I prefer to be very careful with this kind of thing.

Maybe it would also make sense to allow the user to choose if he wants to use a timer or not.

@brotzeit I think this is a good solution.

Since making it async will require some more work/refactoring, we can do it in another PR. Can we disable the timer by default, for this PR ? Until we find a solution to do it asynchronously. If you want to work on that @jcs090218 I can help you.

I would give it a try and see if people complain about performance issues.

I don't think it is the best strategy. A few users will complain in the issue tracker, most of them will just stop using lsp-ui.

[jcs090218]

I just check the code and it's not async.

Got you! Thanks for your confirmation! 👍

It is not difficult to find on google comments about lsp-ui/mode being slow, so I prefer to be very careful with this kind of thing.

Yes, of course. I agree with you.

Since making it async will require some more work/refactoring, we can do it in another PR. Can we disable the timer by default, for this PR ? Until we find a solution to do it asynchronously.

Applied! @sebastiencs Let me know if this work! :)

[brotzeit]

@jcs090218 can you please bump the version as the last commit.

[jcs090218]

can you please bump the version as the last commit.

I have bumped version to 6.3. Did that work for you? Thanks!

[brotzeit]

I have bumped version to 6.3. Does that work for you? Thanks!

I mean just before we are merging this pull request, so every commit is included in the new version.

[jcs090218]

@brotzeit Got it. Just want to confirm if I did that correctly.

[jcs090218]

Okay, I think I have implemented this the way @sebastiencs wants. Let me know if something you want me to change and I will fix it in the later patch! Merge this now!