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

[ADD] Native File System support to reload zim file #1131

Merged
merged 47 commits into from
Nov 8, 2023
Merged

Conversation

Rishabhg71
Copy link
Collaborator

@Rishabhg71 Rishabhg71 commented Oct 11, 2023

Summary and Progress 👇

  • Using FS API get the file and try to load it
  • Save the file in indexDB and Load it again
  • UI for File selection under the config
  • Adding logic and File selection to UI
  • Try to do the same steps but for the whole directory
  • Code cleanup
  • [LATER] add webkit as a fallback

@Rishabhg71
Copy link
Collaborator Author

I have completed a small working prototype. You can Select File and it will reload it back. A few things I don't like are

  • If multiple files are selected browser is gonna request permission for each which will be annoying to the user (in case of split zim could be more than 10). So should I only let the user select a single file? That would mean that FS API will be unable to load split zims (will be only done via Folder select)
  • As you saw I first asked for a confirmation to load the previous Zim file and then requested file access, Do you have any suggestions on how to request file
kiwix.mp4

@Jaifroid
Copy link
Member

@Rishabhg71 That's a great start!

Yes for the reason you state, I limit (in Kiwix JS PWA) the single file picker to a single file if the user is using the File System Access API. If they are using the normal file picker, then they can pick more and all the picked files will show in the dropdown list of archives.

And this is the reason I provided a "folder picker" as a separate button. It completely avoids the issue, because the user only has to give permission to access the (previously picked) folder, and automatically all files in the folder become accessible.

So, I advise you to finish the implementation for single file picking first -- don't try to make it into a multi-picking solution -- and then when you've done that (it seems it's nearly there), you can add folder picking.

Since every browser that supports the showOpenFilePicker method also supports the showDirectoryPicker method, it seems the logical way to handle multiple files and split files (through the directory picker only). It's by far the most convenient method, and I use it all the time in the PWA version.

Firefox is a special case: it doesn't support showOpenFilePicker at all, but it does support webkitDirectory, which is almost exactly the same as showDirectoryPicker.

(Don't forget to handle a single drag-dropped file: you can test if it has a filehandle, and if it has, then it's exactly the same as if you had picked it with showOpenFilePicker. You need to leverage the existing drag-drop handle in app.js -- handleFileDrop()).

@Jaifroid
Copy link
Member

Firefox is a special case: it doesn't support showOpenFilePicker at all, but it does support webkitDirectory, which is almost exactly the same as showDirectoryPicker.

I should just qualify that: in Firefox, you can't store the permission in IndexedDB, the user has to re-pick the directory. But I made it so when a user clicks on the dropdown filename they want to open (stored in a cookie, and restored on app startup), the directory picker is opened, and the user only needs two clicks to re-authorize access.

@Rishabhg71
Copy link
Collaborator Author

@Jaifroid You can have a quick look and check if everything is working as it's supposed to be meanwhile I am starting with select directory implementation

@Jaifroid
Copy link
Member

@Rishabhg71 It seems to be working well! NB I didn't look at the code.

I think in the final version, instead of asking the user if they want to reopen the last ZIM file, it would be better to display it in the dropdown list of archives. Then the user simply clicks on it if they want to reopen it, or else they click on the file or directory picker if they want to pick a different archive. It's the same number of clicks to reopen (two - once on the file, once on Verify), but it's less intrusive.

@Jaifroid
Copy link
Member

NB In the future we will get persistent permissions for this API, at least if the app is installed. So we will want to try on startup to launch the last selected archive, and see if we have permission. For now, however, it's fine just to invoke the verify permission when the user makes a gesture. Unfortunately, launching the app isn't considered a sufficient user gesture to show the verify permission dialogue. You need an actual click in the UI.

@Rishabhg71
Copy link
Collaborator Author

UPDATE: Directory pickup and selection are also done but I am not happy with the messy code so doing refactors now

@Rishabhg71
Copy link
Collaborator Author

Rishabhg71 commented Oct 15, 2023

kiwix.mp4

Is the folder drop behavior a bug, or is it intended? @Jaifroid
This only occurs when dropping folders

@Jaifroid
Copy link
Member

Is the folder drop behavior a bug, or is it intended?

@Rishabhg71 It was intended: if a user drags and drops, then they don't need file picker, so it can be tidied away. However, it might not be so ideal for your implementation, and can be changed. In Kiwix JS PWA (which has now changed from kiwix-js-windows to kiwix-js-pwa, and the development pwa is now at https://kiwix.github.io/kiwix-js-pwa/ ), dragging a file or a folder tucks away the file pickers behind a "Select storage" button, rather than hiding them completely, and then I decided that when a user has successfully picked a file, the file pickers should always be tucked away, so the UI is not so cluttered. Particularly important if we implement the OPFS in the future on Kiwix JS, because adding an OPFS quota panel clutters the interface even more...

But I'm happy for you to do what seems best for a smooth implementation at this stage!

@Jaifroid
Copy link
Member

@Rishabhg71 I'm presuming this isn't ready for review, as it's still marked as Draft, and tests are failing. But let me know if/when you need any input from me.

@Rishabhg71
Copy link
Collaborator Author

PR is nearly done but I am stuck on a bug where the Dropping folder isn't saved into DB
It should be done by tomorrow or day after tomorrow

@Jaifroid
Copy link
Member

OK, no problem! I just wanted to check you weren't waiting for me.

www/js/lib/fileSystem.js Fixed Show fixed Hide fixed
@Rishabhg71
Copy link
Collaborator Author

@Jaifroid could you quickly check out the functionality? I've got folder selection working for WebKit and FS. Still, there are a few test cases I need to add, like making sure it doesn't display non-ZIM files.

Also, can you guide me a little on how split-zim works? Let's say we have a zim split in 4 parts so I will get [zima, zimb, zimc, zimd] but what if it's more than 26 files?

@Jaifroid
Copy link
Member

Jaifroid commented Oct 18, 2023

Also, can you guide me a little on how split-zim works? Let's say we have a zim split in 4 parts so I will get [zima, zimb, zimc, zimd] but what if it's more than 26 files?

You just need to get all the files in a directory that match /\.zim\w\w$/i.test(fileName) (and where the fileName without the split extension letters matches the selected file), then you present them to the file loader in any order, and it will take care to sort them into the right order. It doesn't matter if they are 2 splits or 58 splits.

There is code that handles looping through all files in a directory and matching against the selected archive in kiwix-js-pwa, look for the function processDirectoryOfFiles in app.js. The typical way to do this is something like:

return processNativeDirHandle(directoryHandle, function (fileHandles) {
     processDirectoryOfFiles(fileHandles, selectedArchive);
});

So, I take a native directory handle, iterate over the contents to get all the file handles it contains (at least those that match ZIM archives), and then take all those handles and pass them through processDirectoryOfFiles, with the string of the archive the user has selected (in the case of split files, this is just the .zimaa part). It then takes care of matching the correct file and getting the ZIM parts for split archives.

@Jaifroid
Copy link
Member

@Rishabhg71 I just did a quick test (not the webkit part, just the File System Access API) of folder picking, and it's very impressive. Well done! It works smoothly, and works nicely after reload as well, with the files still listed in the dropdown.

There are some UI tweaks that could be made, but they can be done after core functionality is finished. Before I forget them, I'll list them here anyway, but focus on functionality first:

  • Once a user has picked an archive or a folder of archives, it would be good to have a message above the dropdown displaying the number of archives it contains ("5 archives found in selected location:");
  • Don't display "Select a file..." if there are no files in the dropdown, as it's confusing and distracts the user from noticing the folder selection button. It would be better for the default entry to be blank. However, once the dropdown is populated, it is worth displaying "Select a file..." (so the user only goes to the dropdown when they can see that there are files to select in it);
  • I think the dropdown could be more like 60% of the div width, rather than 100%, as it looks a bit imposing!
  • New UI elements will need to be translated to French and Spanish (but only at end).

Very encouraging work!

www/js/lib/fileSystem.js Fixed Show fixed Hide fixed
www/js/lib/fileSystem.js Fixed Show fixed Hide fixed
@Rishabhg71 Rishabhg71 marked this pull request as ready for review October 19, 2023 18:37
@Rishabhg71 Rishabhg71 self-assigned this Oct 19, 2023
@Rishabhg71
Copy link
Collaborator Author

@Jaifroid I feel this PR is now ready for a review

@Jaifroid
Copy link
Member

I just started some testing (haven't looked at code yet), and I noticed that drag-and-drop appears to be broken in IE11. Could you take a look?

I haven't tried to debug, but in case the issue turns out to be async / await not compiling well for older browsers, then that can usually be cured by converting any functions that will run on older browsers to Promises. All async code can easily be converted to Promises, as async / await are just sugar for Promises under the hood.

@Jaifroid
Copy link
Member

Jaifroid commented Oct 20, 2023

It's looking very good so far (I've only been testing the UI, haven't reviewed any code). Based on my testing, here are a few suggested UI / UX issues / improvements:

  • When picking a folder of split ZIM files (e.g. our legacy Ray Charles test folder), the total number of archives reported above the dropdown list is given as the total number of files, whereas in fact there is only one archive that happens to be split (in the case of the Ray Charles ZIM). Since the dropdown list correctly shows only the .zimaa file, you can simply get the correct total number of archives from the length of the dropdown list of archives once it has been populated;
  • After a user has picked a folder of archives, it would be good to change the blank first line in the dropdown to "Select an archive...";
  • If there is only one archive in the folder (including only one split archive), it seems to me that it would be friendly to launch that archive immediately (the same as happens if picking a single file), otherwise there are a lot of clicks to open an archive from a folder.

www/js/lib/fileSystem.js Fixed Show resolved Hide resolved
@Rishabhg71
Copy link
Collaborator Author

If there is only one archive in the folder (including only one split archive), it seems to me that it would be friendly to launch that archive immediately (the same as happens if picking a single file), otherwise, there are a lot of clicks to open an archive from a folder.

Everything is fixed except this, I tried to load the zim if there is only one file but it's making the code messy
(a lot of unnecessary returns)

@Rishabhg71 Rishabhg71 requested a review from Jaifroid October 20, 2023 14:53
@Jaifroid
Copy link
Member

OK, looking at it now!

@Jaifroid
Copy link
Member

Also just noticed that when I've picked a single archive (in browser supporting window.showOpenFilePicker) and then I refresh the page, the Select an archive... message is still not being shown (on latest code). The same happens with the folder picker.

image

@Jaifroid
Copy link
Member

Jaifroid commented Oct 31, 2023

Final thing I noticed: on modern Firefox, like on Safari, after I've picked a folder and then I refresh the app (or re-launch it), the dropdown list of archives isn't re-populated. Since all it takes is storing it in settings (see populateDropDownListofArchives function, which does this), it shoudl be very easy to set and then restore the folder contents. Clicking on one of those files should simply evoke the folder picker again, so the user can re-pick the folder quickly and easily., since we're talking about browsers that rely on webkitdirectory property. (That's how it works for those browsers in the PWA.)

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

As I've now identified a number of inconsistencies through my UI tests, it would be good to see if some of those can be ironed out. Don't get me wrong, you've done a great job on all these different APIs, and it's a huge improvement. These are just the last niggly details to ensure a good and consistent UX. ㄟ( ▔, ▔ )ㄏ

@Rishabhg71
Copy link
Collaborator Author

image

By the way, Folder picking works on Safari, though the list of archives is not re-populated after I refresh the page. Not sure if that's intended.

I tested on safari it seems like it doesn't support FileSystemAPI (safari 17)

@Jaifroid
Copy link
Member

Jaifroid commented Nov 3, 2023

I tested on safari it seems like it doesn't support FileSystemAPI (safari 17)

Safari supports webkitdirectory just like Firefox. So it's exactly the same case as I mentioned in #1131 (comment). A fix in Firefox would (should) also fix Safari and all other browsers that support webkitdirectory (like Edge 18).

@Rishabhg71
Copy link
Collaborator Author

Rishabhg71 commented Nov 4, 2023

  • Remove JS injection (XSS)
  • Android/IOS file picker inconsistencies
  • Open dir pickup on file select (refresh)

@Rishabhg71
Copy link
Collaborator Author

@Jaifroid Can you review again? I have added a feature that the last file will be loaded automatically on folder select but it doesn't work on folder drop. I tried my best to make it work on folder drop but it's making the code messy

@Jaifroid
Copy link
Member

Jaifroid commented Nov 6, 2023

@Jaifroid Can you review again? I have added a feature that the last file will be loaded automatically on folder select but it doesn't work on folder drop. I tried my best to make it work on folder drop but it's making the code messy

No problem, I'm taking a look now. I don't think I supported folder drop either in the PWA.

@Jaifroid
Copy link
Member

Jaifroid commented Nov 6, 2023

It's working really well! I tested in Edge and Firefox, and actually I got good results with the folder drop in Firefox. I don't think it's possible to do it better: it seemed to remember the files in the folder between app launches, and prompted me to re-upload the folder when I selected an archive from the dropdown. It's not possible to get it only to ask for a permission prompt because with webkitdirectory property, there is not possibility of storing the directory handle between sessions. So you've done as well as is possible with the API, congratulations!

I'll test on IE11 and a few other things after our conversation, but it's looking great!

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

This looks very good. Well done! I've just got some final suggestions that hopefully you'll find helpful. Don't feel you have to use them, but it might just make things a bit more intuitive and not make some of the code appear to depend on the non-existence of a defunct Firefox OS!

The only possible "bug" I noticed is that on Safari (tested on macOS Sonoma on BrowserStack), the folder picker fails to open when clicking on one of the archives in a previously picked folder after refreshing the page. It works fine on Firefox and on Edge 18 (both of which support webkitdirectory, like Safari), so it's puzzling that it doesn't work there. It works with the PWA on the same Safari version, so I suspect you've accidentally ruled out Safari on macOS somewhere. I'm sure it's a quick fix. I've set kiwix.github.io/kiwix-js/ and kiwix.github.io/kiwix-js/dist/ to the to the code on this branch, so it can be tested and debugged on BrowserStack easily.

After you've done those, I don't need to see the code again, so when you feel it's ready, please feel free to go ahead and Squash/Merge. And thank you for another great PR!

www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Show resolved Hide resolved
@Jaifroid
Copy link
Member

Jaifroid commented Nov 6, 2023

PS It would be a good idea to create an issue for the File System UI and/or Unit tests you mentioned. Definitely a separate issue, as I'd really like to get this merged asap, so we can do #1143.

@Rishabhg71 Rishabhg71 merged commit e652d4b into main Nov 8, 2023
9 checks passed
@Rishabhg71 Rishabhg71 deleted the fs-support branch November 8, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants