feat(183557950): Add ProjectsGrid View for Cloud Dashboard #3857

indiv0 · 2022-11-07T16:34:28Z

Pull Request Description

This PR is a draft PR while I learn EnsoGL. The eventual goal is to implement the projects list portion of the cloud dashboard in this PR. This PR will implement part of https://www.pivotaltracker.com/n/projects/2539513/stories/183557950

Important Notes

This PR is still really rough and contains a lot of hacks & hard-coded values. The FRP usage is also likely to be suboptimal and need fixing.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

wdanilo · 2022-11-22T17:08:18Z

app/gui/view/debug_scene/cloud-dashboard/cloud-http/src/lib.rs

+// =============
+// === Error ===
+// =============
+
+/// Type alias for a thread-safe boxed [`Error`].
+///
+/// Convert your error to this type when your error can not be recovered from, or no further context
+/// can be added to it.
+///
+/// [`Error`]: ::std::error::Error
+pub type Error = Box<dyn error::Error + Send + Sync>;


Let's refactor this to a common place, pls.

wdanilo · 2022-11-22T17:11:06Z

app/gui/view/debug_scene/cloud-dashboard/cloud-http/src/lib.rs

+/// This enum shouldn't be used directly by users of this crate, and instead they should interact
+/// with the Cloud API via the [`Client`] struct.
+///


lets keep it private then :)

wdanilo · 2022-11-22T17:12:35Z

app/gui/view/debug_scene/cloud-dashboard/cloud-http/src/lib.rs

+/// HTTP client for the Cloud API.
+#[derive(Clone, Debug)]
+pub struct Client {
+    base_url: String,
+    token:    String,
+    http:     reqwest::Client,
+}


Move the docs about fields here, pls.

wdanilo · 2022-11-22T17:16:38Z

app/gui/view/debug_scene/cloud-dashboard/cloud-http/src/lib.rs

+    /// * `api_gateway_id` - The unique identifier of the Amazon API Gateway that is fronting the
+    ///   Cloud API. This is used to disambiguate between requests to development, staging,
+    ///   production, etc. environments. This is obtained from the Terraform output after the API is
+    ///   deployed.
+    /// * `aws_region` - The AWS region in which the API is deployed. This is obtained from the
+    ///   Terraform output after the API is deployed.
+    /// * `token` - The authentication token to use when making requests to the API. This is
+    ///   obtained from the browser storage after the user is logged in.
+    ///
+    /// [`Client`]: crate::Client
+    pub fn new(api_gateway_id: &str, aws_region: &str, token: String) -> Self {


If we can use real types here instead of &str, I would do it. According to our phone call, this can be done by importing a lib, but please check if the lib does not increase the binary size drastically.

wdanilo · 2022-11-22T17:19:10Z

app/gui/view/debug_scene/cloud-dashboard/cloud-http/src/response.rs

+    // ====================
+
+    /// A response for a successful request to the [`ListProjects`] [`Route`].
+    ///
+    /// [`ListProjects`]: crate::Route::ListProjects
+    /// [`Route`]: crate::Route
+    #[derive(Clone, Debug, serde::Deserialize, serde::Serialize)]
+    #[allow(missing_docs)]
+    pub struct ListProjects {
+        /// A list of all [`Project`]s that the user has access to. The list may be empty, if the
+        /// user has access to no [`Project`]s or no [`Project`]s have been created.
+        ///
+        /// [`Project`]: ::enso_cloud_view::project::Project
+        pub projects: Vec<view::project::Project>,
+    }


This file contain responses, so we can generate here the Routs enum automatically.

wdanilo · 2022-11-22T18:38:58Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+            // represented by an arrow icon.
+            text:           column.to_string().into(),
+            disabled:       Immutable(true),
+            override_width: Immutable(if col == 1 && row == 5 { Some(180.0) } else { None }),


magic numbers

wdanilo · 2022-11-22T18:39:38Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+
+    fn get_projects(
+        client: enso_cloud_http::Client,
+        input: crate::projects_grid::api::public::Input,


You can pass here set_projects: frp::Source<...> and then you'd need to do set_projects.emit(...).

when I tried this I got:

Checking debug-scene-cloud-dashboard v0.1.0 (/home/indiv0/src/enso/app/gui/view/debug_scene/cloud-dashboard) error[E0308]: mismatched types --> app/gui/view/debug_scene/cloud-dashboard/src/lib.rs:554:30 | 554 | get_projects(client, input.set_projects); | ------------ ^^^^^^^^^^^^^^^^^^ expected struct `enso_frp::SourceData`, found struct `enso_frp::AnyData` | | | arguments to this function are incorrect | = note: expected struct `enso_frp::stream::WeakNode<enso_frp::SourceData<std::rc::Rc<std::vec::Vec<enso_cloud_view::project::Project>>>>` found struct `enso_frp::stream::WeakNode<enso_frp::AnyData<std::rc::Rc<std::vec::Vec<enso_cloud_view::project::Project>>>>` note: function defined here --> app/gui/view/debug_scene/cloud-dashboard/src/lib.rs:581:8 | 581 | fn get_projects( | ^^^^^^^^^^^^ 582 | client: enso_cloud_http::Client, 583 | set_projects: frp::Source<Rc<Vec<view::project::Project>>>, | ---------------------------------------------------------- For more information about this error, try `rustc --explain E0308`. error: could not compile `debug-scene-cloud-dashboard` due to previous error

How do I convert AnyData-marked Source to a SourceData-marked Source?

I don't see these lines in this file anymore, where are they now?

get_projects is now in the projects_grid module

wdanilo · 2022-11-22T18:40:17Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

        mem::forget(app);
-        mem::forget(panel);
+        mem::forget(projects_grid);


wdanilo · 2022-11-22T18:40:27Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

-        let panel = app.new_view::<sample_component::View>();
-        panel.set_size(Vector2::new(200.0, 200.0));
-        scene.add_child(&panel);
+        let _main_layer = &app.display.default_scene.layers.node_searcher;


wdanilo · 2022-11-22T18:40:32Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

@@ -211,13 +563,16 @@ pub fn main() {
        theme::builtin::light::register(&app);
        theme::builtin::light::enable(&app);

-        let world = &app.display;
+        let _world = &app.display;


indiv0 · 2022-11-28T01:43:42Z

The StyleWatchFrp changes are incoming

indiv0 · 2022-11-28T01:52:17Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+            requested_section <- header_frp.section_info_needed.map(|&(row, col)| {
+                let sections_size = 2 + col;
+                let section_start = row - (row % sections_size);
+                let section_end = section_start + sections_size;
+                let position = (section_start, col).into();
+                let model = header_entry_model(position);
+                (section_start..section_end, col, model)
+            });
+            header_frp.section_info <+ requested_section;


@farmaazon this code comes from one of the existing header view examples, but I'm having some trouble understanding what it's doing. What is the logic here supposed to be doing?

Generally, the grid view wants to know what to display as a header of the top of the viewport in the current column. Therefore, it asks about information about the section the topmost visible entry is in.

section_info_needed endpoint is exactly that: a question about the section the given entry is in.

IT also assumes that there may be more than one section, therefore it needs to know the span of rows belonging to the returned section (so it knows when to ask about another one).

So, the implementation copied from the example does not make much sense, of course, because you have only one section, so the range should be always 1..=self.rows().

Perfect, thank you :)

farmaazon

I checked just the one file with the grid view. The header requesting system shall be fixed (it should actually crash a lot if I understand the code correctly).

Also, there are many TODO or FIXME notes. Each shall be annotated with the pivotal issue, where the thing is planned to be done/fixed.

farmaazon · 2022-11-28T13:21:11Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+    /// The row and column coordinates of a cell in the [`ProjectsTable`].
+    #[derive(Clone, Copy, Debug, Default)]
+    pub struct Position {
+        row:    Row,
+        column: Col,
+    }


We may consider making it a part of the grid-view crate.

But after reading more of your code I see that you usually construct it from row and col only to decompose it again to two separate variables. So perhaps we could stay with (row: Row, col: Col) everywhere?

@wdanilo suggested to pass around the struct rather than the pair, to ensure that we're dealing with structured values rather than decomposing pairs, but I'm OK with either approach. @wdanilo what do you think?

@farmaazon I suggested that because in the code there was .0 and .1 in many places. It's a little bit hard to understand and its way easier to read code if you have .row and .column. Do you agree with this?

farmaazon · 2022-11-28T13:21:56Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+/// rendered.
+///
+/// [`Project`]: ::enso_cloud_view::project::Project
+pub mod projects_table {


This module is so long I would put it in a separate file.

farmaazon · 2022-11-28T13:29:53Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+            // The first row is the header row, which is displayed with a different entry model, so
+            // we should ignore it here.
+            if row == 0 {
+                return None;


This is a big function with several returning points. We avoid those as they make the code more obscure. Please refactor it, perhaps making some sub-methods.

farmaazon · 2022-11-28T13:32:20Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+            let screen_size = Vector2::from(shape);
+            let margin = Vector2::new(HORIZONTAL_MARGIN, VERTICAL_MARGIN);
+            let size = screen_size - margin;
+            self.grid.scroll_frp().resize(size);


The preferred way for calling FRP inputs is to assign them to FRP outputs. Usually I do it by making the model method returning the event payload (size in this case), and then assign the method output in frp::extend! with <+ operator (see my next comment for example).

farmaazon · 2022-11-28T13:36:43Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+
+            let width = shape.width / Columns::LEN as f32;
+            let size = Vector2(width, ENTRY_HEIGHT);
+            self.grid.set_entries_size(size);


Similar case: we could make this function return size, and have in FRP something like:
model.grid.set_entries_size <+ scene.frp.shape.map(f!((shape) model.refit_entries_to_shape(shape));

This way frp events have more informative backtraces.

farmaazon · 2022-11-28T13:48:38Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+                // FIXME [NP]: How do we stop the grid from calling `model_for_entry` on the
+                // header row? It should be handled via the later extension to the network.


It's impossible. And for the header row, you should just return headers. The "header" API is meant for the headers "pushed down" to be visible even when scrolled down.

farmaazon · 2022-11-28T13:52:56Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+    ensogl_core::define_endpoints_2! {
+        Input {
+            set_projects(Rc<Vec<view::project::Project>>),
+            model_for_entry(Position, EntryModel),


Why this output is here? Is it used?

farmaazon · 2022-11-28T13:59:15Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+    // FIXME [NP]: Should we be assuming that the `row, col` are always in bounds? If we do so, we
+    // can avoid having to return `Option` here, but we run the risk of panicking in the event of an
+    // out of bounds `row, col`. Though this should only happen on implementation error.


You mean, receive with section_info_needed? It should not emit any position outside the last size set by reset_entries or resize. But still, one could just put the wrong size on it.

And as I understand, we still have a rule of avoiding panicking at all cost, so you may assume that the position is valid, but still not panic ;) let's return some dummy entry, for example labeled "INVALID" instead.

farmaazon · 2022-11-28T14:05:58Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+            requested_section <- header_frp.section_info_needed.map(|&(row, col)| {
+                let sections_size = 2 + col;
+                let section_start = row - (row % sections_size);
+                let section_end = section_start + sections_size;
+                let position = (section_start, col).into();
+                let model = header_entry_model(position);
+                (section_start..section_end, col, model)
+            });
+            header_frp.section_info <+ requested_section;


Generally, the grid view wants to know what to display as a header of the top of the viewport in the current column. Therefore, it asks about information about the section the topmost visible entry is in.

section_info_needed endpoint is exactly that: a question about the section the given entry is in.

IT also assumes that there may be more than one section, therefore it needs to know the span of rows belonging to the returned section (so it knows when to ask about another one).

So, the implementation copied from the example does not make much sense, of course, because you have only one section, so the range should be always 1..=self.rows().

farmaazon · 2022-11-28T14:07:58Z

app/gui/view/debug_scene/cloud-dashboard/src/lib.rs

+    /// The width of a single entry in the [`ProjectsTable`], in pixels.
+    const ENTRY_WIDTH: f32 = 130.0;
+    /// The height of a single entry in the [`ProjectsTable`], in pixels.
+    const ENTRY_HEIGHT: f32 = 28.0;
+    /// A placeholder value for the initial height of the viewport. This value is a placeholder
+    /// because the value must be provided when creating the viewport, but is not used since the
+    /// viewport is resized to fit the [`ProjectsTable`] when the graphics context is initialized.
+    const VIEWPORT_HEIGHT: f32 = 300.0;
+    /// A placeholder value for the initial width of the viewport. This value is a placeholder
+    /// because the value must be provided when creating the viewport, but is not used since the
+    /// viewport is resized to fit the [`ProjectsTable`] when the graphics context is initialized.
+    const VIEWPORT_WIDTH: f32 = 400.0;


ENTRY_WIDTH ENTRY_HEIGHT and GRID_HEIGHT_RATIO should be taken from the styles.
And isn't the VIEWPORT_WIDTH calculated basing on the scene shape anyway?

PabloBuchu · 2022-12-01T13:28:21Z

app/gui/view/debug_scene/cloud-dashboard/cloud-http/src/lib.rs

+        aws_region: AwsRegion,
+        token: AccessToken,
+    ) -> Self {
+        let base_url = format!("https://{api_gateway_id}.execute-api.{aws_region}.amazonaws.com");


imho its better to have base_url as a parameter. We will move from apig generated api addresses in few days (the main will be api.cloud.enso.org so personal ones can be api.npekin.cloud.enso.org). This way you can set url as a env variable

This reverts commit a620f5e.

indiv0 requested a review from PabloBuchu November 7, 2022 16:34

indiv0 changed the base branch from develop to wip/wdanilo/min-table-scene November 7, 2022 16:35

indiv0 force-pushed the wip/npekin/ensogl-table-183557950 branch from decf54f to 971da45 Compare November 7, 2022 16:41

indiv0 changed the base branch from wip/wdanilo/min-table-scene to wip/michaelmauderer/cloud_dashboard_skeleton November 7, 2022 16:42

indiv0 force-pushed the wip/npekin/ensogl-table-183557950 branch from 971da45 to 879d7b5 Compare November 7, 2022 16:46

wdanilo reviewed Nov 22, 2022

View reviewed changes

indiv0 changed the base branch from wip/michaelmauderer/cloud_dashboard_skeleton to develop November 28, 2022 01:36

indiv0 force-pushed the wip/npekin/ensogl-table-183557950 branch 2 times, most recently from 302dadf to 136588d Compare November 28, 2022 01:41

indiv0 requested a review from MichaelMauderer November 28, 2022 01:42

indiv0 self-assigned this Nov 28, 2022

indiv0 changed the title ~~[DRAFT] feat(183557950): Add ProjectsGrid View for Cloud Dashboard~~ feat(183557950): Add ProjectsGrid View for Cloud Dashboard Nov 28, 2022

indiv0 commented Nov 28, 2022

View reviewed changes

indiv0 marked this pull request as ready for review November 28, 2022 01:52

indiv0 requested review from farmaazon, mwu-tow and kazcw as code owners November 28, 2022 01:52

farmaazon requested changes Nov 28, 2022

View reviewed changes

wdanilo and others added 11 commits November 28, 2022 15:32

Preparing simple grid view example

7be86ee

fix

ed9ebeb

fix

aa24a2d

re-add example file

dc4cff8

fixes

9efb714

fixes

8610793

refr projects_table to file

7f0dfeb

fixes

d5b6182

fixes

777189b

fixes

9105568

fixes

1e48922

indiv0 added 10 commits December 1, 2022 07:34

format

38b937f

derive deref

fa070b3

tmp: remove application field

64bcfff

move app from model to view

2b87950

format

1ea4f66

use checked_sub for project_index_for_entry

bcc0ead

remove unused model_for_entry

b50520a

rewrite section code

fa011b0

remove unused fields

ac3cae8

impr: use model_for_entry for both header and regular entry model

5b66e56

PabloBuchu reviewed Dec 1, 2022

View reviewed changes

indiv0 added 17 commits December 1, 2022 09:00

switch to anyhow

a620f5e

use proper url type for the base url

baa5a5e

Revert "switch to anyhow"

17ba0a4

This reverts commit a620f5e.

propagate error

4ea66ec

improve error handling

2d74001

move split_twice

189331e

add token validation

a05ab6a

make base_url a parameter

a950c99

fmt

a8750d9

Merge branch 'develop' into wip/npekin/ensogl-table-183557950

7f1264e

update cargo.lock

a1a6efc

update changelog

5ce4b71

increase build size

9bfb942

fix clippy lint

4480ea8

make module public

1aa7b21

remove late-init

4fe307d

Merge branch 'develop' into wip/npekin/ensogl-table-183557950

beb4c9f

mwu-tow merged commit bd455ff into develop Dec 4, 2022

mwu-tow deleted the wip/npekin/ensogl-table-183557950 branch December 4, 2022 04:41

		// FIXME [NP]: How do we stop the grid from calling `model_for_entry` on the
		// header row? It should be handled via the later extension to the network.

feat(183557950): Add ProjectsGrid View for Cloud Dashboard #3857

feat(183557950): Add ProjectsGrid View for Cloud Dashboard #3857

Conversation

indiv0 commented Nov 7, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indiv0 commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indiv0 commented Nov 7, 2022 •

edited

Loading