pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.32k stars 139 forks source link

`.js.source` in config file gets `preferredLineLength` and `wrap-guide` added to it, which don't match my default preferences. #697

Closed danfuzz closed 1 year ago

danfuzz commented 1 year ago

Thanks in advance for your bug report!

What happened?

My default preference is to have an 80-column preferred line length, but I like being able to have wrap guides at a couple wider columns for the few cases where long lines are warranted. I tried to set this up in Pulsar, but when I do that Pulsar generates a couple properties under .js.source which don't match what I said. And really, I don't want Pulsar to insert those properties at all; my defaults under * are in fact what I want for .js.source.

Pulsar version

1.108.0

Which OS does this happen on?

🍎 macOS

OS details

13.4

Which CPU architecture are you running this on?

Apple M1/M2

What steps are needed to reproduce this?

  1. Select Pulsar > Config....
  2. Add under *:
editor:
  preferredLineLength:80
"wrap-guide":
  columns: [
    80
    120
    160
  ]
  1. Save. Wait a moment.

Result: The existing .js.source section will be edited to have:

editor:
  preferredLineLength:80
"wrap-guide":
  columns: [
    80
  ]
  1. Think to yourself, "Well if it's gonna add wrap-guide, might as well make it what I want."
  2. Edit .js.source > "wrap-guide" so it reads:
"wrap-guide":
  columns: [
    80
    120
    160
  ]
  1. Wait a moment.

Result: The preferredLineLength under .js.source gets changed to 160.

  1. Think to yourself, "I guess I have to change that back."
  2. Edit .js.source > editor > preferredLineLength to be 80.

Result: The .js.source > "wrap-guide" gets rewritten back to just [ 80 ].

Additional Information:

No response

confused-Techie commented 1 year ago

As far as I know, and this applies to #696 as well, is Pulsar doesn't inject many values into a configuration file automatically.

Generally the only items that will be added is those that are not the default values, as in only when you change a setting should new values be added.

Is it possible there's another package activated causing this behavior? Could you maybe try listing your installed packages so we can see if one of them advertises this functionality? Or even better disable all packages, except core, remove these parts of your configuration then see if it occurs again, if it doesn't then it seems a community package is causing this behavior.

danfuzz commented 1 year ago

@confused-Techie I started Pulsar in safe mode and verified the weird behavior before filing this issue. I thought safe mode kept it from loading any community packages.

FWIW, the only community packages I have installed are minimap, minimap-selection, and tabs-to-spaces.

danfuzz commented 1 year ago

Here's a more detailed steps-to-repro, which I think satisfies your concerns (and also covers the behavior observed in #696):

  1. Remove ~/.pulsar directory.
  2. Run pulsar --safe in a shell.
  3. Run touch .pulsar/foo.js in a shell.
  4. Close "welcome" tabs.
  5. Select Settings....
  6. Select Open Config Folder. Select ...discard state (if that dialog box appears).
  7. Open config.cson and foo.js.

Here is the contents of the config.cson file:

"*":
  "exception-reporting":
    userId: "dbe65e8e-bf57-446d-9cff-9f769327b428"
  1. Add under *:
  editor:
    preferredLineLength: 80
  "wrap-guide":
    columns: [
      80
      120
      160
    ]
  1. Save. Wait a moment.

A .coffee.source section appears spontaneously with the following content:

".coffee.source":
  editor:
    preferredLineLength: 160
  "wrap-guide":
    columns: [
      80
      120
      160
    ]
  1. Add the following to config.cson:
".js.source":
  editor:
    tabLength: 2
  1. Save.
  2. Open foo.js.
  3. Switch back to the config.cson tab.
  4. Change 80 to 100 in the top wrap-guide list of columns.
  5. Save. Wait a moment.

The .js.source section changes to read:

".js.source":
  editor:
    preferredLineLength: 160
    tabLength: 2
  "wrap-guide":
    columns: [
      100
      120
      160
    ]
confused-Techie commented 1 year ago

@danfuzz Thanks a ton for providing such clear reproduceable steps here!

I was able to reproduce the behavior described and was able to get to the bottom of it.

This behavior stems from here where the wrap-guides package explicitly sets:

atom.set("wrap-guide.columns", columns, {scopeSelector: `.${this.editor.getGrammar().scopeName}`});

Basically setting the wrap-guide.columns value to the new column length under the scope of whatever is the currently opened language. This function is then attached as the callback to a config onDidChange() listener for editor.preferredLineLength.

That's why when you change the setting within Pulsar, if editing the file in Pulsar, you get a new .coffee.source setting, since the config file of CSON, has the scopeName of .coffee.source.

I was able to track this file (across decaf, and repo migration) to the original PR that introduced this saving new settings behavior here, specifically this commit.

Seems that this comment is what triggered this new behavior to be written.

Which it's intention seems to make sense, that if the preferredLineLength or columns are being changed, then those changes should propagate to the same setting scoped by languages, although I think the behavior of checking with whatever language is currently open does seem a little bit misguided, as well as really I'd hope we could actually check if this value is not the default for the scoped setting.

Since if the scoped settings are the default, we don't have to persist the changes to disk (I'd assume), since the scoped setting would then be used in the future.


Admittedly, I'm not the most familiar with the full implications of handling scoped settings, and if there's a better way to accomplish this.

@savetheclocktower Do you have a two cents to offer on this situation? Is there a better way to handle this situation? Is the current behavior necessary? Or is it something we shouldn't really worry about and just remove?

Curious if you have any thoughts, otherwise I'd be inclined to think, that if someone has scoped settings different than other settings, that's their choice, and we shouldn't force any changes to propagate other scopes at all, as that's the users prerogative, and it's their complexity to deal with. Meaning I'd be on board with removing this behavior. But would appreciate any other thoughts on this one

savetheclocktower commented 1 year ago

I can't dig into it until Monday, but if it can wait until then, I'm happy to offer advice.

confused-Techie commented 1 year ago

I can't dig into it until Monday, but if it can wait until then, I'm happy to offer advice.

That sounds fine by me, whenever, if you get the chance I'd appreciate it.

Otherwise like mentioned, my instinct says removing it is not a bad idea.

savetheclocktower commented 1 year ago

I've read the original PR and I'm quite surprised that this sort of magical behavior was mooted without even any discussion about making this configurable. The idea that editor.preferredLineLength should always be equivalent to the largest value of wrap-guide.guides is treated as obvious, even though it would rule out @danfuzz's specific use case. I'm surprised that they didn't envision it or didn't think that it was common enough to consider.

The quickest possible fix for this, and the one I'd be in favor of, is to put this behavior behind a wrap-guide config setting so that anyone can opt out of it if they like. I also wouldn't be opposed to removing this behavior entirely; it would mean a bit more work for some people, but I can't imagine that anyone would be annoyed that we didn't do that work automatically for them.

More generally, I'm also surprised to see an instance in the codebase where a scope-specific setting is automatically applied in a certain scenario. There may well be other examples of it (because the APIs exist, and any package could do it), but I'm pretty sure that all of my own scope-specific config overrides have been manually added to config.cson rather than automatically set by a package. If this sort of thing is more common than I realized, then it raises the salience of the fact that we don't have a way of representing those overrides in the settings GUI. We might eventually want to mark those scope-specific overrides in the GUI the way that we started marking project-specific overrides in #655.

confused-Techie commented 1 year ago

@savetheclocktower I really appreciate you taking a look at this. Especially to confirm that the behavior seems very strange to say the least.

But I'm totally on board with making this configurable, and just to keep expected support maybe we default to being enabled? Even if the idea seems a little outlandish. Otherwise I'm very happy to default to disabled from here on out. While leaving the future in, since you are right, sometimes it could make sense.

I'd be happy to put together a PR for this here, unless you beat me too it (which feel free to do so), but otherwise just wanted to check in with your thoughts since you're a bit more familiar in this space, so I'm glad you had the time

savetheclocktower commented 1 year ago

Defaulting to enabled is fine with me, and then if it bites other people in the future, we can decide if the default should be disabled instead.

confused-Techie commented 1 year ago

Defaulting to enabled is fine with me, and then if it bites other people in the future, we can decide if the default should be disabled instead.

Sounds good to me then, appreciate your input