search

BetterThanTomorrow / calva

Clojure & ClojureScript Interactive Programming for VS Code
https://marketplace.visualstudio.com/items?itemName=betterthantomorrow.calva
Other
1.67k stars 217 forks source link

autoSelectForJackIn in user settings takes precedence over workspace settings #2606

Closed PEZ closed 1 month ago

PEZ commented 2 months ago

When configuring both a user level and a workspace level repl connect sequence, where both have autoSelectForJackIn set, the user level sequence will be used for jack in. This goes against the general expectations that workspace settings override user settings.

Calva can only use one sequence like this, of course. And in https://calva.io/connect-sequences/ we can read:

If you have more than one sequence with autoSelectForJackIn set to true, the first one will be used.

Calva joins the connect sequences from the settings in the following order:

  1. User
  2. Workspace
  3. Workspace Folder

I'm unsure about the implications of changing this order, other than that it would fix the issue with User autoSelectForJackIn, as well as change the order the sequences are listed in the sequence menu. We could also change things so that Calva chooses the last sequence with the autoSelectForJackIn config. None of the options are super nice, but keeping the non-useful behaviour is worse, imo.

seancorfield commented 2 months ago

I knew about the "more than one" ... [use] ... "the first one", but I assumed that meant per "tier", not in the overall merging.

As you say, intuitively, folks expect the workspace settings to override the user settings.

Could you change the merge for this so the tiers prepend the replConnectSequences and then take the first one at the end of the merge?

Are there other settings that rely on merging vectors so that user is first, then workspace, then folder?

PEZ commented 2 months ago

Could you change the merge for this so the tiers prepend the replConnectSequences and then take the first one at the end of the merge?

Not sure I am following. There is not merge currently. The connect sequences are just concatenated. When jacking in this list is searched for sequences with autoSelectForJackIn and the first one found will be used. Same for connect and autoSelectForConnect. Your suggestion/question may be possible, I just need to first understand it. 😄

PEZ commented 2 months ago

Are there other settings that rely on merging vectors so that user is first, then workspace, then folder?

I don't think so. But the jackin and connect commands take connectSequence argument which can be a string and if so is treated as the name. We then:

    connectSequence = getConnectSequences(projectTypes.getAllProjectTypes()).find(
      (s) => s.name === options.connectSequence
    );

JavaScript find() finds the first match: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

Now some brave user may have configured connect sequences with the same name and also keyboard shortcuts or Joyride commands relying on finding the user one. But I'd say that must be unlikely! I have forgotten to document that the first one is used in these cases, and also that we join the sequence tiers the way we do, so at least we haven't promised this behaviour.

The reason we join the sequences in this order is that I thought it was a good idea to always see the same sequence at the top of the menu in the UI. But that doesn't seem super important. It is probably more helpful that any workspace sequences are presented first.

seancorfield commented 2 months ago

By "merge" I just meant however options are constructed from the combination of user + workspace + folder -- how those options are "merged".

seancorfield commented 1 month ago

Any more discussion needed on this one @PEZ or are you happy to move forward and make the change?

PEZ commented 1 month ago

Yeah, if we're fine with just concatenating the sequences in the reverse tier order, which I think I am. I can't really see how it would be all that disruptive to anyone.