fix: keep panes settings after reload

[luis-411]

This fixes the issue mentioned here: standardnotes/forum#2423

I used localStorage to save the changes made by toggeling the panes.
Also I edited the initialization of the panes array, because otherwise all three panes would show up after reload for a short time and then dissapear again depending on your settings.

[amanharwara]

Looks good for the most part. However, using local storage directly is problematic since there can be more than 1 account signed in and all of them might want to have different settings toggled.

Instead, the existing local preferences system (preferences.getLocalValue/preferences.setLocalValue) should be used.

[luis-411]

Ok thank you, I see.
So I tried to implement it that way, but I think I'm missing something here.
I added ListPaneCollapsed to LocalPrefKey.ts and used it like this in PaneController.ts:

Initialize:
listPaneExplicitelyCollapsed = this.preferences.getLocalValue(LocalPrefKey.ListPaneCollapsed, false)

Toggle:
this.preferences.setLocalValue(LocalPrefKey.ListPaneCollapsed, true)

But somehow it always takes the default value provided in getLocalValue when refreshing the page.

[amanharwara]

That's probably because the storage hasnt been decrypted yet. You'll want to handle the LocalPreferencesChanged event, you can look at how the ApplicationEvent.PreferencesChanged event is handled in the same file. Once you set it up that way, you can just call this.preferences.setLocalValue which will then automatically update the this.listPaneExplicitlyCollapsed properties of the class.

[amanharwara]

I think you've changed the incorrect file. .d.ts files aren't supposed to be modified directly. This whole file shouldn't be in git since it is generated from the original PrefDefaults.ts file.

[luis-411]

Thank you, I get that, but in order to access the values in PrefDefaults.ts, I would have to add them to PrefKeys.ts and so what is the point of using LocalPrefKeys.ts? Should I just use PrefKeys.ts

[luis-411]

Or should I just use it like this, without the default stuff, since it is only a boolean value:
listPaneExplicitelyCollapsed = this.preferences.getLocalValue(LocalPrefKey.ListPaneCollapsed, false)

[amanharwara]

Initially all the properties in LocalPrefKey were synced so they were in the PrefKey and so they still use PrefDefaults.

You can just create a LocalPrefDefaults similar to the PrefDefaults in the LocalPrefKey.ts file

[amanharwara]

We keep all the defaults in a constant so that there's a single source of truth in case those might need to be used somewhere else.

[luis-411]

So do you mean in LocalPrefKey.ts add this:

export declare const LocalPrefDefaults: {
  listPaneCollapsed: false;
  navigationPaneCollapsed: false;
}

and use it like this:
listPaneExplicitelyCollapsed = this.preferences.getLocalValue(LocalPrefKey.ListPaneCollapsed, LocalPrefDefaults[LocalPrefKey.ListPaneCollapsed])

[amanharwara]

yes

[amanharwara]

I tried this out locally and the setting does seem to get correctly persisted, but the actual pane removal logic doesn't seem to work on reload? If I turn off the items list, it doesn't stay turned off on reload but from logging I can see that the value has been stored correctly.

[luis-411]

Ok, I could reproduce it: I normally use Arc Browser (which is chromium based) and it works as expected. However if I try it in Chrome or Edge, I get the same behaviour as you described above.

[luis-411]

I did a lot of testing and found out some inconsistencies:

First of all this is not browser related because I noticed the same behaviour for chrome and edge.
The difference is rather running the project on a "fresh" browser (e.g. using incognito tab) or on a browser where you used it before.

First weird behaviour:
After skipping the account creation, the GeneralAccountMenu pops up in the lower left corner. This window keeps appearing after every refresh even if I close it via the X or click somwhere else. Only writing something in a note or clicking on a button like on the Choose a note type button will permanently close the window and therefore turn a "fresh" browser into an "old" one.

Second weird behaviour:
The method setPaneLayout in PaneController.ts only gets called on "old" browsers but not on "fresh" ones.
This is problematic, because it turns out the Preference behaviour only gets applied in this method but not in the beginning of PaneController.ts where the default value of getLocalValue is always used.

So i guess if you type somthing into a note, you should be able to see the intended behaviour of my implementation. I am currently looking into why setLocalValue always takes the default value and what changes after writing something in a note or pressing another button. If you have any idea please let me know and thanks for the help so far. :)

[luis-411]

After some more testing, I don't know how to resolve the problem with the account menu popup, but I found, that for example in line 96 of PaneController.ts the getValue function also returns the default value:
this.setCurrentNavPanelWidth(preferences.getValue(PrefKey.TagsPanelWidth, MinimumNavPanelWidth))
The correct value is set later on in the handleEvent function.

So this is not a problem of LocalPrefKeys but rather something general and from my experience it should work just fine on "normal" accounts.
@amanharwara it would be really interesting to know if you can reproduce this behaviour and how you think this should be handled.

[amanharwara]

The theme-related settings which also use LocalPrefKeys work correctly on empty workspaces, so this not working there is a bug.

I think what you'll probably want to do is instead of running this initialization logic in the constructor, you'll want to run it the first time ApplicationEvent.LocalPreferencesChanged event is run.

[amanharwara]

First weird behaviour:
After skipping the account creation, the GeneralAccountMenu pops up in the lower left corner. This window keeps appearing after every refresh even if I close it via the X or click somwhere else. Only writing something in a note or clicking on a button like on the Choose a note type button will permanently close the window and therefore turn a "fresh" browser into an "old" one.

Btw this is not a bug, it's intended behavior. If the workspace is fresh, we always show the account menu on load so that the user can quickly create an account or sign in.

[luis-411]

The theme-related settings which also use LocalPrefKeys work correctly on empty workspaces, so this not working there is a bug.

I think what you'll probably want to do is instead of running this initialization logic in the constructor, you'll want to run it the first time ApplicationEvent.LocalPreferencesChanged event is run.

This seems to work, thank you. I will change it and push it in a minute.

[amanharwara]

this will now run after every setLocalValue call, no? i think it makes more sense to have a boolean in the class to make sure this only runs once since its supposed to be initialization logic.

[amanharwara]

i was talking about the whole initialization logic, not the particular line 🙂 a boolean like hasPaneInitializationLogicRun = false in the class making sure the initialization logic only runs once.

[luis-411]

Ah ok sorry, that makes sense.
So like this?:

if(!this.hasPaneInitializationLogicRun) {
        if (this.isTabletOrMobile) {
          this.panes = [AppPaneId.Navigation, AppPaneId.Items]
        } else {
          if (!this.listPaneExplicitelyCollapsed && !this.navigationPaneExplicitelyCollapsed) {
            this.panes = [AppPaneId.Navigation, AppPaneId.Items, AppPaneId.Editor]
          } else if (this.listPaneExplicitelyCollapsed && this.navigationPaneExplicitelyCollapsed) {
            this.panes = [AppPaneId.Editor]
          } else if (this.listPaneExplicitelyCollapsed) {
            this.panes = [AppPaneId.Navigation, AppPaneId.Editor]
          } else {
            this.panes = [AppPaneId.Items, AppPaneId.Editor]
          }
        }
        this.hasPaneInitializationLogicRun = true
      }

should i put the const screen ... back in then or leave this extra?

[amanharwara]

So like this?

yes

should i put the const screen ... back in then or leave this extra?

yes. technically the value of isTabletOrMobile should be latest, even though in this particular case the value probably wouldn't change between the construction of the class and the event. however it makes sense to always be correct.

[amanharwara]

Looks good to go now, thanks for the PR!