Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Theme management #1390

Merged
merged 24 commits into from
Mar 30, 2021
Merged

Theme management #1390

merged 24 commits into from
Mar 30, 2021

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented Mar 26, 2021

Pull Request Description

This PR:

  1. Improves theme manager. Now it is possible to switch themes, however, still, not all elements are updating while switching. It introduces a FrpStyleManager which handles theme switching inside FRP networks.
  2. Fixes npm registry node-version issue.
  3. Fixes 2 serious internal bugs - one double mut borrow panic, and one which was causing style sheet variables to be released to early and thus, theme corruption.
  4. Introduces new color style management, including color::AnyFormat that can contain colors in any available format and has associated nice parsing and pretty-print options.
  5. Allows themes to be loaded from string inputs.
  6. Connects theme manager to JS and exposes functions to change, compare, clone, and inspect themes directly from JS Console and Chrome inspector (allowing to change theme properties with nice Chrome Inspector widgets).
  7. Fixes several small bugs in IDE visuals
  8. Fixes a light theme to look nicer, however, it will still be reworked slightly soon.

Checklist

Please include the following checklist in your PR:

  • The CHANGELOG.md was updated with the changes introduced in this PR.
  • The documentation has been updated if necessary.
  • All code conforms to the Rust style guide.
  • All code has automatic tests where possible.
  • All code has been profiled where possible.
  • All code has been manually tested in the IDE.
  • All code has been manually tested in the "debug/interface" scene.
  • All code has been manually tested by the PR owner against our test scenarios.
  • All code has been manually tested by at least one reviewer against our test scenarios.

@wdanilo wdanilo requested a review from farmaazon March 29, 2021 11:01
@wdanilo wdanilo marked this pull request as ready for review March 29, 2021 11:01
build/run.js Outdated
// The following ignore list is the same as in `src/rust/.gitignore`, but are written with
// qualified paths. This is a test if this will prevent some infinite-loop compilation with the
// Cargo Watch tool. If so, an error should be reported to their bugtracker.
let args = ['watch', '-i', 'src/rust/target', '-i', 'src/rust/ensogl/msdf-sys/msdfgen_wasm.js', '-i', 'src/rust/ide/parser/pkg/', '-s', `${target}`]
Copy link
Collaborator

Choose a reason for hiding this comment

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

long line. Does the prettier fix that?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@@ -33,6 +33,7 @@
<script type="module" src="/assets/run.js" defer></script>
</head>
<body>
<div id="colors-debug" style="--graph_editor-node-background:rgb(255 0 0); --graph_editor-node-shadow:rgb(0 0 0); --graph_editor-node-shadow-exponent:2.0;"></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this for? Can you add a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

Comment on lines 224 to 229
// for (let name of logsFns) {
// this.raw[name] = console[name]
// console[name] = (...args) => {
// this.handle(name,args)
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it commented?

Copy link
Member Author

@wdanilo wdanilo Mar 30, 2021

Choose a reason for hiding this comment

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

uncommented

@@ -0,0 +1,102 @@
//! Example scene showing simple usage of a shape system.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The file name is complex_shape_system, but the docs says about "simple usage". Who is right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The file name.

InvExponent(f32),
}
InvExponent(exp:f32) inv_exponent,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Newline.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


impl StyleWatchFrp {
/// Constructor.
#[allow(trivial_casts)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we allow trivial_casts here?

Copy link
Member Author

@wdanilo wdanilo Mar 30, 2021

Choose a reason for hiding this comment

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

Otherwise thsidoesnt compile let callback = Rc::new(RefCell::new(Box::new(||{}) as Box<dyn Fn()>)); And I don't see a way to make it nicer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So put that in the comment, so nobody will need to check that in the future :)

Comment on lines 273 to 281
// TODO[WD]: This impl should be uncommented, and the `self.update()` line removed,
// but now it causes project name to be red (to be investigated).
// // First theme set can skip lazy change, as this is normally done on app startup.
// // It will also make the startup faster, as the theme will not be updated on the next
// // frame, which would make all shaders re-compile.
// if self.initialized.get() {
// self.initialized.set(true);
// self.update()
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will you investigate it in this PR? If not, please add an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -32,7 +32,7 @@ const ARROW_SIZE_Y : f32 = 20.0;
const HOVER_EXTENSION : f32 = 10.0;

const MOUSE_OFFSET : f32 = 2.0;
const NODE_PADDING : f32 = node::SHADOW_SIZE;
const NODE_PADDING : f32 = 10.0; // node::SHADOW_SIZE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented code.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -132,7 +132,7 @@ ensogl::define_endpoints! {
set_hover (bool),
set_connected (bool,Option<Type>),
set_parent_connected (bool),
set_definition_type (Option<Type>),
set_definition_type (Option<Type>),
Copy link
Collaborator

Choose a reason for hiding this comment

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

unnecessary space.

Copy link
Member Author

Choose a reason for hiding this comment

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

its ok in code. Git shows that strangely

Comment on lines +203 to +204
// FIXME[WD]: Think how to refactor it, as it needs to be done before model, as we do not
// want shader recompilation. Model uses styles already.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is important and should be fixed in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be done after all styles are connected and theme manager works 100% correct. Now its impossible to be designed correctly IMO.

@farmaazon farmaazon merged commit c8bfe42 into develop Mar 30, 2021
@farmaazon farmaazon deleted the wip/wd/themes branch March 30, 2021 14:16
mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants