fix devtools processes

[Charly6596]

Added an autocmd triggered when vim is closed to kill the devtools process if it was started by the user.
As mentioned in #41, the flutter run command starts a dev tools server if running on mobile and has a parameter to start an already running dev tools server url, so I check if we are running it and pass the url if so.

The autocmd could be used to cleanup other modules if needed.

There is an issue, if we are not running devtools and we start the app, if we wanted to copy the url we would need to start another devtools process with the command, we would have 2 processes, one manage by flutter and another by our plugin
I think we have 3 options

1. Keep it as it is.
2. Always run the dev tools server before running flutter run and pass the url as parameter
3. Let flutter start the server and pass its url it to the dev_tools module, preventing us from starting another server.

I prefer the second one myself but I would like to know what you think.

[akinsho]

@Charly6596 this looks good to me, regarding the flutter process versus our own, tbh if flutter is already doing it, I'm not sure why try and take control of it rather than just track the url it creates in the devtools module i.e option 3.

What would be the benefit of method 2 i.e. creating it and then passing it to the run command? not to say that there'd be no value I'm just not sure what it would be

[akinsho]

Will killing the job via it's PID cause any side effects for plenary jobs? they have a close method which I'm sure does this at some point under the hood I'm just wondering what they do if the underlying process dies outside of it's control.

It's not a big deal since this is on close but will the user see any messy errors on vim leave?

[Charly6596]

Actually yes, if you execute the stop function and the server is running you can see the errors (not if exiting)
lua require('flutter-tools.dev_tools').stop()

Error executing vim.schedule lua callback: /home/charly/.config/nvim/lua/flutter-tools/dev_tools.lua:52: bad argument #1 to 'ipairs' (table expected, got strin
g)

We get the same behaviour if we call job:shutdown() inside M.stop() and kill the process in our on_exit handle and that is because they pass nil to the self._user_on_stdout and self._user_on_stderr functions here

The readers are linked to self._user_on_stdout and self._stderr_reader

So we would avoid getting that message if we check for nil at the start of our handle_error and handle_start functions.
In their specification they don't specify the data parameter as nullable so I think we should open an issue for that

I'm assuming Job:shutdown is being called when we kill the dart devtools process.
If we close the job using :shutdown, devtools is still there, that's why I kill the process. Maybe there is another way, but I couldn't figure out.
By the way, if we do job:shutdown before killing the process, it takes way more time to close neovim, whereas I can't notice any slowdown if we kill directly

[Charly6596]

Now that I check the error message it says that a string was received, so, errr, what I said applies for executing job:shutdown , maybe my assumption is wrong and they don't call it when devtools dies, will check that out later

[Charly6596]

Ok, just checked, here's the result

• If we kill the process directly, we get the following in the on_stderr function (second message, we would need to check if the data parameter is a table, otherwise ignore it)
  
• If we do job:shutdown before killing the process, we get nil in both functions:

Error executing vim.schedule lua callback: /home/charly/.config/nvim/lua/flutter-tools/dev_tools.lua:36: attempt to get length of local 'data' (a nil value)

So if you agree with me, I will ignore values that are not tables in the handle_error function and it should work. The problem: our job is exiting with an error

[akinsho]

I'm not entirely sure I follow tbh. So using Job:shutdown errors? even if the we haven't killed the process? is it possible to just check if the module variable pub_get_job exists and if so use shutdown i.e. we started the process so we end it cleanly with plenary's method if there is no job i.e. we inherited the process from flutter then use uv.kill?

Plenary shouldn't error just from using shutdown since that's how I stop the flutter run process which doesn't error. I'm just wary of creating a job through plenary then potentially working around plenary using uv to end that job especially since if we created it we should still have a reference to it

[Charly6596]

Using Job:shutdown doesn't kill our devtools server. I've checked the devtools pid getting it from the json data and it's different from the job.pid value so for some reason they are two different processes, that's why I'm killing it without plenary.

This is what I get printing the job pid (third parameter in the handle_start function) and the devtools pid took from the json data

(using ui.notify({j.pid, devtools_pid})
Job:shutdown errors because it passes nil to our methods, we could just check if the argument data is not nil

[Charly6596]

This is what I get using job:shutdown
First start the tools with the command:

And both processes are in the process tree (the parent of zsh is my terminal)

Executing lua require('flutter-tools.dev_tools').stop() inside neovim shows the Devtools stopped message, but the processes are still there (both of them actually, same tree)

After exiting the processes, instead of terminating, are detached:

It doesn't matter if I run the .stop command myself or trigger it exiting neovim, the output is the same

[akinsho]

Thanks for the explanation @Charly6596 that's strange that there are different processes in which case I'm fine with you using uv.kill and adding the necessary checks in the on_stderr 👍🏾

[Charly6596]

What would be the benefit of method 2 i.e. creating it and then passing it to the run command? not to say that there'd be no value I'm just not sure what it would be

@akinsho That's a very good question
I will show what I've found out:

• If running on mobile (Adroid), flutter:
  • Outputs the url to the debugger and profiler, just like this: An Observatory debugger and profiler on sdk gphone x86 arm is available at: http://127.0.0.1:46051/NvCev-HjyX4=/. That's the link we use in the dev tools server.
  • By default starts a devtools server and outputs the full link including the previous url. This is similar to the link we build to copy to the clipboard. The output is similar to Flutter DevTools, a Flutter debugger and profiler, on sdk gphone x86 arm is available at: http://127.0.0.1:9102?uri=http%3A%2F%2F127.0.0.1%3A46051%2FNvCev-HjyX4%3D%2F
• If running on web (chromium), flutter doesn't start the devtools server and only outputs the debug link (the one we need to introduce in the devtools web): Debug service listening on ws://127.0.0.1:44293/heXbxLM_lhM=/ws

So it 2nd might not be the best solution, I've tried to provide support for both. We could do the 3rd option if we detect the full url (hence flutter started the service and we are running mobile), otherwise we start the process.

[akinsho]

@Charly6596 I'd rather optimise for mobile since that is still by far the biggest segment of flutter users and make web best effort where it differs. So I guess from what you've said we can do 3 if it's mobile i.e. we have a full url so we just give dev tools the one flutter created and if we don't detect it then we start it? I think that should be configurable btw we shouldn't just start it without giving people a way out.

Does that work?

[Charly6596]

@akinsho yes, that sounds fine, I also agree it should be configurable
So we keep the command to start the devtools and pass our devtools if it's started? + what you said?

[Charly6596]

@akinsho Alright, I've done a part of the 3rd option.
Possible changes/questions/additions:

• Right now I log everything with floating text using ui.notify. Maybe would be nice to log some of the messages with dev_logger or utils.echomsg?
• Do we shutdown the devtools server when the user closes the app or keep it running?
• I've seen some methods have a _ _ prefix, what's the meaning of that?
• Quiet messages?
• Auto-starting devtools if not found and giving the option to disable the feature
• Auto-open the url in the browser, configurable

Just as a side note, my last commits could be squashed, I will keep them for now just in case

[akinsho]

@Charly6596 this LGTM thanks for your work on this, I've left one comment which I think is quite important to resolve around adding more conditions that could prevent running the app which I'd rather avoid

[akinsho]

Right now I log everything with floating text using ui.notify. Maybe would be nice to log some of the messages with dev_logger or utils.echomsg?
Do we shutdown the devtools server when the user closes the app or keep it running?
I've seen some methods have a _ _ prefix, what's the meaning of that?
Quiet messages?
Auto-starting devtools if not found and giving the option to disable the feature
Auto-open the url in the browser, configurable

1. I quite like the floating UI but as you say it means there is no history which is a pain, maybe the messages can be added silently to the message history. Maybe something like a silent echomsg 'thing' would work. Alternatively I've been wondering about whether this plugin should maintain it's own logs for debugging separate to the lsp logs, but all this is a separate issue in general.
2. I think the server should shutdown when the app shuts down, since not sure what the point of having it open without an app is, I also don't think it continues to work if the app is shutdown.
3. The __ prefix e.g. _G.__flutter_tools_select_outline_item() is because these methods are global and pollute a user's global namespace so I've added those to indicate privacy but also to try to reduce the likelihood of them clashing with a function a user declares. Global functions is one of the ways to workaround the lack of native support for lua functions in autocommands, there are others but will wait till native support gets merged and refactor all of those.
4. I think auto-starting would be cool if the user hasn't opted out but happy for that to be later 🤷🏾
5. This would be what I'd be most keen on since copy and pasting etc. is a pain/I'm lazy (but also happy for this to be in a separate PR)

[akinsho]

@Charly6596 lemme know if this ready to merge 👍🏾 think it was just the typo remaining

[Charly6596]

@akinsho done 👍
I'll open issues for 4th and 5th
I agree with all you said. I'm not sure what the message history is, but all I know is coc-flutter has a cool logger and is very helpful to debug issues, would we be able to retrieve the logs for only this plugin from the message history if using silent echomsg?

[akinsho]

@Charly6596 no we wouldn't using just message history you can find out more about it using :h messages but it's a dump of all the error/messages in the nvim session in a slightly unwieldy command line format, so I'd much rather look into creating a logger for this plugin. Actually TJ Devries has project which is a single copy paste logger that we can use and the work is already done would just need to integrate it and add some wrapper commands for it.

Anyway, will merge this for now and can iterate later