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

Passing events to sub-widgets in List Editor and refactoring of the slider component. #6433

Merged
merged 12 commits into from
Apr 27, 2023

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented Apr 26, 2023

Pull Request Description

Fixes #6428 .
This PR improves List Editor component and refactors the Slider component.

List Editor Improvements

It is possible now to use complex widgets as List Editor items (previously dragging them was not possible). Also, now the mouse events are passed to the items if the item is not being dragged:

Screen.Recording.2023-04-26.at.15.24.01.mov

Slider Improvements

The slider code was heavily refactored to make it use the most modern EnsoGL API. In particular:

  • The track / thumb shape was unified, so less shapes are used.
  • The slider size is now controlled by display objects, rather than manually controlled by FRP endpoints.
  • The slider start, middle, and end part of the track can be dragged now.
  • The slider origin is now in it's left-bottom corner.
Screen.Recording.2023-04-26.at.15.31.40.mov

However, the slider code after the update do not contain all the functionality of the old implementation. In particular, the following things do not work yet:

  • Vertical sliders.
  • Overflow values.

As the slider is not used in real places, we are keeping it as is for now, while the created changes were needed to test it's behavior with List Editor component.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@wdanilo wdanilo added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 26, 2023
@wdanilo wdanilo marked this pull request as ready for review April 26, 2023 13:32

insert((Index, Weak<T>)),
insert((Index, Rc<RefCell<Option<T>>>)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this has no docstring while other two inputs do?

@@ -362,7 +358,7 @@ impl<T: display::Object + CloneRef> ListEditor<T> {

// Do not pass events to children, as we don't know whether we are about to drag
// them yet.
eval on_down ([] (event) event.stop_propagation());

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the comment above remains up-to-date after the change?


on_item_removed(Response<(Index, Weak<T>)>),
on_item_removed(Response<(Index, Rc<RefCell<Option<T>>>)>),
Copy link
Contributor

Choose a reason for hiding this comment

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

The same question as above.

slider10.frp.orientation(Axis2::Y);
self.root.add_child(&slider10);
self.sliders.borrow_mut().push(slider10);
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Tons of commented-out code. Is it needed? If so, why is it commented?

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 is explained in the PR description and in the file - the commented code needs to be uncommented and updated one day. For now, the slider implementation is improved in some places, but some places from the old implementation are not working yet.

mem::forget(frp);
mem::forget(navigator);
mem::forget(root);
mem::forget(vector_editor);
Copy link
Contributor

Choose a reason for hiding this comment

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

If lifetimes of these objects are tied, shouldn't they be grouped together in a single struct?

Copy link
Member Author

Choose a reason for hiding this comment

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

As agreed via Discord, let's leave it as it. It is a debug scene and we are doing so there as this is less boilerplate than creating structures. But generally, yes, we should think about managing that nicer in debug scenes in general.

@wdanilo wdanilo merged commit ae94a9f into develop Apr 27, 2023
@wdanilo wdanilo deleted the wip/wd/vector-editor5 branch April 27, 2023 02:42
Procrat added a commit that referenced this pull request Apr 27, 2023
* develop:
  Passing events to sub-widgets in List Editor and refactoring of the slider component. (#6433)
  Revert "Cloud/desktop mode switcher (#6308)" (#6444)
  Widgets integrated with graph nodes (#6347)
  Table Visualization and display text changes. (#6382)
  Skip redundant compilations (#6436)
  Add parse extensions to Text type. #6330 (#6404)
  Cloud/desktop mode switcher (#6308)
  Replace Table should_equal with should_equal_verbose (#6405)
  Rollback event handling changes for the mouse (#6396)
  Dashboard directory interactivity (#6279)
  Ability to change the execution environment between design and live. (#6341)
  Implementation of elements adding to List Editor and a lot of internal API (#6390)
  Drop method exported from WASM + removing leaks. (#6365)
  Turn null into UnexpectedExpression when Union type is incomplete (#6415)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vector_Editor component allowing passing events to sub-widgets.
2 participants