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

feat(view): Allow reactive files view content #918

Closed
wants to merge 1 commit into from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Apr 4, 2024

Allow views to accept a callback to call if the current content has changed (e.g. user settings etc).

For example if a view changed while the user is still on the same view, the files app content currently will not update.
This allows reactive content (another way would be to allow Vue reactive return values but this brings other problems as soon as files app and app registering the view have different Vue versions).

So a simple vanilla JS solution is to provide a callback to call when we need to inform the files app that the current view has changed and it needs to refetch the content.

Allow views to accept a callback to call if the current content has changed (e.g. user settings etc).

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added enhancement New feature or request 3. to review labels Apr 4, 2024
@susnux susnux requested a review from skjnldsv April 4, 2024 11:19
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.50%. Comparing base (01981d1) to head (ac40f9c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #918   +/-   ##
=======================================
  Coverage   77.50%   77.50%           
=======================================
  Files          17       17           
  Lines         440      440           
  Branches      118      118           
=======================================
  Hits          341      341           
  Misses         98       98           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skjnldsv
Copy link
Contributor

skjnldsv commented Apr 4, 2024

Can you explain what view reactivity issues you're facing?

(e.g. user settings etc).

Any setting in particular?

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

Any setting in particular?

show_hidden


Problem I want to solve:

  • Hidden files should be user configurable
  • Hidden files that are favorites should be always listed in favorites view
  • Files inside hidden folders should not be listed on the recent view if not enabled

For the last one we need to update the root response of getContents for other files the files list will handle hidden files, but for the root view we need to manage it.
We can hack this into the files list, but it feels cleaner to let views decide what to show there.

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

Need this for nextcloud/server#44651

Copy link
Contributor

@skjnldsv skjnldsv 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 adjusting this should be simpler and enough

The complexity layer added by https://github.com/nextcloud/server/pull/44651/files is far too much

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

I think adjusting this should be simpler and enough

I did this before, but it does not work out as it is dependent to the view you are in.
Of course we can hack in "if view === recent" and "elseif view === favorites", but I do not think this scales and does not work for external apps.

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

Following problem:
Have file .hidden/not-hidden marked as favorite.

  1. not-hidden should show up in Favorites-view
  2. not-hidden should not show up in Recent-view

Also currently it is not possible to react on content changes for other views than the default one.
For the default one you can use the files:node:created event, but this does not work for custom views.

@skjnldsv
Copy link
Contributor

skjnldsv commented Apr 4, 2024

Second suggestion, have the current view reload when a setting change 🤷
Then you can filter however you want from your getContents method

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

Second suggestion, have the current view reload when a setting change 🤷

Yes but this causes a lot of unnecessary reload and waiting time. Because if not doing a hard reload the files list will not reload.

What do you think about an other option: Emitting an files:view:reload event that causes the files list to reload current content? No library changes needed, only 2 lines in the files list?

@skjnldsv
Copy link
Contributor

skjnldsv commented Apr 4, 2024

Events are not here to trigger a chain reaction, but to notify a reference change.
We ruled against that kind of mechanism in the past :)

@skjnldsv
Copy link
Contributor

skjnldsv commented Apr 4, 2024

Yes but this causes a lot of unnecessary reload and waiting time. Because if not doing a hard reload the files list will not reload.

Ah, I see the misunderstanding. I meant a file reload, not the full view. Just a getContents refresh, which you would get anyway

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

But also that will cause a lot of unneeded reloads, because the view can not decide if needed (even if it knows) but is forced to do.

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

But yes that would be possible

@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

server only solution:

nextcloud/server#44661

@susnux susnux closed this Apr 4, 2024
@susnux susnux deleted the feat/allow-reactive-content branch April 4, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants