Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide app.storage.client as a location to store volatile data which only matters for the current connection #2820

Merged
merged 29 commits into from
Apr 9, 2024

Conversation

Alyxion
Copy link
Contributor

@Alyxion Alyxion commented Apr 4, 2024

As discussed in pull request #2811 and the discussion Add app.storage.tab or similar #1308 I added a feature which allows the storage of data per Client connection.

This - in practice - and when either implementing an own single page application approach, e.g. based upon the example/single_page_app or when #2811 is merged enables developers to create per-browser tab user data instances, for example with different logins and/or settings with the same comfort of as currently when using app.storage.user - so without handing over another per-page instance from event handler to handler.

…in the current Client instance - which in practice means "per browser tab".
@Alyxion Alyxion changed the title Per browser user data app.storage.session Per browser tab user data app.storage.session Apr 4, 2024
@Alyxion Alyxion marked this pull request as ready for review April 4, 2024 11:23
nicegui/client.py Outdated Show resolved Hide resolved
nicegui/client.py Outdated Show resolved Hide resolved
nicegui/client.py Outdated Show resolved Hide resolved
nicegui/storage.py Outdated Show resolved Hide resolved
Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you replace the double quotes with single quotes? See https://github.com/zauberzeug/nicegui/blob/main/CONTRIBUTING.md#single-vs-double-quotes

nicegui/storage.py Outdated Show resolved Hide resolved
Added app.storage.client clear test
@rodja rodja changed the title Per browser tab user data app.storage.session Per browser tab user data app.storage.client Apr 6, 2024
@rodja rodja changed the title Per browser tab user data app.storage.client Provide app.storage.client as a location to store volatile data which only matters for the current connection Apr 6, 2024
@rodja rodja mentioned this pull request Apr 6, 2024
3 tasks
@rodja
Copy link
Member

rodja commented Apr 6, 2024

I just started a first proof-of-conecept for app.storage.tab in #2837.

@Alyxion
Copy link
Contributor Author

Alyxion commented Apr 6, 2024

Could you replace the double quotes with single quotes? See https://github.com/zauberzeug/nicegui/blob/main/CONTRIBUTING.md#single-vs-double-quotes

Done. Have read the CONTRIBUTING.md of course, after so many years of C++ and double quotes for everything except single character it though "hurts" as much as I would have to use a knife in my left and fork in my right hand ;-)... fixed the other branch/PR as well.

@Alyxion Alyxion closed this Apr 6, 2024
@Alyxion Alyxion reopened this Apr 6, 2024
nicegui/storage.py Outdated Show resolved Hide resolved
Alyxion and others added 3 commits April 7, 2024 21:32
Dropped implementation client.current_client for now
Added exceptions for calls to app.storage.client from auto index or w/o client connection.
Added tests for changes
@Alyxion
Copy link
Contributor Author

Alyxion commented Apr 7, 2024

Added the missing checks + unit tests, removed Client.current_client and added the sections to the documentation page based upon your examples in the .tab variant.

tests/test_client_state.py Outdated Show resolved Hide resolved
nicegui/storage.py Outdated Show resolved Hide resolved
@Alyxion
Copy link
Contributor Author

Alyxion commented Apr 8, 2024

Updated. I already referred #2837 in the documentation to explain when to use tab and when to use client. The count in It features four built-in storage types: needs to be adjusted accordingly when both PRs should be united in the main branch.

@Alyxion
Copy link
Contributor Author

Alyxion commented Apr 8, 2024

Resolved the conflicts with the new main branch and adjusted the doc accordingly.

Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just made minimal changes to the documentation. In markdown we like start each sentence in a new line.
@falkoschindler it would be great if you can have a look, too.

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just reviewed the code and made a few minor changes. Most prominently I moved the tests into test_storage.py. Even though this file is already pretty long, we tend to move tests for file x.py into a file test_x.py. So all storage tests are in one place.

I've got three questions remaining:

    • Is client.state a good name for this kind of storage? What do you think about client.storage? I'm undecided though. Since it is very volatile, it's somehow less than real storage. But it is implemented and documented as "storage".
    • Why do we need to call get_slot_stack() in clear()? I guess it's for some important but slightly unintuitive reason.
    • Why are there two buttons and two labels in test_client_storage?

@falkoschindler falkoschindler added the enhancement New feature or request label Apr 8, 2024
@falkoschindler falkoschindler added this to the 1.4.21 milestone Apr 8, 2024
@Alyxion
Copy link
Contributor Author

Alyxion commented Apr 8, 2024

Thanks @falkoschindler, regarding your questions:

  1. Is client.state a good name for this kind of storage? What do you think about client.storage? I'm undecided though. Since it is very volatile, it's somehow less than real storage. But it is implemented and documented as "storage".

The reason why I chose state was actually 1:1 taken from Streamlit st.session_state which has exactly the same behaviour - "keep the state" as long as the user is connected. I think it's a bit philosophical what one interpretes into the words "state" and "storage", after all its also called Solid State Drive and not Solid Storage Drive, right? ;-)

  1. Why do we need to call get_slot_stack() in clear()? I guess it's for some important but slightly unintuitive reason.

Otherwise app.reset crashes during the tests because it is also called w/o an available slot and then context.get_client would crash.

  1. Why are there two buttons and two labels in test_client_storage?

Fixed, I assume through a git merge which duplicated those lines.

@falkoschindler
Copy link
Contributor

  1. Why do we need to call get_slot_stack() in clear()? I guess it's for some important but slightly unintuitive reason.

Otherwise app.reset crashes during the tests because it is also called w/o an available slot and then context.get_client would crash.

Ok, I've rewritten this part a little:

  • I explicitly get the client and catch any runtime errors.
  • I don't check for auto-index clients, because we wan't to clear all storage.
  • We don't need getting the slot stack anymore, which was a bit confusing in my opinion.

@falkoschindler
Copy link
Contributor

Code for testing all our storages:

@ui.page('/')
async def index():
    if 'time' not in app.storage.browser:
        app.storage.browser['time'] = time.time()
    ui.label(f'{app.storage.browser["time"]=}')

    if 'time' not in app.storage.client:
        app.storage.client['time'] = time.time()
    ui.label(f'{app.storage.client["time"]=}')

    if 'time' not in app.storage.user:
        app.storage.user['time'] = time.time()
    ui.label(f'{app.storage.user["time"]=}')

    if 'time' not in app.storage.general:
        app.storage.general['time'] = time.time()
    ui.label(f'{app.storage.general["time"]=}')

    await context.get_client().connected()

    if 'time' not in app.storage.tab:
        app.storage.tab['time'] = time.time()
    ui.label(f'{app.storage.tab["time"]=}')

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to merge now. Thanks everyone! 🙂

@falkoschindler falkoschindler merged commit 1428e6d into zauberzeug:main Apr 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants