bloop jvm settings

[zmerr]

It resolves #2697
It is for getting bloop JVM settings from user config and then writing it in the global bloop json file.

[ckipp01]

Thanks for starting to dig into this @zmerr! Keep in mind that one other thing we'll need to think about is how to handle this when Bloop is already running. We'll probably want to write the new settings, then restart Bloop to ensure it's taken into account.

[zmerr]

@ckipp01 Now everything on the server side is implemented. Can you please take a second look?
The problem with the current implementation I think is that, if both the Bloop version and the Bloop JVM properties get modified, the user receives two dialogues asking them to restart. And if the Jvm properties one is not approved, then the bloop’s global json file would not be overwritten with the newly requested jvm memory settings.
So is there currently a standard way to blend in those dialogues in one, or should I do it my own preferred way?

[zmerr]

ah, one more thing: in the metals standup today, it was agreed upon that I put a checkbox on metals settings asking the user whether they want to manually enter the Jvm properties in the Metal extension’s settings, or they just want the contents of their bloop global json file to be used for that…
So that is yet to be implemented...

[tgodzik]

ah, one more thing: in the metals standup today, it was agreed upon that I put a checkbox on metals settings asking the user whether they want to manually enter the Jvm properties in the Metal extension’s settings, or they just want the contents of their bloop global json file to be used for that… So that is yet to be implemented...

I was thinking about something a bit different:

• if user doesn't have the global bloop json create then we just modify it and add an additional field to notify that it's Metals generated
• next time user modifies the settings we could check if that field exists, if it does just replace everything, otherwise offer message request with option to override or just opening the file (invoke goto lcoation).

[zmerr]

add an additional field to notify that it's Metals generated

@tgodzik then can’t it potentially cause some kind of error later in Bloop or another tool reading the file, due to an unrecognized key?
Is there a good way of adding it as some kind of comment or somewhere else that wouldn’t cause this?

[tgodzik]

add an additional field to notify that it's Metals generated

@tgodzik then can’t it potentially cause some kind of error later in Bloop or another tool reading the file, due to an unrecognized key? Is there a good way of adding it as some kind of comment or somewhere else that wouldn’t cause this?

Ach, you are right, I hoped it would just get ignored. We could add an additional file (kind of a lock file) like .bloop/metals.lock maybe?

[zmerr]

We could add an additional file (kind of a lock file) like .bloop/metals.lock maybe

@tgodzik Ok, then I would go with that approach…

I hope in the updates or reinstallations of Bloop, it won’t get erased…

Also, what if the user later modifies the json file after the metals.lock is created?

I would say, let’s just persist the time at which we are modifying the bloop json file by reading its "last modified time" value after we create or write to it in the metals server internal files, then in the next times, we compare the modification date to our persisted one, to see if it was us who modified it...

[tgodzik]

We could add an additional file (kind of a lock file) like .bloop/metals.lock maybe

@tgodzik Ok, then I would go with that approach…

I hope in the updates or reinstallations of Bloop, it won’t get erased…

Also, what if the user later modifies the json file after the metals.lock is created?

I would say, let’s just persist the time at which we are modifying the bloop json file by reading its "last modified time" value after we create or write to it in the metals server internal files, then in the next times, we compare the modification date to our persisted one, to see if it was us who modified it...

We could do that yeah, might be a good approach and otherwise ask the user about it.

[tgodzik]

Great work! The approach looks solid, I left some overall comments, but nothing major.

Did you manage to make it work with the VS Code extension?

[zmerr]

@ckipp01 do you think this should be done for other editors as well?

[ckipp01]

Great job on this so far @zmerr! I've left a few suggestions, questions, and nits.

@ckipp01 do you think this should be done for other editors as well?

So since this is using existing LSP methods already this should already work as is for other editors. For example in nvim-metals I just needed to add the newly created bloopJvmProperties as a possible configuration value and everything else just worked ™️

[tgodzik]

Great work so far! I left some more ideas, mostly because doing this PR right would help a lot of people.

[tgodzik]

Just tested it locally and everything looks great!

Could you rebase your PR on the newest changes? We can merge afterwards, thanks!

[tgodzik]

Let's merge this!