How to help with JSDialogs development (porting dialogs to native HTML)

Hi,
I’ve just prepared patch which will allow to enable any dialog to use JSDialogs on demand by using environment variable. (It’s not merged yet, but will be soon).
https://gerrit.libreoffice.org/c/core/+/131580

How to use it?
When running debug build of Online:
SAL_JSDIALOG_ENABLE=modules/scalc/ui/validationdialog.ui:modules/scalc/ui/validationcriteriapage.ui make run

as you can see I enabled 2 .ui files related to calc validity dialog. You can enable multiple dialogs, just use ‘:’ separator.

To enable any dialog: identify it’s .ui file in the LibreOffice repository eg. validationdialog.ui, then find the string used in the code as it can differ from file path in the repo. You can use this tool:
https://opengrok.libreoffice.org/search?project=core&full=“validationdialog.ui”&defs=&refs=&path=&hist=&type=&xrd=&nn=1&si=full&si=full
In the validate.cxx we can see usage of that file with string we should use in our environment variable.
In case dialog with tabs, you need to identify also all .ui files for tabs.

Then when you run online, that dialog will be created using native HTML controls. You can help with checking if dialog works correctly, all looks correctly and in case it needs some improvements - propose a PR.

2 Likes

Patch https://gerrit.libreoffice.org/c/core/+/131580 has been merged on
the core side. Thanks Szymon

A word of adviceL It’s important to test all dialogs functionality before enabling. Example: Special Paste looks ok (SAL_JSDIALOG_ENABLE=cui/ui/pastespecial.ui make run)
image

but it does not work as expected:

Control.JSDialog.js:350 Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline' 'self'".

onJSUpdate @ Control.JSDialog.js:350
fire @ Events.js:152
fireJSDialogEvent @ Socket.js:1406
_onJSDialog @ Socket.js:1421
_onMessage @ Socket.js:1194
_emitSlurpedEvents @ Socket.js:331
(anonymous) @ Socket.js:281
setTimeout (async)
_queueSlurpEventEmission @ Socket.js:279
_slurpMessage @ Socket.js:379

Good catch, I prepared patch for this error.

Also what we should check for dialogs: if they are “async”. It means multiple users can work with the same dialog without interrupting each other.

Steps for testing:
Please DO NOT enable JSDialogs for testing this case, we need old, canvas based dialog.

  1. user A opens dialog
  2. user B opens the same dialog
  3. user A does some action in dialog and clicks “OK”
  4. user B does some action in dialog and clicks “OK”

Expected: changes were applied in both sessions, dialog is closed correctly
Wrong result: dialog doesn’t close but starts to flicker, changes are not applied

Nice thanks for clarification!
Here is a short list of low hanging simple ones that might be easy to
convert:

 ◦ Writer
     ▪ Table: Split table dialog and
     ▪ Table: Number Format dialog (?)
     ▪ Image/shape: Right click, Caption dialog
 ◦ Calc
     ▪ Right click (cell) -> clear contents: Delete dialog
     ▪ Right click (column) -> Optimal column width dialog

I’ve started working on this; I was trying to convert the split table dialog however it doesn’t seem to be async. If I open it up on 2 instances of CODE and then submit the first one, changes aren’t applied until I submit the second one.

I’m now wondering about how I can make dialogs async? Does anyone have any pointers/suggestions for what I can look at?

Here is a sample commit that you can perhaps follow: https://gerrit.libreoffice.org/c/core/+/130438

2 Likes

Is there any existing (already converted dialog) that runs asynchronously but that has other code execution that acts as if the dialog was cancelled? I think pointing a commit like that would help immensely @NickWingate

I remember @szyklos mentioning that probably in those cases is not using the share pointer (but if its maybe we can store the reference in the view shell). Nevertheless I think in your case @NickWingate (with epub etc) we need to use UNO classes (UNO reference) but the concept would be the same, move class outside of the view shell .