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

Adding pagination capability #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mahsashadi
Copy link

@mahsashadi mahsashadi commented Nov 16, 2021

File manager'd better to support pagination for directories with lots of files.

  • Every time a new mountpoint /directory is selected, vfs stat function is called to have total number of files and total size. It is needed for statusbar and also for detecting termination of fetching pages (if pagination is supported).
  • Every time a new mountpoint is selected, vfs capability function is called to detect if it supports pagination, and if so, what is its pageSize
  • If pagination is supported, every time the scroll hits the bottom it fetches the new page, and the previous list is merged by the new list (probable repetetive items checked to remove)- it repeats till all pages are fetched.

@mahsashadi mahsashadi changed the title pagination capability is added Adding pagination capability Nov 16, 2021
@mahsashadi
Copy link
Author

mahsashadi commented Nov 22, 2021

Hi @andersevenrud , I have two questions:

  1. SORTING: where should I check if adapter (server-side) do sorting, and if not do it client-side? I think, since we have added capabilities method which tells us about server-side sorting: (A) we can check it in filemanager app itself (we already have capabilities response by clicking on any mountpoint). In this way transformReaddir function should not do sorting. (B) we can call capabilities in tranformReaddir, but in this way, we call capabilities whenever readdir is called, which is not efficient.

  2. REFRESHING: What is your suggestion for refreshing list to support actions like delete, paste and rename while pagination is active. Imagine we have deleted a file within one of our previous fetched pages. Should I again fetch all previously pages? (sorry, previously asked here)

@mahsashadi
Copy link
Author

@andersevenrud

Hi @andersevenrud , I have two questions:

  1. SORTING: where should I check if adapter (server-side) do sorting, and if not do it client-side? I think, since we have added capabilities method which tells us about server-side sorting: (A) we can check it in filemanager app itself (we already have capabilities response by clicking on any mountpoint). In this way transformReaddir function should not do sorting. (B) we can call capabilities in tranformReaddir, but in this way, we call capabilities whenever readdir is called, which is not efficient.
  2. REFRESHING: What is your suggestion for refreshing list to support actions like delete, paste and rename while pagination is active. Imagine we have deleted a file within one of our previous fetched pages. Should I again fetch all previously pages? (sorry, previously asked here)

Hi again. any suggestion for these two questions?

@andersevenrud
Copy link
Member

  1. Hm. An extra capabilities call would definitely introduce some overhead. Maybe just remove the arguments on that call (no more path etc.) ? That way it could be cached. The capabilities should not be called within transformReaddir however because utility functions like this should not really have any side effects. It fits better in the top VFS abstraction layer, since this is where this function is used.
  2. Not sure if re-fetching everything is optional. It could be possible just to update the list that the UI renders if the VFS call(s) were successful.

@mahsashadi
Copy link
Author

  1. Maybe just remove the arguments on that call (no more path etc.) ? That way it could be cached

I did not understand this part.

2. It could be possible just to update the list that the UI renders if the VFS call(s) were successful.

Yes. In this way, we need to find a quick way to insert data, in such a way the list (which prpbably is huge) remains sorted

@andersevenrud
Copy link
Member

andersevenrud commented Nov 26, 2021

I did not understand this part.

Right now the signature is like this: await vfs.capabilities(path); because we discussed the option of having capabilities that are unique to directories, not just the VFS adapter.

If this was removed then this only would have to be called once and cached (possibly with lifetime...if reloading turns out not to be optimal).

So the VFS abstraction for this would be:

const capabilityCache = {}

export const capabilities = (adapter, mount) => () => {
  const cached = capabilityCache[mount.name]
  if (cached) {
    return Promise.resolve(cached)
  }

  return adapter.capabilities(mount)
    .then(capabilities => {
      capabilityCache[mount.name] = capabilities
      return capabilities
    })
}

@mahsashadi
Copy link
Author

mahsashadi commented Dec 26, 2021

Hi again @andersevenrud
I was dealing with COVID 19, so I think There was a long break! Now I am fine and ready to Finish this project.

I did what you said about sorting here, but it seems server-side vfs does not support requests without any argument. Is it is OK to send path in capabilities api for caching goal. If not, how can we support that?

@andersevenrud
Copy link
Member

Hey @mahsashadi . Glad to hear you're doing fine :)

Is it is OK to send path in capabilities api for caching goal. If not, how can we support that?

Hm. It would probably be best to create a PR that changes the api to support methods without arguments 🤔

@mahsashadi
Copy link
Author

@andersevenrud Hi, I have fixed problems about pagination in file manager app. I hope you have time to examine them.

@andersevenrud
Copy link
Member

Sorry for the late reply.

I will take a thorough look this weekend :) Work days is a bit to busy for me to give this my full attention.

@mahsashadi
Copy link
Author

For this pull request on file manager, these two pull requests should also be considered:
os-js/osjs-client#165
os-js/osjs-server#56

@andersevenrud andersevenrud self-assigned this Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants