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

Open Project Dialog #5607

Merged
merged 14 commits into from
Feb 20, 2023
Merged

Open Project Dialog #5607

merged 14 commits into from
Feb 20, 2023

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented Feb 9, 2023

Pull Request Description

Closes #5022

This is basically a reimplementation of the Open Project Dialog that was present in the IDE a while ago. Now it uses the modern shiny grid-view instead of the old rusty list-view.

2023-02-15.18-14-32.mp4

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.

@vitvakatu vitvakatu added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 9, 2023
@vitvakatu vitvakatu self-assigned this Feb 9, 2023
Comment on lines 428 to 442
fn attach_frp_to_project_list_notifications(
self,
notifications: Publisher<ProjectListNotification>,
project_list_ready: frp::Source<()>,
) -> Self {
let weak = Rc::downgrade(&self.model);
let notifications = notifications.subscribe();
spawn_stream_handler(weak, notifications, move |notification, _| {
match notification {
ProjectListNotification::ProjectListReady => project_list_ready.emit(()),
}
std::future::ready(())
});
self
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it's a bit strange we make a publisher to publish notifications for ourselves. Cannot we just call "reset_entries" on project list in the async block where we wait for new list load?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I guess it's a lot easier :) Done.

Comment on lines +39 to +50

/// Show a spinner covering the whole viewport.
#[allow(unsafe_code)]
#[wasm_bindgen(method)]
#[wasm_bindgen(js_name = showProgressIndicator)]
pub fn show_progress_indicator(this: &App, progress: f32);

/// Hide a spinner.
#[allow(unsafe_code)]
#[wasm_bindgen(method)]
#[wasm_bindgen(js_name = hideProgressIndicator)]
pub fn hide_progress_indicator(this: &App);
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere we hide the progress indicator after project load. Can we use this method there? I think I didn't see it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't because it is the responsibility of the JS code that loads WASM. Spinner is tracking download progress and is hidden after it is done automatically.

Copy link
Member

Choose a reason for hiding this comment

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

We CAN reuse the spinner if we want. Its API is exposed by JS app that we are connected to in Rust. I can help with it if we want 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.

@wdanilo what do you mean? We are using a spinner from JS app, I was just forced to add a bit of code to make it work from Rust (show_progress_indicator and hide_progress_indicator methods).

@vitvakatu vitvakatu requested a review from farmaazon February 10, 2023 10:20
@galin-enso
Copy link
Contributor

QA passed

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Feb 13, 2023
@vitvakatu vitvakatu removed the CI: Ready to merge This PR is eligible for automatic merge label Feb 13, 2023
@vitvakatu
Copy link
Contributor Author

Updated the code to correctly close the project before opening a new one, now, the nodes from the previous project are not visible. The video in the description was updated.

@vitvakatu vitvakatu requested a review from farmaazon February 15, 2023 14:17
app/gui/src/controller/ide.rs Outdated Show resolved Hide resolved
app/gui/src/presenter.rs Show resolved Hide resolved
@MichaelMauderer
Copy link
Contributor

QA
For me the selection highlight of the project items is not working

Peek.2023-02-15.15-08.mp4

@vitvakatu
Copy link
Contributor Author

@MichaelMauderer, that's weird, it works for me.

  1. Can you select entries still?
  2. Do you have the grid view demo scene working / other places where grid view is used (CB and its section navigator)?

@MichaelMauderer
Copy link
Contributor

MichaelMauderer commented Feb 15, 2023

Can you select entries still?

Yes, I can select them and opening a new project seems to work.

Do you have the grid view demo scene working / other places where grid view is used (CB and its section navigator)?

The demo scene and the CB seem to have the correct highlights.

@farmaazon
Copy link
Contributor

Perhaps the shape ordering is undefined?

@vitvakatu
Copy link
Contributor Author

@MichaelMauderer I moved the grid to the dedicated layer, could you check it?

@MichaelMauderer
Copy link
Contributor

Still does not appear. There appears to be a shape visible in the debug layers, see the video.

Peek.2023-02-16.10-54.mp4

I'm seeing the same behaviour between Chromium and Firefox (both on Linux). I can try a different machine / OS and the electron package if that helps.

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.

Please add spacing in these places:
image

Just like in component browser - the highlights should not touch the borders of the container they are in. Use the same spacing parameters as we use in component browser please.

(rows, cols)
}));
entry_model <- project_list.grid.model_for_entry_needed.map(f!([model]((row, _)) {
model.available_projects.borrow().get(*row).map(|(name, _)| (*row, 0, name.clone_ref()))
Copy link
Member

Choose a reason for hiding this comment

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

this is super complex. Also, the 0 is magic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +39 to +50

/// Show a spinner covering the whole viewport.
#[allow(unsafe_code)]
#[wasm_bindgen(method)]
#[wasm_bindgen(js_name = showProgressIndicator)]
pub fn show_progress_indicator(this: &App, progress: f32);

/// Hide a spinner.
#[allow(unsafe_code)]
#[wasm_bindgen(method)]
#[wasm_bindgen(js_name = hideProgressIndicator)]
pub fn hide_progress_indicator(this: &App);
Copy link
Member

Choose a reason for hiding this comment

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

We CAN reuse the spinner if we want. Its API is exposed by JS app that we are connected to in Rust. I can help with it if we want it.

@wdanilo
Copy link
Member

wdanilo commented Feb 19, 2023

Still does not appear. There appears to be a shape visible in the debug layers, see the video.

I'm seeing the same behaviour between Chromium and Firefox (both on Linux). I can try a different machine / OS and the electron package if that helps.

Regarding this - please remember that if you do not set explicitly the ordering of shapes, it is random and can depend on different factors, liveth order of shapes created, which can be different on different machines. Looks to me like just a lack of shapes ordering specification :)

@vitvakatu
Copy link
Contributor Author

I changed the hover color – the issue was caused by OpenGL bug on my machine that changed the colors for me. Also added paddings as @wdanilo asked, now the window looks like this:

image

(please ignore black hover color, it is an OpenGL bug #4964)

@MichaelMauderer
Copy link
Contributor

QA: Passed.

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Feb 20, 2023
@mergify mergify bot merged commit 19beb01 into develop Feb 20, 2023
@mergify mergify bot deleted the wip/vitvakatu/open-dialog-5022 branch February 20, 2023 14:47
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. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open Project dialog is empty
5 participants