Remember user preference for the visibility of the "about" panel

[LouisVallat]

Possible fix for LycheeOrg/Lychee#653

Main goal

This PR brings an enhancement to the front-end: remembering if the user wants to have the "about" panel open or not.

Current behavior

1. Open an album or a photo
2. Click on the "info" button or press the i key
3. Go back to a view where the "about" panel can't be shown
4. Open an album or a photo
5. The "about" panel is always closed, regardless of its last user-set state

Aimed behavior

1. Open an album or a photo
2. Click on the "info" button or press the i key
3. Go back to a view where the "about" panel can't be shown
4. Open an album or a photo
5. The "about" panel is closed or open, based on the last user choice

How it is achieved

In this PR, a solution is proposed.

• We should distinguish whether or not toggling the "about" panel visibility is user-generated or not
  • In order to do that, a parameter has been added to the sidebar.toggle method: is_user_initiated
• We should save the fact that the user toggled the "about" panel visibility
  • Whenever the toggling is user-generated, this visibility status is saved in the SessionStorage, so the status is Session-dependent, and is forgotten once the browser is closed. If needed it could be stored in the LocalStorage, to keep it persistent after browser restarts.
• We should open the "about" panel based on the user's last saved preference
  • When the "about" button is showed, it is checked whether or not the user had the panel previously open, and if yes, open it automatically if it's not already visible
• By default, the about panel is kept closed, no change compared to current way of doing it.

Things to discuss and TODOs

• SessionStorage or LocalStorage?
• Key name: is sidebarUserPreferenceVisible a good name or not? And if not, what to choose?

[kamil4]

I don't have a strong opinion about the name of the key.

I'm definitely against using LocalStorage for this so your choice of SessionStorage strikes me as more sensible. I have a genuine question here though: do we even need to use SessionStorage at all? Can't we simply store this info as a variable under sidebar? I guess my question reveals that I don't really understand what SessionStorage is meant to be used for...

Overall, the PR looks good to me.

[LouisVallat]

I have a genuine question here though: do we even need to use SessionStorage at all? Can't we simply store this info as a variable under sidebar?

Well, it is true that this could be a good fit for using a variable instead of the SessionStorage. It can be modified quite easily if needed.
My choice went for the SessionStorage because it persists even after refresh. Although it won't persist after a browser reboot or opening another tab with the same Lychee instance, as two tabs are considered as separate Sessions, according to mdn. It really depends on how long you want that state to be remembered/saved and used. SessionStorage persists after refresh, variable doesn't, but dies with the session, where LocalStorage doesn't, so it truly depends on how long you want that setting to be saved.

Would you prefer me to use a variable instead of the SessionStorage?

[kamil4]

OK, thank you for the explanation. I don't have a strong opinion on it then and I think that others should chime in.

AFAIK, we don't use SessionStorage anywhere else, which is why it made me wonder "what's so special about this case to warrant it?". We do use LocalStorage, for remembering the scroll positions of individual albums, and honestly that has always struck me as an odd choice (why should that be persistent?) and if it was up to me, I would covert that to SessionStorage (now that I know that there is this option 😄).

[LouisVallat]

"what's so special about this case to warrant it?"

Well in my opinion, it'd be pertinent to have this setting (the fact that the user wants to have the info sidebar open or not) being remembered even after refreshing but staying in the same tab (miss-clicks happen, but yeah it's just a button away so not a big deal). Really I don't have a strong opinion about the SessionStorage vs. variable situation, and it can be converted quite easily if needed.
We really need to answer the question: "should this setting be remembered across refreshes?" if yes, then let's use the SessionStorage, and if not then it can be a simple variable. Simple 🤷‍♂️
Also the key name (sidebarUserPreferenceVisible) kind of bothers me, seems too long, but I couldn't find any shorter name that'd still be as clear and as explicit..

[ildyria]

We really need to answer the question: "should this setting be remembered across refreshes?" if yes, then let's use the SessionStorage, and if not then it can be a simple variable. Simple man_shrugging

Personally, I would be in favor of a simple variable. :)

[nagmat84]

I would use the SessionStorage (or even the LocalStorage) for it as I believe that GUI layout and settings should be persisted. This is just about convenience.

I understand @kamil4 hesitations not to use SessionStorage, but for me it is a weak argument. Then let this one be the first usage. Maybe we will find other use cases for SessionStorage in the future, too.

[LouisVallat]

Ok so:

• 1 vote in favor of a simple variable
• 1 vote in favor of using SessionStorage
• 1 vote in favor of using LocalStorage
  😆

[ildyria]

I am also in favor of SessionStorage (assuming it get's wiped out once you restart your browser) :)

[kamil4]

Let's stick to SessionStorage then, especially since that's what the PR already does. Is it ready for merge?

[LouisVallat]

I am also in favor of SessionStorage (assuming it get's wiped out once you restart your browser) :)

As stated here:

• A page session lasts as long as the tab or the browser is open, and survives over page reloads and restores.
• Closing a tab/window ends the session and clears objects in sessionStorage.

There's one case where it isn't reset when closing the browser I believe, when restoring a previous session (i.e when you set your browser to start from where you left off).

Is it ready for merge?

I'll re-read my code and check for potential bugs, just in case. Also I still haven't got any answer concerning the long variable name. Is it a good name, or should we use another one? (sidebarUserPreferenceVisible)

[ildyria]

Personally I would just go for keepSidebarVisible, but that is just me. :)

[nagmat84]

Personally I would just go for keepSidebarVisible, but that is just me. :)

I like @ildyria proposal more. Rationale: It starts with a verb and so matches the naming scheme. Also it makes clear that the variable is a boolean.

If it all, then sidebarUserPreferenceVisible should be userPreferenceSidebarVisibibility (the last component must be a noun), but this name does not bear any intuitive type information.

[LouisVallat]

Okay, I totally agree with that, I'll change it

[LouisVallat]

Just changed the variable name, and the corresponding method in favor of keepSidebarVisible, this should be good to go I believe, if it's all good for you too.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[kamil4]

I fixed it so that it actually compiles after the latest changes and I rebased it on the current master. I also tested it. This should be good to go now.