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

Improve/add features to the Ace module such that current alterations/patches are not needed #2

Open
bouzidanas opened this issue May 29, 2023 · 0 comments
Labels
enhancement New feature or request

Comments

@bouzidanas
Copy link
Owner

bouzidanas commented May 29, 2023

Core scripts (and extensions/plugins/themes) inside the ace-builds and react-ace node modules have been modified/patched to work with streamlit-code-editor. The problem is multifold.

  1. These modules should not have code specific to any application that uses said module.
  2. The functionality these changes provide should be transferable to future versions of the modules so that component doesnt break when any of the modified modules are updated.

The original reason for such modifications was that these modules were broken and/or are generally no longer maintained/updated.

Examples:

  • 'keyboard' was misspelled in some places the Ace keyboard shortcut extension which broke imports and prevented the extension from working.
  • Ace does not have the ability to set the default theme to something other than textmate and loading another theme when component is rerenders happens with a delay resulting in a flicker between themes.
  • Ace doesnt have theme removal functionality.
  • React-Ace lacks a way to get mode id from Syntax type object

Enhancement: General fixes and features should be added to Ace and React-Ace so that app-specific modifications are not needed.

For example, to add theme removal feature, a new attribute can be added to the theme module object containing other themes to remove after theme is loaded. The theme modules themselves can be defined with this attribute set:

Current solution:

ace.define("ace/theme/streamlit_light", ["require", "exports", "module", "ace/theme/streamlit_light.css", "ace/lib/dom"], function (require, exports, module) {
    exports.isDark = false;
    exports.cssClass = "ace-streamlit-light";
    exports.cssText = require("./streamlit_light.css");
    exports.$id = "ace/theme/streamlit_light";
    var dom = require("../lib/dom");
    dom.importCssString(exports.cssText, exports.cssClass, false);
    dom.removeElementById("ace-streamlit-dark");    //removes the style element containing the css text for the theme this theme replaces
});

And inside setTheme function:

function afterLoad(module) {
    if (_self.$themeId != theme)
        return cb && cb();
    if (!module || !module.cssClass)
        throw new Error("couldn't load module " + theme + " or it didn't call define");
    if (module.$id)
        _self.$themeId = module.$id;
    dom.importCssString(module.cssText, module.cssClass, _self.container);

    // The following IF/ELSE is a workaround for a theme replacement bug in
    // Streamlit component that uses ACE
    if (module.cssClass == "ace-streamlit-light") 
        dom.removeElementById("ace-streamlit-dark");
    else if (module.cssClass == "ace-streamlit-dark")
        dom.removeElementById("ace-streamlit-light");
    // ... //

Perhaps something like this would be better:

ace.define("ace/theme/streamlit_light", ["require", "exports", "module", "ace/theme/streamlit_light.css", "ace/lib/dom"], function (require, exports, module) {
    exports.isDark = false;
    exports.cssClass = "ace-streamlit-light";
    exports.cssText = require("./streamlit_light.css");
    exports.$id = "ace/theme/streamlit_light";
    exports.cssRemove = ["ace-streamlit-dark"]
    var dom = require("../lib/dom");
    dom.importCssString(exports.cssText, exports.cssClass, false);
   replaces
});

And inside setTheme function:

function afterLoad(module) {
    if (_self.$themeId != theme)
        return cb && cb();
    if (!module || !module.cssClass)
        throw new Error("couldn't load module " + theme + " or it didn't call define");
    if (module.$id)
        _self.$themeId = module.$id;
    dom.importCssString(module.cssText, module.cssClass, _self.container);
    if (module.cssRemove && module.cssRemove.length > 0) {
        module.cssRemove.forEach( function(cssClass) {
            dom.removeElementById(cssClass);
        });
    }
    // ... //

Obviously more consideration is needed.

@bouzidanas bouzidanas added the enhancement New feature or request label May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant