search

TheSpyder / SyncedSideBar

Sublime Text plugin to sync project sidebar (folder view) with currently active file.
341 stars 23 forks source link

Added missing settings entry on Package Settings. #41

Closed evandrocoan closed 8 years ago

TheSpyder commented 8 years ago

I think you were right with what you had in #40, it should have a package user settings file rather than pointing user settings to the global preferences file. The problem is likely that user settings aren't being searched (although I'm using the Sublime API that says it should be).

I'll look into it.

evandrocoan commented 8 years ago

It could be because you are writing them to the User/Preferences file: https://github.com/TheSpyder/SyncedSideBar/blob/master/SyncedSideBar.py#L132-L137

And the User/Preferences always override any plugin settings file.

evandrocoan commented 8 years ago

I fixed all the problems. Everything should be working fine now.

TheSpyder commented 8 years ago

ah, I see what you mean now. The sublime default inherited settings (project -> user -> system) are all based on the User/Preferences file.

PR mostly looks good but for reveal-all-tabs it still needs to read view.settings() otherwise we won't be able to override the configuration with a .sublime-project file. I think it's important to keep that.

evandrocoan commented 8 years ago

I think it is it, I added some new lines for catching user settings from the preferences file.

TheSpyder commented 8 years ago

Sorry to nitpick, I didn't explain what I wanted well enough :)

view.settings().get('reveal-all-tabs') will read from Preferences.sublime-settings for us; Sublime automatically searches the entire hierarchy of settings files. In particular, it does project settings which has been lost in your change. https://www.sublimetext.com/docs/3/settings.html

This is why userSettings was not cached at all previously. Please roll that back and check view.settings().get() plus packageSettings.get() in the reveal_all function.

Also, there's a bug in your preference save code - if reveal-on-activate is none in both user and package, it will not actually persist the change.

evandrocoan commented 8 years ago

view.settings().get('reveal-all-tabs') will read from Preferences.sublime-settings for us; Sublime automatically searches the entire hierarchy of settings files. In particular, it does project settings which has been lost in your change.

You are right, I lost this, but I need to manually load the own package settings file, otherwise view.settings() do not access them:

reloading plugin SyncedSideBar.SyncedSideBar
view.settings().get(reveal-all-tabs): None

On the above should return 'False', as I have it set to false on my 'SyncedSideBar.sublime-settings'. Even loading the file 'SyncedSideBar.sublime-settings' at plugin_loaded() does not helps.

Also, there's a bug in your preference save code - if reveal-on-activate is none in both user and package, it will not actually persist the change.

Thanks, I think I fixed it.

TheSpyder commented 8 years ago

hmm. I'm testing it out before I merge it, and I'm trying to decide what the order of preferences should be.

With your changes if the project settings make reveal-all-tabs true but the user settings have false it treats it as false. Does that make sense? Or should project settings be able to override user settings? 🤔

TheSpyder commented 8 years ago

Looking at the sublime docs I linked earlier, user prefs override project settings. So let's go with your code being correct there. However if we accept that for reveal-all-tabs, then the reveal-on-activate code is wrong.

Please swap the order of both reading and writing; in plugin_loaded on read packageSettings needs to go last so it takes priority, and in SideBarUpdateSync on write it needs to go first so it can override correctly on the next load.

Long term I will probably switch both reveal-on-activate and reveal-all-tabs to globals, or have some sort of combined global, to make them more consistent and easier to follow. I'll work on that after your PR is merged though.

Thanks for all of your efforts on this :)

evandrocoan commented 8 years ago

Hi, and sorry, I wrote too much.

Please swap the order of both reading and writing; in plugin_loaded on read packageSettings needs to go last so it takes priority, and in SideBarUpdateSync on write it needs to go first so it can override correctly on the next load.

Settings files are consulted in this order:

  1. Packages/Default/Preferences.sublime-settings
  2. Packages/Default/Preferences (<platform>).sublime-settings
  3. Packages/User/Preferences.sublime-settings <--we are around here, the 3º less priority level
  4. <Project Settings>
  5. Packages/<syntax>/<syntax>.sublime-settings
  6. Packages/User/<syntax>.sublime-settings
  7. <Buffer Specific Settings>

On this 3º less priority level we are creating a new role, the Package Own Settings File. Therefore at this level we need to choose, whether this new Package Own Settings File is more priority, or less priority than the Packages/User/Preferences.sublime-settings. On my opinion, the packages settings file is less priority than the user's settings file.

Hypothetical Angles

What I wrote is like this:

  1. Packages/Default/Preferences.sublime-settings
  2. Packages/Default/Preferences (<platform>).sublime-settings
  3. Package Own Settings File <-- This takes less priority, as it is overridden by the user settings, because it is read, before it.
  4. Packages/User/Preferences.sublime-settings <--we are around here, the 3º less priority level
  5. <Project Settings>
  6. Packages/<syntax>/<syntax>.sublime-settings
  7. Packages/User/<syntax>.sublime-settings
  8. <Buffer Specific Settings>

What you just told me to change is:

  1. Packages/Default/Preferences.sublime-settings
  2. Packages/Default/Preferences (<platform>).sublime-settings
  3. Packages/User/Preferences.sublime-settings <--we are around here, the 3º less priority level
  4. Package Own Settings File <-- This takes more priority, as it is overriding the user settings, because it is read, after it.
  5. <Project Settings>
  6. Packages/<syntax>/<syntax>.sublime-settings
  7. Packages/User/<syntax>.sublime-settings
  8. <Buffer Specific Settings>

But if I am correct, this review above is wrong, because we cannot hack the Sublime Text settings reading order, this way what is actually happening is this:

Current Angles

What I wrote is like this:

  1. Package Own Settings File <-- This takes less priority, as it is overridden by the user settings, because it is read, before it.
  2. Packages/Default/Preferences.sublime-settings
  3. Packages/Default/Preferences (<platform>).sublime-settings
  4. Packages/User/Preferences.sublime-settings <--we are around here, the 3º less priority level
  5. <Project Settings>
  6. Packages/<syntax>/<syntax>.sublime-settings
  7. Packages/User/<syntax>.sublime-settings
  8. <Buffer Specific Settings>

What you just told me to change is:

  1. Packages/Default/Preferences.sublime-settings
  2. Packages/Default/Preferences (<platform>).sublime-settings
  3. Packages/User/Preferences.sublime-settings <--we are around here, the 3º less priority level
  4. <Project Settings>
  5. Packages/<syntax>/<syntax>.sublime-settings
  6. Packages/User/<syntax>.sublime-settings
  7. <Buffer Specific Settings>
  8. Package Own Settings File <-- This takes more priority, as it is overriding the user settings, because it is read, after it.

The point here is, the way it is coded, the package settings will be used if the user settings file does not specify them, and if so, the SideBarUpdateSync(1) will correctly write them to the user settings files or the the project settings files. But if by default, as the first time the user run the program, it will write then to the packages files, and as soon as the user writes it, at his settings file, the program will correctly read and write the settings from/to the user's settings file.

Conclusion

On my opinion, I would really hate to place settings on the Packages/User/Preferences.sublime-settings file, and they be overridden by some other package file. So, this is why I wrote them like this:

On plugin_loaded(0), first come the package, then comes the user, this way the package file is overriden.

    # read initial setting
    read_pref_package()
    read_pref_user()

On SideBarUpdateSync(1), first I check whether the user has written the setting on this settings file Packages/User/Preferences.sublime-settings, if not then I write the preferences on the package file. This way, when the function plugin_loaded(0) is called, we check whether the there is a setting on the package file, and former on the user file.

        userSettings = sublime.load_settings('Preferences.sublime-settings')
        vis          = userSettings.get('reveal-on-activate')
        if vis is not None:
            userSettings.set('reveal-on-activate', enable)
            sublime.save_settings('Preferences.sublime-settings')
        else:
            packageSettings = sublime.load_settings('SyncedSideBar.sublime-settings')
            packageSettings.set('reveal-on-activate', enable)
            sublime.save_settings('SyncedSideBar.sublime-settings')

Also, I do not see how to easily invert the order here, because if we do so, we would always write on the Packages/User/Preferences.sublime-settings, despite the package file setting exists or not.

To fix it, I would first to load the package settings, and if it does not contains settings, then I write then on the user settings file.

I am not changing this for now because you said something which is do not looks true:

Looking at the sublime docs I linked earlier, user prefs override project settings.

It looks the opposite, the project settings override the user settings. Why? Because the Settings files are consulted in this order:

  1. Packages/Default/Preferences.sublime-settings
  2. Packages/Default/Preferences (<platform>).sublime-settings
  3. Packages/User/Preferences.sublime-settings
  4. <Project Settings>
  5. Packages/<syntax>/<syntax>.sublime-settings
  6. Packages/User/<syntax>.sublime-settings
  7. <Buffer Specific Settings>

So, as we may see, the Project Settings are read after the Packages/User/Preferences.sublime-settings, this way the user settings are overridden by the package settings, if and only they conflict with each other, i.e., the Project Settings defined A false and the Packages/User/Preferences.sublime-settings defined A and B as true, then, the user's package value true will be overridden by the project settings false, but the user's value B true will hold true.

TheSpyder commented 8 years ago

I don't mind detailed explanations! Right, I did get that "user prefs override" backwards. Thanks :)

The only reason to invert save is if we invert read, so no need to worry about that. To clarify, though, if we inverted save we wouldn't always write to the user preferences; we would only do it if the package prefs aren't set. It's the exact opposite of now where we write to the package prefs if the user prefs aren't set (which I'm not totally happy with but it's about the best we can do with the current Sublime APIs).

OK, so if we're going with user overrides package there's a bug in reveal-all-tabs. The way the logic is structured reveal-all-tabs is allowing a package false to override a user true.

evandrocoan commented 8 years ago

OK, so if we're going with user overrides package there's a bug in reveal-all-tabs.

Thanks for notice, I wrote so many bugs. I think I fixed it.

TheSpyder commented 8 years ago

There is complexity in the behaviour, which makes it easy to have accidental bugs. I hope to clean that up soon.

For now, I thought I had found bug but it's happening in the release as well so I think this PR is good to merge!