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

LouisVallat commented 3 years ago

Possible fix for https://github.com/LycheeOrg/Lychee/issues/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

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

Aimed behavior

Open an album or a photo
Click on the "info" button or press the i key
Go back to a view where the "about" panel can't be shown
Open an album or a photo
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

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

LouisVallat commented 3 years ago

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 commented 3 years ago

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 :smile:).

LouisVallat commented 2 years ago

"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 :man_shrugging: 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Ok so:

1 vote in favor of a simple variable
1 vote in favor of using SessionStorage
1 vote in favor of using LocalStorage :laughing:

ildyria commented 2 years ago

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

kamil4 commented 2 years ago

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

LouisVallat commented 2 years ago

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 commented 2 years ago

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

nagmat84 commented 2 years ago

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 commented 2 years ago

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

LouisVallat commented 2 years ago

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.