Remove the dependencies on the UI of js backend files

[mossroy]

In zimArchive and zimArchiveLoader files, there are error cases where we display an error message to the user.
It was initially done using window.alert, and is now done using uiUtil.systemAlert.

In all cases, a backend js file should not depend on the UI: it breaks the software layers.
For example, this code might one day run inside a Worker, where it would not have access to the UI at all, and this would simply fail.

The error should instead be returned to the caller, that would have the responsibility to handle it: maybe display it to the user, or something else

[mossroy]

It should be possible to do that by adding an error callback to the functions (just like we have callbacks to return the results)

Another option would be to change the code to use Promises instead: we had thought about it in #67, but I don't think it's worth it. It would be a much more significant change for a code that will (hopefully) become deprecated when we manage to use wasm lbzim instead.

[Jaifroid]

I see the issue, especially with regard to a WASM/ASM backend which does indeed run in a worker and is isolated from UI completely. Thanks for the explanation.

[gaurava05]

Since my current PR is almost complete, I would like to take this up.

[gaurava05]

I can't work on this before merging of #804, right? Or, should I branch out based on the branch for #804?

[Jaifroid]

@gaurav7019 It would be easier to work on this after merging #804, though you can always start it and then rebase on master later if you're comfortable doing that. If you work on things not touched by #804 at first, then rebase would be easier.

[gaurava05]

I'll maybe wait for #804 to merge, meanwhile will work on #804 and create another issue that I've in mind.

[mossroy]

Now that we have merged #804 (and several other ones), you can tackle this one and #841
The goal is to remove the requirejs dependencies on uiUtil from the following files:

• zimArchiveLoader.js
• settingsStore.js
• zimArchive.js

Ideally, we should remove them from xzdec_wrapper.js and zstddec_wrapper.js too. But it's certainly more complicated.
We can leave these 2 ones aside for now, and focus on the first 3 ones.

In the end, we shoud be able to remove the uiUtil parameters in the following lines at the beginning of each of these files:

define([..., 'uiUtil'],
       function(..., uiUtil) {

To be able to do that, we have to remove the calls to uiUtil.systemAlert in them.
To do that, you'll have to parse every call to uiUtil.systemAlert in these files, and find a way to inform the caller of the function (the one in app.js) that there has been an error (with the error message). And this caller should use uiUtil.systemAlert in app.js (which has the dependency on uiUtil)

For zimArchive*.js, a clean solution should be to add a parameter callbackError to the existing functions. When there is an error, you could run callbackError(the_error_message). In the caller of app.js, you would pass a lambda function, that simply calls uiUtil.systemAlert with the given error message.

For settingsStore.js, it will be a bit different. I think we should remove the performReset inner function, so that reset always does the job. And the confirmation should be asked by the caller, in app.js. There is only one line in app.js where settingsStore.reset is called without parameters: it's where we should call uiUtil.systemAlert to ask the confirmation before calling settingsStore.reset

@Jaifroid, don't hesitate to say if you disagree or have better solutions

[mossroy]

Regarding *dec_wrapper.js, here is a possible solution:
We could store the *MachineType and error messages in variables of each *dec_wrapper.js. It's already the case for *MachineType: only the error message needs to be added. These 2 variables should also be exported (at the end of the file, like Decompressor)
When there is an error in one of these wrappers, the variables would be assigned values, instead of calling a function from uiUtil.

In app.js, we could add some code that runs on startup, and reads the *MachineType of each wrapper. If it's null, it should try again one second later (with a setTimeout call) until it's not null. Then it would call the uiUtil.reportAssemblerErrorToAPIStatusPanel function

[Jaifroid]

Sounds good to me, though I had to look up "lambda function" (today I learnt...). I'd just add (I didn't check code) that where there is an error being reported inside the catch of a Promise, then the Promise will pass that error to the catch function of the calling function (which might need to be added if there isn't one). In such cases it should be easy, but I don't know from memory if there are many such cases.

It might be easier to break this into a couple of PRs, one with easy cases, and another with the more complicated ones (like the wrappers), which we can collaborate on.

[Jaifroid]

In app.js, we could add some code that runs on startup, and reads the *MachineType of each wrapper. If it's null, it should try again one second later (with a setTimeout call) until it's not null. Then it would call the uiUtil.reportAssemblerErrorToAPIStatusPanel function

That's maybe not necessary. We only need to show the decompressor type in the API panel when the user clicks on Config. And if it's not ready on startup when Config is first shown, then we can simply show "Decompressor not initialized" (which currently shows in a number of cases anyway). Thereafter, the object params.decompressorAPI (and its properties) can simply be used as the source for the display function, as it's a global object (as you suggest).

[gaurava05]

you would pass a lambda function

@mossroy By this, do you mean an anonymous function? Because being on ES5 we can't use arrow functions I suppose.

[gaurava05]

Also, the ZIMArchive function (which uses uiUtil.systemAlert) defined in zimArchive.js is not called by app.js directly (In fact app.js doesn't depend on zimArchive.js at all ). It's being called by functions defined in zimArchiveLoader.js which are then called by app.js. So, will have to pass the callbacks at multiple levels.

Will that work fine?

[mossroy]

In app.js, we could add some code that runs on startup, and reads the *MachineType of each wrapper. If it's null, it should try again one second later (with a setTimeout call) until it's not null. Then it would call the uiUtil.reportAssemblerErrorToAPIStatusPanel function

That's maybe not necessary. We only need to show the decompressor type in the API panel when the user clicks on Config. And if it's not ready on startup when Config is first shown, then we can simply show "Decompressor not initialized" (which currently shows in a number of cases anyway). Thereafter, the object params.decompressorAPI (and its properties) can simply be used as the source for the display function, as it's a global object (as you suggest).

You're probably right, and it would indeed be much simpler.

[mossroy]

you would pass a lambda function

@mossroy By this, do you mean an anonymous function? Because being on ES5 we can't use arrow functions I suppose.

You're totally right. I meant an anonymous fonction. My bad

[mossroy]

Also, the ZIMArchive function (which uses uiUtil.systemAlert) defined in zimArchive.js is not called by app.js directly (In fact app.js doesn't depend on zimArchive.js at all ). It's being called by functions defined in zimArchiveLoader.js which are then called by app.js. So, will have to pass the callbacks at multiple levels.

Will that work fine?

It should. You will have to add the callbackError function as a parameter of the intermediate functions, and transmit the errors if any.

[mossroy]

I reopen this, because there are still the *dec_wrapper.js to handle

[BirushaNdegeya]

@gaurava05

I am going to work on this task. If you have attempted to solve this issue before, I would appreciate it if you could share what you have done so far. It would help me as a beginner in this project.

[Jaifroid]

@gaurava05 You can see what was previously tried in #1273, but that PR got stuck with failing tests and never got completed. As a beginner, please read CONTRIBUTING.md carefully (in root of Repo) all the way through, in particular what it says about setup and testing.

[Jaifroid]

In fact, @gaurava05, this is an issue you previously worked on, and largely solved with the exception of one file! So please look back at your previous work in the history above!

[gaurava05]

Hey @BirushaNdegeya, you can find all my work and discussions in the PR #845. Please have a look.

[BirushaNdegeya]

@gaurava05 Thanks!