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

Implement area selection. #1588

Merged
merged 13 commits into from Jun 13, 2021

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented May 26, 2021

Pull Request Description

Implement working area selection as per #479

Peek.2021-05-26.15-27.mp4
Peek.2021-05-26.15-29.mp4
Peek.2021-06-03.13-22.mp4

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.

@MichaelMauderer MichaelMauderer added Difficulty: Hard Significant prior knowledge requried Priority: High Should be scheduled as soon as possible Type: Enhancement An enhancement to the current state of Enso IDE Category: GUI The Graphical User Interface labels May 26, 2021
@MichaelMauderer MichaelMauderer self-assigned this May 26, 2021
@MichaelMauderer MichaelMauderer marked this pull request as ready for review May 26, 2021 13:41
src/rust/ide/view/graph-editor/src/lib.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/lib.rs Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/lib.rs Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/selection.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/selection.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/selection.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/selection.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/selection.rs Outdated Show resolved Hide resolved
Co-authored-by: Adam Obuchowicz <adam.obuchowicz@luna-lang.org>
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

The movies are super fast - could you make also a movie where you move your mouse super slow, so it will be seen what triggers node to be selected, please? Also, could you include corner selection (corner of area overlapping with the corner of the node) in that video as well, please?

src/rust/ide/view/graph-editor/src/component/node.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/component/node.rs Outdated Show resolved Hide resolved
visualisations : Visualisations,
frp : FrpEndpoints,
navigator : Navigator,
selection_controller : selection::Controller,
Copy link
Member

Choose a reason for hiding this comment

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

TBH I dont like the name "selection_controller". We are using "controller" in 2 meanings so far:

  1. As the "backend GUI part.
  2. As a visual widget to control things, like bezier curve controller.

This would be a third meaning for it. Also, by reading the name I do not really understand what this abstraction does, so the name may not be the best here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(discussion for this on your other comment below)

Comment on lines 380 to 381
pub fn new(inputs:&crate::FrpEndpoints,out:&crate::FrpEndpoints,cursor:&Cursor,
mouse:&frp::io::Mouse,touch:&TouchState,nodes:&Nodes)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the layout of the code is correct:

  1. This module requires FRP endpoints defined in another module, which then uses this module. Such circular dependencies are insanely hard to track and understand.
  2. Both inputs and outputs use the same type, and if Im not wrong, you are passing the same thing twice here, arent you?

Basically, I don't like the whole coupling of this thing. It is coupled in other places as well - like the cursor. Instead of taking it as an argument, it could just expose an input FRP stream which could be plugged from outside.

I really like it is extracted from graph editor, but still I feel it is too much coupled now. Lets have a talk about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(2) Yes you are right, this is changed.

This is essentially a sub-component of the graph-editor that is meant to be in charge of a set of operations related to selections. To achieve that, it does need to access the graph editors API and some application data. I'm not convinced that it makes sense to completely decouple it from the graph editor.

If we did want to decouple it we could just try to turn it into an independent FRP API that takes all required inputs and produces the required outputs that then get hooked up to a matching API in the graph editor.

That could look like this (just a rough outline, some types might be missing, and some things might be redundant or easier to achieve another way by moving key bindings to this entity

ensogl::define_endpoints! {
    Input {
        cursor_position(),
    
        mouse_down_primary(),
        mouse_position(),
        mouse_up_primary(),
    
        touch_background_is_down(),
        touch_background_selected(),
        touch_nodes_selected(),
        touch_nodes_down(),
    
        editor_edit_mode_off(),
        editor_edit_mode_on(),
        editor_some_edge_endpoints_unset(),
        editor_select_node(NodeId),
        editor_deselect_all_nodes(),
        editor_enable_node_multi_select(),
        editor_disable_node_multi_select(),
        editor_toggle_node_multi_select(),
        editor_enable_node_merge_select(),
        editor_disable_node_merge_select(),
        editor_toggle_node_merge_select(),        
        editor_enable_node_subtract_select(),
        editor_disable_subtract_select(),
        editor_toggle_subtract_select(),      
        editor_enable_node_inverse_select(),
        editor_disable_inverse_select(),
        editor_toggle_inverse_select(),
    }
    Output {
        node_selected(NodeId),
        node_deselected(NodeId),
   }
}

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is WAY more than I anticipated when writing my comment. Let's leave it as is then!

src/rust/ide/view/graph-editor/src/lib.rs Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/lib.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/selection.rs Outdated Show resolved Hide resolved
@MichaelMauderer
Copy link
Contributor Author

The movies are super fast - could you make also a movie where you move your mouse super slow, so it will be seen what triggers node to be selected, please? Also, could you include corner selection (corner of area overlapping with the corner of the node) in that video as well, please?

I've added a new video that is slower and starts a selection from the corners.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
Comment on lines 25 to 26
[1588]: https://github.com/enso-org/ide/pull/1588

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
[1588]: https://github.com/enso-org/ide/pull/1588
[1588]: https://github.com/enso-org/ide/pull/1588

src/rust/ide/view/graph-editor/src/lib.rs Outdated Show resolved Hide resolved
src/rust/ide/view/graph-editor/src/selection.rs Outdated Show resolved Hide resolved
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Thanks, Michael.
I've noticed on the video a strange thing - although the selection was already above the node, the node did not got selected. This looks like a misbehavior to me:

image

Also, in a selection mode, can we not show the icons on the right side of the nodes nor output ports on hover? When selecting nodes, users are not able to manipulate them, so there is no point in cluttering the view with elements that cannot be manipulated.

Beside that, I'm OK with it, so I'm accepting the task - it can be merged as soon as the above is fixed AND it is tested by someone.

MichaelMauderer and others added 3 commits June 10, 2021 16:32
Co-authored-by: Adam Obuchowicz <adam.obuchowicz@luna-lang.org>
…or API. Account for padding during selection.
# Conflicts:
#	CHANGELOG.md
#	src/rust/ide/view/graph-editor/src/component/node.rs
#	src/rust/ide/view/graph-editor/src/lib.rs
@MichaelMauderer
Copy link
Contributor Author

MichaelMauderer commented Jun 10, 2021

@wdanilo I've made the changes and resolved the issue.

Peek.2021-06-10.17-02.mp4

@mwu-tow
Copy link
Collaborator

mwu-tow commented Jun 11, 2021

I have done testing for this PR.
I've found one issue: it is possible to drag selection area in the fullscreen visualization. Also, slider in the fullscreen visualization does not work -- but I believe it was already like this.

@MichaelMauderer MichaelMauderer merged commit db31134 into develop Jun 13, 2021
@MichaelMauderer MichaelMauderer deleted the wip/mm/ide-479--make-area-selection-work branch June 13, 2021 19:52
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
Category: GUI The Graphical User Interface Difficulty: Hard Significant prior knowledge requried Priority: High Should be scheduled as soon as possible Type: Enhancement An enhancement to the current state of Enso IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants