eclipse / nebula

Nebula Project
https://eclipse.org/nebula
Eclipse Public License 2.0
84 stars 97 forks source link

RichTextEditor Slow to Open #571

Closed ans-kanec closed 3 months ago

ans-kanec commented 4 months ago

When using the RichTextEditor in a basic SWT form control, the initial load of the control takes around 30 seconds to fully open and initialize. Subsequent openings of the control only take 1-2 seconds. If I remove the RichTextEditor component, the initial load of the control is 1-2 seconds. It appears there is a performance issue with the initial load of the RichTextEditor component. Is there anything that can be done to speed up this initial load process?

merks commented 4 months ago

Can you share a sample that demonstrates the problem?

ans-kanec commented 4 months ago

`import org.eclipse.nebula.widgets.richtext.RichTextEditor; import org.eclipse.nebula.widgets.richtext.RichTextEditorConfiguration; import org.eclipse.swt.SWT; import org.eclipse.swt.layout.GridLayout; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Shell;

public final class RichTextTest { public static void main(String[] args) { Display display = new Display();

    final Shell shell = new Shell(display);
    shell.setText("SWT Rich Text Editor example");
    shell.setSize(500,500);

    shell.setLayout(new GridLayout(1, true));

    RichTextEditorConfiguration rtec = new RichTextEditorConfiguration();               
    RichTextEditor textEditorBody = new RichTextEditor(shell, rtec, SWT.NONE);

    shell.open();
    while (!shell.isDisposed()) {
        if (!display.readAndDispatch())
            display.sleep();
    }
    display.dispose();
}

}`

ans-kanec commented 4 months ago

This can also be observed with the RichTextEditorExample:

https://github.com/eclipse/nebula/blob/master/widgets/richtext/org.eclipse.nebula.widgets.richtext.example/src/org/eclipse/nebula/widgets/richtext/example/RichTextEditorExample.java

ans-kanec commented 4 months ago

I believe I have narrowed down the issue. The problem exists in the 'RichTextEditorConfiguration' with the following code: ` private static URL resourceURL; static { resourceURL = RichTextEditorConfiguration.class.getClassLoader().getResource("org/eclipse/nebula/widgets/richtext/resources/ckeditor/lang");

    // if we are in an OSGi context, we need to convert the bundle URL to a file URL
    Bundle bundle = FrameworkUtil.getBundle(RichTextEditor.class);
    if (bundle != null) {
        try {
            resourceURL = FileLocator.toFileURL(resourceURL);
        } catch (IOException e) {
            e.printStackTrace();
        }
    } else if (resourceURL.toString().startsWith("jar")) {
        BusyIndicator.showWhile(Display.getDefault(), new Runnable() {

            @Override
            public void run() {
                resourceURL = ResourceHelper.getRichTextResource("ckeditor/lang");
            }
        });
    }

    File directory = new File(resourceURL.getFile());
    File[] files = directory.listFiles();
    if (files != null) {
        for (File file : files) {
            SUPPORTED_LANGUAGES.add(file.getName().substring(0, file.getName().indexOf('.')));
        }
    }
}`
fipro78 commented 4 months ago

IIUC you want to use the RichTextEditor in a plain SWT setup, is that correct? So no OSGi involved.

In that case your assumption is only halfway correct, but related. The code related to that slow startup is in ResourceHelper#extractResources().

The RichTextEditor control uses the ckeditor in a SWT Browser. ckeditor consists of multiple files that use relative paths to connect. It is not possible to load this from within a jar file, as relative paths can't be resolved in those resources. Therefore the resources need to be extracted and referenced from the file system.

In an OSGi context, e.g. an Eclipse RCP based application, the resources are already extracted in the plugins folder because of the following instruction in the MANIFEST.MF:

Eclipse-BundleShape: dir

Starting the example from the checked out sources also don't show the issue, because the files are located from the sources.

But in your case, the extract operation is executed. And file I/O operations are slow. Without additional configuration, the extract operation is done to a temporary directory. And temporary directories are changing on every start. Therefore the extract operation is executed on every new startup.

You can configure a directory to extract to via the system property org.eclipse.nebula.widgets.richtext.jar.unpackdir. The following snippet is from the NatTable examples to do this:

        // set the directory to which the richtext resources should be unpacked
        System.setProperty(
                RichTextEditor.JAR_UNPACK_LOCATION_PROPERTY,
                System.getProperty("user.dir")
                        + File.separator
                        + RichTextEditor.class.getPackage().getName());

This way the files are extracted to that directory, and on subsequent calls, it will check if the files in that directory already exist. That means, you only need to suffer from the slow startup once, but on subsequent calls, the startup should be fast. On the other side it comes with the downside, that if the ckeditor version is updated in the future, you need to delete that folder in advance, otherwise you will never get the new ckeditor version. But compared to the slow startup issue, an acceptable risk IMHO, as the ckeditor version is rarely updated.

laeubi commented 4 months ago

On the other side it comes with the downside, that if the ckeditor version is updated in the future, you need to delete that folder in advance, otherwise you will never get the new ckeditor version.

What do you think about placing a .version file in the folder and if the version do not match automate the process of delete/extract? Or even using a versioned folder inside the specified one?

laeubi commented 4 months ago

I also like to note that BusyIndicator#showWhile is likley used wrong here as it does not drive the SWT EventQueue, BusyIndicator.execute(SwtRunnable<E>) is a more modern and save variant of this.

fipro78 commented 4 months ago

What do you think about placing a .version file in the folder and if the version do not match automate the process of delete/extract? Or even using a versioned folder inside the specified one?

Yes, that would be an option. We should keep this in mind once there are plans to update the ckeditor version.

I also like to note that BusyIndicator#showWhile is likley used wrong here as it does not drive the SWT EventQueue, BusyIndicator.execute(SwtRunnable<E>) is a more modern and save variant of this.

That might be, although I am actually not aware of the method you mention. But probably because it was introduced with version 3.123, and the RichTextEditor was implemented with an older version of SWT and is also intended to be used with older versions. Such an update would probably break a lot of applications out there if they try to update only the RichTextControl.

But thanks for the hint. Definitely something to consider on an update.

laeubi commented 4 months ago

If you want to retain backward compatibility, you need to spin a new Thread or Completable future in the run and drive the event queue otherwise it will block the main thread. Of course you can also just look how it is implemented in SWT and be inspired there :-)

fipro78 commented 4 months ago

Hm, well if we don't block the main thread at that special position, the UI probably breaks and fires exceptions, because the necessary resources needed for rendering are not available. I mean, if the resources are not copied and available for the further processing, there are exceptions and the application breaks. At least in setups like the examples above, where there is only a RichTextEditor shown directly. SO IIRC the idea back then was actually to block the main thread in that situation. Doing the stuff really in background caused troubles. But that is just recalled from memory. Quite a while since I implemented this. And maybe there are more elegant variants to solve this nowadays.

laeubi commented 4 months ago

You will still block the further control flow here (like with a modal dialog), but in the current form you will get an "unresponsive" warning and for example can't minimize/maximize the window or close it.

You can see the "old" way here in the snippet https://github.com/eclipse-platform/eclipse.platform.swt/blob/1f251edf602f89449cb3c484a6fd9218d9dcc2aa/examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet130.java

ans-kanec commented 4 months ago

Thanks for the information. Setting the system property for the unpack location seems like the best approach so far.

Is it possible to have it load only one of the language packs? Seems like this would allow for initial performance increases as well.

fipro78 commented 4 months ago

No that is not possible. And actually I don't think that is really an issue. It does not load anything, it only lists the files in the folder. At least I don't notice the mentioned performance issue.

ans-kanec commented 4 months ago

Thanks for all the help, I have a good path forward now.

fipro78 commented 3 months ago

Sounds like this issue can be closed, as there is no open issue.