feat: close markmap when watched buffer is closed #16

MarcoBuess · 2024-03-30T00:27:51Z

This was a late one for me. Let me know how you like it, or if you can spot major flaws right away. cheers and happy easter. :)

Zeioth · 2024-03-31T02:01:27Z

I've been testing the solution and I like it but there is a problem I'm not sure it has solution:

Every time the user run :MarkmapWatch, it spawns a markmap process.
Those processes can accumulate infinitely until the user decides to close the buffer.

Every markmap process take 0.8% RAM on my machine, about 150Mb.

Advantages

Because this approach allow multiple processes to spawn, it makes possible to watch multiple files without having to run :MarkmapWatch every time you change to a different buffer, which is actually a better UX.

Disadvantages

A user might potentially spawn a unnecessary amount of processes that could easily take from 500Mb to few Gb in the worst case.

Failed solutions

Only allow one process per buffer → Not a viable solution. This would make impossible for the user to run :MarkmapWatch again if he closes the html page accidentally, as we don't currently implement a complex solution like RPC.

Potencially viable solutions

Updating the readme to describe precisely the new behavior.
A notification to inform the user the process will remain until he closes the buffer or run :MarkmapWatchStop (which also need changes).

Give me a bit more of time to think.

MarcoBuess · 2024-03-31T07:26:40Z

Only allow one process per buffer → Not a viable solution. This would make impossible for the user to run :MarkmapWatch again if he closes the html page accidentally, as we don't currently implement a complex solution like RPC.

You could relatively easily store the jobs by buffer in a table and make a simple lookup before spawning a new job. This would allow for watching of multiple buffers, but prevents launching a second one. You would have to rework WatchStop to stop on buffer basis and/or have lt gobally stop all jobs. I'm kinda suprised that this even works. During my tests (on windows) I was not able to launch a second mm instance, as it would always try and spawn on the same default port. I might have to take a closer look.

MarcoBuess · 2024-04-02T12:12:43Z

@Zeioth I added my suggestions in a couple (hopefully digestible) commits. Watching multiple buffers should now handle appropreatly. On windows, you will receive an error when you try to start multiple instances. Generally you will now receive an error if markmap cli reports to stderr, print the respective message and deregister the autocmd as well as take the affected buffer from the watch list.

MarkmapWatchStop now only handles the currently open buffer instance. We could either introduce a MarkmapWatchStopAll command or parameterize MarkmapWatchStop in case that kind of functionality is needed.

Let me know what you think.

Zeioth · 2024-05-09T17:33:28Z

@MarcoBuess It doesn't seem to be working correctly currently:

Manual test

Open two .md files
Run :MarkmapWatch on one of them.
Close the buffer.
Run :MarkmapWatch on the other file.
Markmap seems to still be running.

Manual test 2

If we run :MarkmapWatch in the second file before closing the first file, it will cause the former error. This error is persistent in further attempts, meaning something is not ok.
But If I close the file first, and then I try to run MarkmapWatch on the second one, it will work as expected.

More info

It doesn't seem to be killing the existing markmap process correctly, as the process seem to still be running on background
Unless I'm wrong, there is no point in tracking grace_period per file because MarkmapWatch only allow tracking one file per port anyway.
The PR change lines that are not related with the PR.

MarcoBuess · 2024-05-16T19:41:54Z

@Zeioth Please try and update your node.js version. I had this issue happen with v18 on Windows and thought it was Windows specific. The Problem was gone after updating to v20+. v18 would not spawn another instance on a different port but rather tries to reuse port 3000 which will be blocked by the running node instance.

Zeioth · 2024-06-09T20:12:52Z

still repro on nodejs 20 and also nodejs 22

Zeioth · 2024-06-09T20:19:21Z

But that error come from markmap itself.

I'm gonna report it and see what happens

Zeioth · 2024-06-09T20:26:19Z

Waiting for feedback: markmap/markmap#250

This was a late one for me. Let me know how you like it, or if you can spot major flaws right away. cheers and happy easter. :)

removed `remove_trailing_seperators` option, which would prevent stylua from formatting

To handle that windows can't have multiple instances of markmapcli running I added a passthrough, so that I can handle appropreatly when markmapcli errors. I deregister autocmds and take the buffer from the watch list.

Looks like I've broken open and save after adding opts. This should adress this. For some reason vim.fn.jobstart() can't handle an empty table as opts. Maybe someone has a better solution that this, but this works for now.

MarcoBuess mentioned this pull request Mar 30, 2024

[suggestion] improved watch stop #15

Closed

MarcoBuess added 7 commits June 11, 2024 16:54

feat: close markmap when watched buffer is closed

2ece916

This was a late one for me. Let me know how you like it, or if you can spot major flaws right away. cheers and happy easter. :)

fix(stylua): removed unknown configuration option

a4b80af

removed `remove_trailing_seperators` option, which would prevent stylua from formatting

style(config): stylua changed quotes

478053d

feat(utils): extended jobstart with opts

01a0cbb

To handle that windows can't have multiple instances of markmapcli running I added a passthrough, so that I can handle appropreatly when markmapcli errors. I deregister autocmds and take the buffer from the watch list.

feat(init): added tracking of watched buffers

4013ab3

chore(init): removed unused variable uv

d7af548

fix(utils): Vim:E475 after adding opts to jobstart wrapper

1ca57ed

Looks like I've broken open and save after adding opts. This should adress this. For some reason vim.fn.jobstart() can't handle an empty table as opts. Maybe someone has a better solution that this, but this works for now.

MarcoBuess force-pushed the main branch from d409975 to 1ca57ed Compare June 11, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: close markmap when watched buffer is closed #16

feat: close markmap when watched buffer is closed #16

MarcoBuess commented Mar 30, 2024

Zeioth commented Mar 31, 2024 •

edited

Loading

MarcoBuess commented Mar 31, 2024 •

edited

Loading

MarcoBuess commented Apr 2, 2024 •

edited

Loading

Zeioth commented May 9, 2024 •

edited

Loading

MarcoBuess commented May 16, 2024 •

edited

Loading

Zeioth commented Jun 9, 2024

Zeioth commented Jun 9, 2024

Zeioth commented Jun 9, 2024

feat: close markmap when watched buffer is closed #16

Are you sure you want to change the base?

feat: close markmap when watched buffer is closed #16

Conversation

MarcoBuess commented Mar 30, 2024

Zeioth commented Mar 31, 2024 • edited Loading

Advantages

Disadvantages

Failed solutions

Potencially viable solutions

MarcoBuess commented Mar 31, 2024 • edited Loading

MarcoBuess commented Apr 2, 2024 • edited Loading

Zeioth commented May 9, 2024 • edited Loading

Manual test

Manual test 2

More info

MarcoBuess commented May 16, 2024 • edited Loading

Zeioth commented Jun 9, 2024

Zeioth commented Jun 9, 2024

Zeioth commented Jun 9, 2024

Zeioth commented Mar 31, 2024 •

edited

Loading

MarcoBuess commented Mar 31, 2024 •

edited

Loading

MarcoBuess commented Apr 2, 2024 •

edited

Loading

Zeioth commented May 9, 2024 •

edited

Loading

MarcoBuess commented May 16, 2024 •

edited

Loading